From 55c573328b79aee96e24e027cf1bc65374089dbf Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 1 Aug 2026 17:07:59 +0200 Subject: [PATCH 1/3] feat: answer `mimetype` without libmagic, and deprecate the option `odr::mimetype` was the only thing libmagic ever did for us, and it did it worse than we can. It cannot look inside a zip or a compound file binary, so an odt came back as `application/zip` and a doc as whatever the CFB header suggested; it gets `svm` wrong; and it costs a dependency plus the 8 MB `magic.mgc` database that every consumer has to ship and then point us at. It now runs the open strategy and reports the last type that came out of it, so a container is named by what it holds. `ODR_WITH_LIBMAGIC` and the conan `with_libmagic` option stay, deprecated and off by default, for whoever still relies on the old answers; one CI job keeps asking for them so the code goes on compiling and linking until it is removed. `docs/design/README.md` lists what goes with it. The signature table is what answers for everything the open strategy does not open, so it grew the media formats a viewer is regularly handed alongside documents - webp, tiff, heif, avif, mp3, m4a, ogg, wav, flac, mp4, mov, 3gp, matroska, avi - with `audio` and `video` file categories. Naming them is the point: a caller that knows a file is a video hands it to a player, where before the text fallback would have called it plain text. They are detected only, never decoded, so `detect_by_content` is the single capability they carry. New enumerators are appended, because the java, python and objc mirrors follow this enum by ordinal. `magic::file_type(std::istream &)` matched a short file against the uninitialized stack behind what it actually read; it now reads into a value initialized buffer and matches only `gcount()` bytes of it. The magic tests asserted the mimetype only under `has_libmagic()`, which no job that runs the suite had turned on, so the branch this PR makes the default was never exercised. They now assert both answers, and `svm` - the one format the two disagree on - asserts each side. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01C8zVbqCiCedPBvigwpt5ta --- .github/workflows/build_test.yml | 9 +- AGENTS.md | 6 +- CMakeLists.txt | 12 +- android/build_native.py | 1 - apple/CMakeLists.txt | 8 +- apple/build_xcframework.py | 1 - apple/include/OdrCoreObjC/ODRFile.h | 19 +++ apple/include/OdrCoreObjC/ODRGlobalParams.h | 3 + apple/src/ODRFile.mm | 22 +++ conanfile.py | 3 +- docs/design/README.md | 9 + .../app/opendocument/core/FileCategory.java | 2 +- jni/java/app/opendocument/core/FileType.java | 6 +- .../app/opendocument/core/GlobalParams.java | 9 + pyproject.toml | 11 +- python/pyodr/__init__.py | 7 +- python/src/bind_core.cpp | 2 + python/src/bind_file.cpp | 24 ++- python/tests/test_meta.py | 21 +-- src/odr/file.cpp | 4 +- src/odr/file.hpp | 39 +++++ src/odr/global_params.hpp | 3 + src/odr/internal/file_type_table.cpp | 156 ++++++++++++++++++ src/odr/internal/magic.cpp | 118 ++++++++++++- src/odr/internal/magic.hpp | 5 +- src/odr/odr.cpp | 4 + test/src/internal/magic_test.cpp | 121 ++++++++++++-- test/src/odr_test.cpp | 2 +- 28 files changed, 559 insertions(+), 68 deletions(-) diff --git a/.github/workflows/build_test.yml b/.github/workflows/build_test.yml index d2a1986b6..6b8000134 100644 --- a/.github/workflows/build_test.yml +++ b/.github/workflows/build_test.yml @@ -44,7 +44,10 @@ jobs: matrix: include: - { os: ubuntu-24.04, build_profile: ubuntu-24.04-clang-18, host_profile: ubuntu-24.04-clang-18, bindings: true } - - { os: ubuntu-24.04, build_profile: ubuntu-24.04-gcc-14, host_profile: ubuntu-24.04-gcc-14 } + # the one job that still asks for the deprecated libmagic, so it keeps + # compiling and linking until it goes. everything else, `test` + # included, builds the default and detects on its own + - { os: ubuntu-24.04, build_profile: ubuntu-24.04-gcc-14, host_profile: ubuntu-24.04-gcc-14, libmagic: true } - { os: macos-15, build_profile: macos-15-armv8-clang-14, host_profile: macos-15-armv8-clang-14 } - { os: macos-26, build_profile: macos-26-armv8-clang-14, host_profile: macos-26-armv8-clang-14, bindings: true } - { os: windows-2022, build_profile: windows-2022-msvc-1940, host_profile: windows-2022-msvc-1940 } @@ -111,6 +114,7 @@ jobs: run: > conan install . ${{ matrix.bindings && '-o "&:with_jni=True" -o "&:with_python=True"' || '' }} + ${{ matrix.libmagic && '-o "&:with_libmagic=True"' || '' }} --profile:host '${{ matrix.host_profile }}' --profile:build '${{ matrix.build_profile }}' --build missing @@ -125,8 +129,8 @@ jobs: -DCMAKE_CXX_FLAGS="-Werror" -DCMAKE_INSTALL_PREFIX=install -DODR_TEST=ON - -DODR_WITH_LIBMAGIC=ON -DODR_BUNDLE_ASSETS=ON + ${{ matrix.libmagic && '-DODR_WITH_LIBMAGIC=ON' || '' }} ${{ matrix.bindings && '-DODR_JNI=ON -DODR_PYTHON=ON' || '' }} - name: cmake @@ -138,7 +142,6 @@ jobs: -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=install -DODR_TEST=ON - -DODR_WITH_LIBMAGIC=OFF - name: build run: cmake --build build --config Release diff --git a/AGENTS.md b/AGENTS.md index f356c6096..ec534e85a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,9 +18,11 @@ bytes ─▶ magic/open_strategy ─▶ DecodedFile ─▶ Document ─▶ Eleme DecoderEngine) format) elements) walks public API) ``` -1. **Detect** — `internal/magic.cpp` (+ `internal/libmagic`) sniffs the file; +1. **Detect** — `internal/magic.cpp` sniffs the head of the file; `internal/open_strategy.cpp` picks a `FileType` + `DecoderEngine` and builds - the matching `abstract::DecodedFile`. + the matching `abstract::DecodedFile`. `odr::mimetype` composes the two, so a + zip is named by what is inside it. (`internal/libmagic` is a deprecated + alternative to that last step, off by default.) 2. **Decode** — a document file yields an `abstract::Document`. 3. **Element tree** — a `Document` exposes a root `ElementIdentifier` plus an `abstract::ElementAdapter`. Public value-semantics handles (`Element`, `Slide`, diff --git a/CMakeLists.txt b/CMakeLists.txt index 6a26c63cd..1d984ba7b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -16,7 +16,12 @@ option(ODR_TEST_FETCH_DATA "Fetch the test data pinned in test/data.cmake" OFF) option(ODR_CLI "enable command line interface" ON) option(ODR_WITH_HTTP_SERVER "Build the HTTP server (requires cpp-httplib)" ON) option(ODR_CLANG_TIDY "Run clang-tidy static analysis" OFF) -option(ODR_WITH_LIBMAGIC "Build with libmagic" ON) +# Deprecated, and off by default since our own detection took `mimetype` over: +# that reads inside zip and cfb containers, which libmagic cannot, and it costs +# neither a dependency nor the 8 MB `magic.mgc` database every consumer has to +# ship and point us at. Kept for whoever still relies on the old answers; see +# "Open tasks" in docs/design/README.md. +option(ODR_WITH_LIBMAGIC "Build with libmagic (deprecated)" OFF) option(ODR_BUNDLE_ASSETS "Bundle assets during build and install" OFF) option(ODR_PYTHON "Build Python bindings" OFF) option(ODR_JNI "Build JNI bindings" OFF) @@ -314,6 +319,11 @@ if (ODR_WITH_HTTP_SERVER) ) endif () if (ODR_WITH_LIBMAGIC) + message(DEPRECATION + "ODR_WITH_LIBMAGIC is deprecated and will be removed. Our own " + "detection answers `odr::mimetype` without it, and better for " + "container formats.") + find_package(libmagic REQUIRED) target_sources(odr PRIVATE diff --git a/android/build_native.py b/android/build_native.py index a11c51fff..79a55d426 100644 --- a/android/build_native.py +++ b/android/build_native.py @@ -99,7 +99,6 @@ def build(architecture: str, conan: str, build_profile: str, output: Path) -> No "-DODR_JNI=ON", "-DODR_CLI=OFF", "-DODR_TEST=OFF", - "-DODR_WITH_LIBMAGIC=ON", "-DODR_WITH_HTTP_SERVER=ON", "-DODR_BUNDLE_ASSETS=ON"]) run(["cmake", "--build", cmake_dir, "--target", "odr_jni", diff --git a/apple/CMakeLists.txt b/apple/CMakeLists.txt index ca150c402..70c63accf 100644 --- a/apple/CMakeLists.txt +++ b/apple/CMakeLists.txt @@ -17,8 +17,8 @@ if (BUILD_SHARED_LIBS) endif () if (NOT ODR_BUNDLE_ASSETS) message(FATAL_ERROR - "ODR_APPLE needs ODR_BUNDLE_ASSETS=ON: the renderer's css/js and " - "the libmagic database ship as bundle resources") + "ODR_APPLE needs ODR_BUNDLE_ASSETS=ON: the renderer's css/js ship " + "as bundle resources") endif () enable_language(OBJCXX) @@ -64,8 +64,8 @@ set(ODR_APPLE_PUBLIC_HEADERS "include/OdrCoreObjC/ODRTable.h" ) -# The renderer data the root build already staged, plus the libmagic database -# `ODR_BUNDLE_ASSETS` copied in beside it. +# Whatever the root build staged: the renderer data, and `magic.mgc` beside it +# in the deprecated builds that still ask for libmagic. file(GLOB ODR_APPLE_RESOURCES "${ODR_BUILD_ODR_DATA_PATH}/*") if (NOT ODR_APPLE_RESOURCES) message(FATAL_ERROR "no resources in ${ODR_BUILD_ODR_DATA_PATH}") diff --git a/apple/build_xcframework.py b/apple/build_xcframework.py index d9050f837..5069055c0 100755 --- a/apple/build_xcframework.py +++ b/apple/build_xcframework.py @@ -110,7 +110,6 @@ def build(profile: str, conan: str, build_profile: str) -> None: "-DODR_TEST=OFF", "-DODR_JNI=OFF", "-DODR_PYTHON=OFF", - "-DODR_WITH_LIBMAGIC=ON", "-DODR_WITH_HTTP_SERVER=ON", "-DODR_BUNDLE_ASSETS=ON"]) run(["cmake", "--build", cmake_dir, "--target", "odr_apple", diff --git a/apple/include/OdrCoreObjC/ODRFile.h b/apple/include/OdrCoreObjC/ODRFile.h index b3d3f2626..e0a8430ef 100644 --- a/apple/include/OdrCoreObjC/ODRFile.h +++ b/apple/include/OdrCoreObjC/ODRFile.h @@ -56,6 +56,23 @@ typedef NS_ENUM(NSInteger, ODRFileType) { ODRFileTypeTruetypeFont, ODRFileTypeOpentypeFont, + + ODRFileTypeWebp, + ODRFileTypeTaggedImageFileFormat, + ODRFileTypeHighEfficiencyImageFormat, + ODRFileTypeAv1ImageFileFormat, + + ODRFileTypeMpegAudio, + ODRFileTypeMpeg4Audio, + ODRFileTypeOggAudio, + ODRFileTypeWaveformAudio, + ODRFileTypeFreeLosslessAudioCodec, + + ODRFileTypeMpeg4Video, + ODRFileTypeQuicktimeVideo, + ODRFileTypeThirdGenerationPartnershipVideo, + ODRFileTypeMatroskaVideo, + ODRFileTypeAudioVideoInterleave, } NS_SWIFT_NAME(FileType); typedef NS_ENUM(NSInteger, ODRFileCategory) { @@ -65,6 +82,8 @@ typedef NS_ENUM(NSInteger, ODRFileCategory) { ODRFileCategoryArchive, ODRFileCategoryDocument, ODRFileCategoryFont, + ODRFileCategoryAudio, + ODRFileCategoryVideo, } NS_SWIFT_NAME(FileCategory); typedef NS_ENUM(NSInteger, ODRFileLocation) { diff --git a/apple/include/OdrCoreObjC/ODRGlobalParams.h b/apple/include/OdrCoreObjC/ODRGlobalParams.h index 47f94e9cf..1649aff9f 100644 --- a/apple/include/OdrCoreObjC/ODRGlobalParams.h +++ b/apple/include/OdrCoreObjC/ODRGlobalParams.h @@ -14,6 +14,9 @@ NS_SWIFT_NAME(GlobalParams) /// The css and js of the HTML renderer. @property(class, nonatomic, copy) NSString *odrCoreDataPath; /// The libmagic database (`magic.mgc`). +/// +/// Deprecated: read only by a core built with `ODR_WITH_LIBMAGIC`, which is off +/// by default now that detection is odrcore's own and needs no database. @property(class, nonatomic, copy) NSString *libmagicDatabasePath; /// Points the paths above at this framework's bundle. Runs automatically at diff --git a/apple/src/ODRFile.mm b/apple/src/ODRFile.mm index 2df888cf7..abbddfc95 100644 --- a/apple/src/ODRFile.mm +++ b/apple/src/ODRFile.mm @@ -61,6 +61,26 @@ ODR_SAME_ENUM(ODRFileTypeStarviewMetafile, odr::FileType::starview_metafile); ODR_SAME_ENUM(ODRFileTypeTruetypeFont, odr::FileType::truetype_font); ODR_SAME_ENUM(ODRFileTypeOpentypeFont, odr::FileType::opentype_font); +ODR_SAME_ENUM(ODRFileTypeWebp, odr::FileType::webp); +ODR_SAME_ENUM(ODRFileTypeTaggedImageFileFormat, + odr::FileType::tagged_image_file_format); +ODR_SAME_ENUM(ODRFileTypeHighEfficiencyImageFormat, + odr::FileType::high_efficiency_image_format); +ODR_SAME_ENUM(ODRFileTypeAv1ImageFileFormat, + odr::FileType::av1_image_file_format); +ODR_SAME_ENUM(ODRFileTypeMpegAudio, odr::FileType::mpeg_audio); +ODR_SAME_ENUM(ODRFileTypeMpeg4Audio, odr::FileType::mpeg4_audio); +ODR_SAME_ENUM(ODRFileTypeOggAudio, odr::FileType::ogg_audio); +ODR_SAME_ENUM(ODRFileTypeWaveformAudio, odr::FileType::waveform_audio); +ODR_SAME_ENUM(ODRFileTypeFreeLosslessAudioCodec, + odr::FileType::free_lossless_audio_codec); +ODR_SAME_ENUM(ODRFileTypeMpeg4Video, odr::FileType::mpeg4_video); +ODR_SAME_ENUM(ODRFileTypeQuicktimeVideo, odr::FileType::quicktime_video); +ODR_SAME_ENUM(ODRFileTypeThirdGenerationPartnershipVideo, + odr::FileType::third_generation_partnership_video); +ODR_SAME_ENUM(ODRFileTypeMatroskaVideo, odr::FileType::matroska_video); +ODR_SAME_ENUM(ODRFileTypeAudioVideoInterleave, + odr::FileType::audio_video_interleave); ODR_SAME_ENUM(ODRFileCategoryUnknown, odr::FileCategory::unknown); ODR_SAME_ENUM(ODRFileCategoryText, odr::FileCategory::text); @@ -68,6 +88,8 @@ ODR_SAME_ENUM(ODRFileCategoryArchive, odr::FileCategory::archive); ODR_SAME_ENUM(ODRFileCategoryDocument, odr::FileCategory::document); ODR_SAME_ENUM(ODRFileCategoryFont, odr::FileCategory::font); +ODR_SAME_ENUM(ODRFileCategoryAudio, odr::FileCategory::audio); +ODR_SAME_ENUM(ODRFileCategoryVideo, odr::FileCategory::video); ODR_SAME_ENUM(ODRFileLocationMemory, odr::FileLocation::memory); ODR_SAME_ENUM(ODRFileLocationDisk, odr::FileLocation::disk); diff --git a/conanfile.py b/conanfile.py index 99d846c97..a2edcb9a4 100644 --- a/conanfile.py +++ b/conanfile.py @@ -17,6 +17,7 @@ class OpenDocumentCoreConan(ConanFile): options = { "shared": [True, False], "fPIC": [True, False], + # deprecated, see the option in CMakeLists.txt "with_libmagic": [True, False], "with_http_server": [True, False], "with_cli": [True, False], @@ -28,7 +29,7 @@ class OpenDocumentCoreConan(ConanFile): default_options = { "shared": False, "fPIC": True, - "with_libmagic": True, + "with_libmagic": False, "with_http_server": True, "with_cli": True, "with_python": False, diff --git a/docs/design/README.md b/docs/design/README.md index d2908ec66..b5f5598d9 100644 --- a/docs/design/README.md +++ b/docs/design/README.md @@ -95,5 +95,14 @@ ### Open tasks +- remove libmagic. `ODR_WITH_LIBMAGIC` and the conan `with_libmagic` option are + deprecated and off by default: `odr::mimetype` runs the open strategy instead, + which names what is *inside* a zip or a compound file where libmagic could only + answer `application/zip`, and which gets `svm` right where libmagic does not. + What goes with the option: `src/odr/internal/libmagic/`, the CMake block that + stages `magic.mgc`, `GlobalParams::libmagic_database_path` and its java, + python and objc mirrors, `project_info::has_libmagic`, + `pyodr._init_libmagic_database_path`, and the `magic.mgc` lookup in the apple + bootstrap - collect additional pdf files via the translate cli and capture the ones that break - exercise editing across all formats (odp editing appears broken via an HTML issue) diff --git a/jni/java/app/opendocument/core/FileCategory.java b/jni/java/app/opendocument/core/FileCategory.java index 975d9861f..f77654c81 100644 --- a/jni/java/app/opendocument/core/FileCategory.java +++ b/jni/java/app/opendocument/core/FileCategory.java @@ -2,7 +2,7 @@ /** Mirrors {@code odr::FileCategory}; constant order must match the C++ declaration. */ public enum FileCategory { - UNKNOWN, TEXT, IMAGE, ARCHIVE, DOCUMENT, FONT; + UNKNOWN, TEXT, IMAGE, ARCHIVE, DOCUMENT, FONT, AUDIO, VIDEO; static FileCategory fromNative(int code) { return code < 0 ? null : values()[code]; diff --git a/jni/java/app/opendocument/core/FileType.java b/jni/java/app/opendocument/core/FileType.java index eab8ef0cd..4e1c77aff 100644 --- a/jni/java/app/opendocument/core/FileType.java +++ b/jni/java/app/opendocument/core/FileType.java @@ -11,7 +11,11 @@ public enum FileType { COMMA_SEPARATED_VALUES, JAVASCRIPT_OBJECT_NOTATION, MARKDOWN, ZIP, COMPOUND_FILE_BINARY_FORMAT, PORTABLE_NETWORK_GRAPHICS, GRAPHICS_INTERCHANGE_FORMAT, JPEG, BITMAP_IMAGE_FILE, STARVIEW_METAFILE, - TRUETYPE_FONT, OPENTYPE_FONT; + TRUETYPE_FONT, OPENTYPE_FONT, WEBP, TAGGED_IMAGE_FILE_FORMAT, + HIGH_EFFICIENCY_IMAGE_FORMAT, AV1_IMAGE_FILE_FORMAT, MPEG_AUDIO, MPEG4_AUDIO, + OGG_AUDIO, WAVEFORM_AUDIO, FREE_LOSSLESS_AUDIO_CODEC, MPEG4_VIDEO, + QUICKTIME_VIDEO, THIRD_GENERATION_PARTNERSHIP_VIDEO, MATROSKA_VIDEO, + AUDIO_VIDEO_INTERLEAVE; static FileType fromNative(int code) { return code < 0 ? null : values()[code]; diff --git a/jni/java/app/opendocument/core/GlobalParams.java b/jni/java/app/opendocument/core/GlobalParams.java index 614cb5d19..010821329 100644 --- a/jni/java/app/opendocument/core/GlobalParams.java +++ b/jni/java/app/opendocument/core/GlobalParams.java @@ -8,10 +8,19 @@ public final class GlobalParams { public static native String odrCoreDataPath(); + /** + * @deprecated Read only by a native library built with the deprecated {@code + * ODR_WITH_LIBMAGIC}. Detection is our own otherwise and needs no database. + */ + @Deprecated public static native String libmagicDatabasePath(); public static native void setOdrCoreDataPath(String path); + /** + * @deprecated See {@link #libmagicDatabasePath()}. + */ + @Deprecated public static native void setLibmagicDatabasePath(String path); private GlobalParams() {} diff --git a/pyproject.toml b/pyproject.toml index 939660729..1fb014a48 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -61,12 +61,11 @@ ODR_PYTHON = "ON" ODR_CLI = "OFF" ODR_TEST = "OFF" BUILD_SHARED_LIBS = "OFF" -# libmagic's database is just a data file, so it can ship: `ODR_BUNDLE_ASSETS` -# copies `magic.mgc` next to the odr.js assets, `python/CMakeLists.txt` installs -# that directory as `pyodr/data`, and `pyodr/__init__.py` points `GlobalParams` -# at the installed copy. Costs ~0.4 MB of wheel (the database deflates to that -# from 8.5 MB) and buys real MIME detection instead of odr's 12-byte sniffing. -ODR_WITH_LIBMAGIC = "ON" +# libmagic is deprecated and stays off: `mimetype` runs the open strategy now, +# which names what is *inside* a zip or a compound file where libmagic could +# only answer `application/zip`. The wheel no longer carries `magic.mgc` for it +# (~0.4 MB deflated of 8.5 MB), and `pyodr/__init__.py` finding no database is +# the normal case rather than a problem. ODR_BUNDLE_ASSETS = "ON" # Section must exist for setuptools-scm to pick up its pyproject config. diff --git a/python/pyodr/__init__.py b/python/pyodr/__init__.py index 18160528c..b871bb480 100644 --- a/python/pyodr/__init__.py +++ b/python/pyodr/__init__.py @@ -48,9 +48,10 @@ def _init_odr_core_data_path() -> None: def _init_libmagic_database_path() -> None: - # Same resolution order for libmagic's compiled database, which is a file - # rather than a directory. Leaving it unresolved is not fatal: odrcore then - # tries the system database and finally falls back to its own sniffing. + # Deprecated, and a no-op for the wheels we ship: only a core built with + # `ODR_WITH_LIBMAGIC` reads this, and that is off by default now that + # detection is odrcore's own. Same resolution order as above, for a file + # rather than a directory, and finding nothing is the normal case. if _is_configured(_core.GlobalParams.libmagic_database_path()): return env_path = os.environ.get("ODR_LIBMAGIC_DATABASE_PATH") diff --git a/python/src/bind_core.cpp b/python/src/bind_core.cpp index e65b1a9f9..8a711ca65 100644 --- a/python/src/bind_core.cpp +++ b/python/src/bind_core.cpp @@ -25,6 +25,8 @@ void odr_python::bind_core(py::module_ &m) { py::class_(m, "GlobalParams", "Global resource paths of the library.") .def_static("odr_core_data_path", &odr::GlobalParams::odr_core_data_path) + // the two libmagic paths are deprecated: only a core built with the + // deprecated `ODR_WITH_LIBMAGIC` reads them .def_static("libmagic_database_path", &odr::GlobalParams::libmagic_database_path) .def_static("set_odr_core_data_path", diff --git a/python/src/bind_file.cpp b/python/src/bind_file.cpp index c9003af7c..826da09c0 100644 --- a/python/src/bind_file.cpp +++ b/python/src/bind_file.cpp @@ -55,7 +55,25 @@ void odr_python::bind_file(py::module_ &m) { .value("bitmap_image_file", odr::FileType::bitmap_image_file) .value("starview_metafile", odr::FileType::starview_metafile) .value("truetype_font", odr::FileType::truetype_font) - .value("opentype_font", odr::FileType::opentype_font); + .value("opentype_font", odr::FileType::opentype_font) + .value("webp", odr::FileType::webp) + .value("tagged_image_file_format", + odr::FileType::tagged_image_file_format) + .value("high_efficiency_image_format", + odr::FileType::high_efficiency_image_format) + .value("av1_image_file_format", odr::FileType::av1_image_file_format) + .value("mpeg_audio", odr::FileType::mpeg_audio) + .value("mpeg4_audio", odr::FileType::mpeg4_audio) + .value("ogg_audio", odr::FileType::ogg_audio) + .value("waveform_audio", odr::FileType::waveform_audio) + .value("free_lossless_audio_codec", + odr::FileType::free_lossless_audio_codec) + .value("mpeg4_video", odr::FileType::mpeg4_video) + .value("quicktime_video", odr::FileType::quicktime_video) + .value("third_generation_partnership_video", + odr::FileType::third_generation_partnership_video) + .value("matroska_video", odr::FileType::matroska_video) + .value("audio_video_interleave", odr::FileType::audio_video_interleave); py::enum_(m, "FileCategory") .value("unknown", odr::FileCategory::unknown) @@ -63,7 +81,9 @@ void odr_python::bind_file(py::module_ &m) { .value("image", odr::FileCategory::image) .value("archive", odr::FileCategory::archive) .value("document", odr::FileCategory::document) - .value("font", odr::FileCategory::font); + .value("font", odr::FileCategory::font) + .value("audio", odr::FileCategory::audio) + .value("video", odr::FileCategory::video); py::enum_(m, "FileLocation") .value("memory", odr::FileLocation::memory) diff --git a/python/tests/test_meta.py b/python/tests/test_meta.py index 963f0edb2..f6ad88472 100644 --- a/python/tests/test_meta.py +++ b/python/tests/test_meta.py @@ -1,16 +1,6 @@ -import os - -import pytest - import pyodr -def _libmagic_database() -> str: - """The resolved libmagic database, or "" when this build has none.""" - path = pyodr.GlobalParams.libmagic_database_path() - return path if path and os.path.isfile(path) else "" - - def test_version(): # A dev build carries no project version; only assert consistency. assert isinstance(pyodr.version(), str) @@ -133,11 +123,8 @@ def test_global_params(): assert isinstance(pyodr.GlobalParams.libmagic_database_path(), str) -@pytest.mark.skipif( - not _libmagic_database(), - reason="built without libmagic, or its database was not bundled", -) -def test_mimetype_uses_libmagic(odt_path): - # odr's own sniffing only reaches the ZIP container ("application/zip"); - # recognising the ODF mimetype entry stored inside it is what libmagic adds. +def test_mimetype_names_what_is_inside_the_container(odt_path): + # An ODF file is a ZIP, and the answer worth having is the one from inside + # it. Detection opens the container to get there, so this holds with or + # without the deprecated libmagic. assert pyodr.mimetype(str(odt_path)) == "application/vnd.oasis.opendocument.text" diff --git a/src/odr/file.cpp b/src/odr/file.cpp index 96e78e7d0..085822ef1 100644 --- a/src/odr/file.cpp +++ b/src/odr/file.cpp @@ -58,8 +58,8 @@ std::vector DecodedFile::list_file_types(const std::string &path, } std::string_view DecodedFile::mimetype(const std::string &path, - [[maybe_unused]] const Logger &logger) { - return internal::magic::mimetype(path); + const Logger &logger) { + return internal::magic::mimetype(path, logger); } DecodedFile::DecodedFile(std::shared_ptr impl) diff --git a/src/odr/file.hpp b/src/odr/file.hpp index 9a983b30a..d370ab731 100644 --- a/src/odr/file.hpp +++ b/src/odr/file.hpp @@ -89,6 +89,42 @@ enum class FileType { truetype_font, // https://en.wikipedia.org/wiki/OpenType opentype_font, + + // Detection only, like `word_perfect` above: the media formats a viewer is + // regularly handed alongside documents. Naming them is the point - a caller + // that knows a file is a video hands it to a player instead of asking us to + // translate it, and without a name the text fallback would call it plain + // text. New entries go at the end: the bindings mirror this enum by ordinal. + // https://en.wikipedia.org/wiki/WebP + webp, + // https://en.wikipedia.org/wiki/TIFF + tagged_image_file_format, + // https://en.wikipedia.org/wiki/High_Efficiency_Image_File_Format + high_efficiency_image_format, + // https://en.wikipedia.org/wiki/AVIF + av1_image_file_format, + + // https://en.wikipedia.org/wiki/MP3 + mpeg_audio, + // https://en.wikipedia.org/wiki/MP4_file_format + mpeg4_audio, + // https://en.wikipedia.org/wiki/Ogg + ogg_audio, + // https://en.wikipedia.org/wiki/WAV + waveform_audio, + // https://en.wikipedia.org/wiki/FLAC + free_lossless_audio_codec, + + // https://en.wikipedia.org/wiki/MP4_file_format + mpeg4_video, + // https://en.wikipedia.org/wiki/QuickTime_File_Format + quicktime_video, + // https://en.wikipedia.org/wiki/3GP_and_3G2 + third_generation_partnership_video, + // https://en.wikipedia.org/wiki/Matroska + matroska_video, + // https://en.wikipedia.org/wiki/Audio_Video_Interleave + audio_video_interleave, }; /// @brief Collection of file categories. @@ -99,6 +135,9 @@ enum class FileCategory { archive, document, font, + // appended rather than sorted in, for the same reason as `FileType` + audio, + video, }; /// @brief Collection of file locations. diff --git a/src/odr/global_params.hpp b/src/odr/global_params.hpp index c53214182..3f9b1fb8e 100644 --- a/src/odr/global_params.hpp +++ b/src/odr/global_params.hpp @@ -7,9 +7,12 @@ namespace odr { class GlobalParams final { public: static const std::string &odr_core_data_path(); + /// @deprecated Read only by a build with the deprecated `ODR_WITH_LIBMAGIC`. + /// Detection is our own otherwise, and looks at nothing outside the file. static const std::string &libmagic_database_path(); static void set_odr_core_data_path(const std::string &path); + /// @deprecated See @ref libmagic_database_path. static void set_libmagic_database_path(const std::string &path); private: diff --git a/src/odr/internal/file_type_table.cpp b/src/odr/internal/file_type_table.cpp index 549ea8d50..1a5716e45 100644 --- a/src/odr/internal/file_type_table.cpp +++ b/src/odr/internal/file_type_table.cpp @@ -162,6 +162,59 @@ constexpr std::array otf_mimetypes{"font/otf"sv, "application/x-font-otf"sv, "application/x-font-opentype"sv, "application/vnd.ms-opentype"sv}; +constexpr std::array webp_extensions{"webp"sv}; +constexpr std::array webp_mimetypes{"image/webp"sv}; + +constexpr std::array tiff_extensions{"tif"sv, "tiff"sv}; +constexpr std::array tiff_mimetypes{"image/tiff"sv, "image/x-tiff"sv}; + +constexpr std::array heif_extensions{"heic"sv, "heif"sv, "hif"sv}; +constexpr std::array heif_mimetypes{"image/heic"sv, "image/heif"sv, + "image/heic-sequence"sv, + "image/heif-sequence"sv}; + +constexpr std::array avif_extensions{"avif"sv, "avifs"sv}; +constexpr std::array avif_mimetypes{"image/avif"sv, "image/avif-sequence"sv}; + +constexpr std::array mp3_extensions{"mp3"sv}; +constexpr std::array mp3_mimetypes{"audio/mpeg"sv, "audio/mp3"sv, + "audio/x-mpeg"sv}; + +constexpr std::array m4a_extensions{"m4a"sv, "m4b"sv}; +constexpr std::array m4a_mimetypes{"audio/mp4"sv, "audio/x-m4a"sv, + "audio/m4a"sv}; + +constexpr std::array ogg_extensions{"ogg"sv, "oga"sv, "opus"sv}; +constexpr std::array ogg_mimetypes{"audio/ogg"sv, "application/ogg"sv, + "audio/opus"sv}; + +constexpr std::array wav_extensions{"wav"sv, "wave"sv}; +constexpr std::array wav_mimetypes{"audio/wav"sv, "audio/x-wav"sv, + "audio/wave"sv, "audio/vnd.wave"sv}; + +constexpr std::array flac_extensions{"flac"sv}; +constexpr std::array flac_mimetypes{"audio/flac"sv, "audio/x-flac"sv}; + +constexpr std::array mp4_extensions{"mp4"sv, "m4v"sv}; +constexpr std::array mp4_mimetypes{"video/mp4"sv, "video/x-m4v"sv}; + +constexpr std::array mov_extensions{"mov"sv, "qt"sv}; +constexpr std::array mov_mimetypes{"video/quicktime"sv}; + +constexpr std::array third_gpp_extensions{"3gp"sv, "3g2"sv}; +constexpr std::array third_gpp_mimetypes{"video/3gpp"sv, "video/3gpp2"sv, + "audio/3gpp"sv}; + +// one type for the whole EBML container: matroska and webm are the same bytes +// until the DocType element, which sits deeper than a signature reaches +constexpr std::array mkv_extensions{"mkv"sv, "webm"sv, "mka"sv}; +constexpr std::array mkv_mimetypes{"video/x-matroska"sv, "video/webm"sv, + "audio/x-matroska"sv, "audio/webm"sv}; + +constexpr std::array avi_extensions{"avi"sv}; +constexpr std::array avi_mimetypes{"video/x-msvideo"sv, "video/avi"sv, + "video/msvideo"sv}; + // The single source of truth behind every public format lookup. `odr_test` // asserts that it covers each `FileType` exactly once and that the capability // bits agree with what the engines actually do. @@ -420,6 +473,109 @@ constexpr std::array table{ FileCategory::font, DocumentType::unknown, {.detect_by_content = true, .open = true, .translate_html = true}}, + + // Named but not decoded, so `detect_by_content` is the only capability + // they carry - see the comment on these in `FileType`. + Row{FileType::webp, + "webp"sv, + webp_extensions, + webp_mimetypes, + FileCategory::image, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::tagged_image_file_format, + "tiff"sv, + tiff_extensions, + tiff_mimetypes, + FileCategory::image, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::high_efficiency_image_format, + "heif"sv, + heif_extensions, + heif_mimetypes, + FileCategory::image, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::av1_image_file_format, + "avif"sv, + avif_extensions, + avif_mimetypes, + FileCategory::image, + DocumentType::unknown, + {.detect_by_content = true}}, + + Row{FileType::mpeg_audio, + "mp3"sv, + mp3_extensions, + mp3_mimetypes, + FileCategory::audio, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::mpeg4_audio, + "m4a"sv, + m4a_extensions, + m4a_mimetypes, + FileCategory::audio, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::ogg_audio, + "ogg"sv, + ogg_extensions, + ogg_mimetypes, + FileCategory::audio, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::waveform_audio, + "wav"sv, + wav_extensions, + wav_mimetypes, + FileCategory::audio, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::free_lossless_audio_codec, + "flac"sv, + flac_extensions, + flac_mimetypes, + FileCategory::audio, + DocumentType::unknown, + {.detect_by_content = true}}, + + Row{FileType::mpeg4_video, + "mp4"sv, + mp4_extensions, + mp4_mimetypes, + FileCategory::video, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::quicktime_video, + "mov"sv, + mov_extensions, + mov_mimetypes, + FileCategory::video, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::third_generation_partnership_video, + "3gp"sv, + third_gpp_extensions, + third_gpp_mimetypes, + FileCategory::video, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::matroska_video, + "mkv"sv, + mkv_extensions, + mkv_mimetypes, + FileCategory::video, + DocumentType::unknown, + {.detect_by_content = true}}, + Row{FileType::audio_video_interleave, + "avi"sv, + avi_extensions, + avi_mimetypes, + FileCategory::video, + DocumentType::unknown, + {.detect_by_content = true}}, }; /// Finds the row whose list, selected by @p list, contains @p needle. diff --git a/src/odr/internal/magic.cpp b/src/odr/internal/magic.cpp index 1bf1ba071..5595bfe44 100644 --- a/src/odr/internal/magic.cpp +++ b/src/odr/internal/magic.cpp @@ -1,12 +1,20 @@ #include +#include #include #include #include +#include #include +#include #include +#include +#include +#include +#include + namespace odr::internal { namespace { @@ -28,6 +36,54 @@ bool match_magic(const std::string &head, const std::string &pattern) { return true; } +/// The four byte tag at @p offset, or empty if the head stops short of it. +std::string_view tag_at(const std::string &head, const std::size_t offset) { + return head.size() < offset + 4 ? std::string_view{} + : std::string_view(head).substr(offset, 4); +} + +/// Which format a `RIFF` container holds, from its form tag. +FileType riff_file_type(const std::string &head) { + const std::string_view form = tag_at(head, 8); + if (form == "WEBP") { + return FileType::webp; + } + if (form == "WAVE") { + return FileType::waveform_audio; + } + if (form == "AVI ") { + return FileType::audio_video_interleave; + } + return FileType::unknown; +} + +/// Which format an ISO base media container holds, from its major brand. +/// +/// The brands are open ended and every writer adds its own, so this names the +/// ones that mean something other than video and lets the rest - `isom`, +/// `mp41`, `mp42`, `avc1`, `dash` and whatever comes next - fall through to +/// what the container almost always is. +FileType iso_base_media_file_type(const std::string &head) { + const std::string_view brand = tag_at(head, 8); + if (brand == "qt ") { + return FileType::quicktime_video; + } + if (brand.starts_with("3g")) { + return FileType::third_generation_partnership_video; + } + if (brand == "avif" || brand == "avis") { + return FileType::av1_image_file_format; + } + if (brand.starts_with("hei") || brand.starts_with("hev") || brand == "mif1" || + brand == "msf1") { + return FileType::high_efficiency_image_format; + } + if (brand.starts_with("M4A") || brand.starts_with("M4B")) { + return FileType::mpeg4_audio; + } + return FileType::mpeg4_video; +} + } // namespace FileType magic::file_type(const std::string &magic) { @@ -74,16 +130,52 @@ FileType magic::file_type(const std::string &magic) { match_magic(magic, "74 74 63 66")) { // 'ttcf' — TrueType Collection return FileType::truetype_font; } + + // the media formats below are named, not decoded: see `FileType` + if (match_magic(magic, "52 49 46 46")) { // 'RIFF' + if (const FileType file_type = riff_file_type(magic); + file_type != FileType::unknown) { + return file_type; + } + } + if (match_magic(magic, "?? ?? ?? ?? 66 74 79 70")) { // 'ftyp' box + return iso_base_media_file_type(magic); + } + if (match_magic(magic, "49 49 2A 00") || // little endian + match_magic(magic, "4D 4D 00 2A")) { // big endian + return FileType::tagged_image_file_format; + } + if (match_magic(magic, "1A 45 DF A3")) { // EBML + return FileType::matroska_video; + } + if (match_magic(magic, "4F 67 67 53")) { // 'OggS' + return FileType::ogg_audio; + } + if (match_magic(magic, "66 4C 61 43")) { // 'fLaC' + return FileType::free_lossless_audio_codec; + } + if (match_magic(magic, "49 44 33") || // 'ID3' tag + match_magic(magic, "FF FB") || // frame sync, no crc + match_magic(magic, "FF F3") || // + match_magic(magic, "FF F2") || // + match_magic(magic, "FF FA")) { // frame sync, with crc + return FileType::mpeg_audio; + } + return FileType::unknown; } FileType magic::file_type(std::istream &in) { - static constexpr auto max_head_size = 12; + static constexpr std::size_t max_head_size = 12; - char head[max_head_size]; - in.read(head, sizeof(head)); + // value initialized, and cut back to what was actually read: a file shorter + // than the longest signature would otherwise be matched against whatever the + // stack held behind it + std::array head{}; + in.read(head.data(), head.size()); - return file_type(std::string(head, max_head_size)); + return file_type( + std::string(head.data(), static_cast(in.gcount()))); } FileType magic::file_type(const abstract::File &file) { @@ -94,11 +186,25 @@ FileType magic::file_type(const File &file) { return file_type(*file.stream()); } -std::string_view magic::mimetype(const std::string &path) { +std::string_view magic::mimetype(const std::string &path, + [[maybe_unused]] const Logger &logger) { #ifdef ODR_WITH_LIBMAGIC + // deprecated, and no longer what a default build does - see the option in + // CMakeLists.txt. it cannot see inside a zip or a compound file binary, so + // it answers `application/zip` where the branch below names the document return libmagic::mimetype(path); #else - return odr::mimetype_by_file_type(magic::file_type(path)); + // the signature table above is not enough on its own: a zip and a compound + // file binary say nothing about which document they hold, and the answer for + // those only comes out of opening them. that is what `list_file_types` does, + // and it reports the container first and what was decoded from it after + const std::vector file_types = + open_strategy::list_file_types(std::make_shared(path), logger); + if (file_types.empty()) { + throw UnknownFileType(); + } + + return odr::mimetype_by_file_type(file_types.back()); #endif } diff --git a/src/odr/internal/magic.hpp b/src/odr/internal/magic.hpp index 85fb4e8e4..cf780e3e9 100644 --- a/src/odr/internal/magic.hpp +++ b/src/odr/internal/magic.hpp @@ -6,6 +6,7 @@ namespace odr { enum class FileType; class File; +class Logger; } // namespace odr namespace odr::internal::abstract { @@ -18,5 +19,7 @@ FileType file_type(std::istream &in); FileType file_type(const abstract::File &file); FileType file_type(const File &file); -std::string_view mimetype(const std::string &path); +/// Without libmagic this opens the file to tell what a container holds, so it +/// needs a logger like the rest of the open strategy. +std::string_view mimetype(const std::string &path, const Logger &logger); } // namespace odr::internal::magic diff --git a/src/odr/odr.cpp b/src/odr/odr.cpp index dbb250af7..915ecb768 100644 --- a/src/odr/odr.cpp +++ b/src/odr/odr.cpp @@ -90,6 +90,10 @@ std::string odr::file_category_to_string(const FileCategory type) { return "text"; case FileCategory::font: return "font"; + case FileCategory::audio: + return "audio"; + case FileCategory::video: + return "video"; default: return "unnamed"; } diff --git a/test/src/internal/magic_test.cpp b/test/src/internal/magic_test.cpp index 00607b67c..9d07795f3 100644 --- a/test/src/internal/magic_test.cpp +++ b/test/src/internal/magic_test.cpp @@ -1,4 +1,5 @@ #include +#include #include #include @@ -7,18 +8,24 @@ #include +#include + using namespace odr; using namespace odr::internal; using namespace odr::test; +// the mimetype comes from the open strategy, and from libmagic only in a build +// that still asks for the deprecated option. both answers are asserted, because +// the two agree on everything but `svm` - and because the open strategy branch +// used to be unreachable in every job that runs the tests, and stayed broken +// for it + TEST(magic, odt) { const File file(TestData::test_file_path("odr-public/odt/about.odt")); EXPECT_EQ(magic::file_type(*file.impl()), FileType::zip); - if (project_info::has_libmagic()) { - EXPECT_EQ(magic::mimetype(file.disk_path().value()), - "application/vnd.oasis.opendocument.text"); - } + EXPECT_EQ(magic::mimetype(file.disk_path().value(), Logger::null()), + "application/vnd.oasis.opendocument.text"); } TEST(magic, doc) { @@ -26,18 +33,22 @@ TEST(magic, doc) { EXPECT_EQ(magic::file_type(*file.impl()), FileType::compound_file_binary_format); - if (project_info::has_libmagic()) { - EXPECT_EQ(magic::mimetype(file.disk_path().value()), "application/msword"); - } + EXPECT_EQ(magic::mimetype(file.disk_path().value(), Logger::null()), + "application/msword"); } TEST(magic, svm) { const File file(TestData::test_file_path("odr-public/svm/chart-1.svm")); EXPECT_EQ(magic::file_type(*file.impl()), FileType::starview_metafile); + // the one format the two disagree on, and the one where our own table is + // right if (project_info::has_libmagic()) { - EXPECT_EQ(magic::mimetype(file.disk_path().value()), + EXPECT_EQ(magic::mimetype(file.disk_path().value(), Logger::null()), "application/octet-stream"); + } else { + EXPECT_EQ(magic::mimetype(file.disk_path().value(), Logger::null()), + "application/x-starview-metafile"); } } @@ -45,9 +56,8 @@ TEST(magic, odf) { const File file(TestData::test_file_path("odr-private/pdf/sample.pdf")); EXPECT_EQ(magic::file_type(*file.impl()), FileType::portable_document_format); - if (project_info::has_libmagic()) { - EXPECT_EQ(magic::mimetype(file.disk_path().value()), "application/pdf"); - } + EXPECT_EQ(magic::mimetype(file.disk_path().value(), Logger::null()), + "application/pdf"); } TEST(magic, wpd) { @@ -55,8 +65,89 @@ TEST(magic, wpd) { TestData::test_file_path("odr-public/wpd/Sync3 Sample Page.wpd")); EXPECT_EQ(magic::file_type(*file.impl()), FileType::word_perfect); - if (project_info::has_libmagic()) { - EXPECT_EQ(magic::mimetype(file.disk_path().value()), - "application/vnd.wordperfect"); - } + EXPECT_EQ(magic::mimetype(file.disk_path().value(), Logger::null()), + "application/vnd.wordperfect"); +} + +namespace { + +FileType detect(const std::string &head) { + std::istringstream in(head); + return magic::file_type(in); +} + +} // namespace + +// The media formats are named from their head alone and never opened, so a +// synthetic signature is the whole input the detection ever sees. +TEST(magic, riff_container) { + using namespace std::string_literals; + + EXPECT_EQ(detect("RIFF\x24\x00\x00\x00WEBP"s), FileType::webp); + EXPECT_EQ(detect("RIFF\x24\x00\x00\x00WAVE"s), FileType::waveform_audio); + EXPECT_EQ(detect("RIFF\x24\x00\x00\x00" + "AVI "s), + FileType::audio_video_interleave); + // a RIFF holding something we do not name is still not a text file + EXPECT_EQ(detect("RIFF\x24\x00\x00\x00RMID"s), FileType::unknown); +} + +TEST(magic, iso_base_media_container) { + using namespace std::string_literals; + + EXPECT_EQ(detect("\x00\x00\x00\x18" + "ftypisom"s), + FileType::mpeg4_video); + EXPECT_EQ(detect("\x00\x00\x00\x18" + "ftypmp42"s), + FileType::mpeg4_video); + EXPECT_EQ(detect("\x00\x00\x00\x14" + "ftypqt "s), + FileType::quicktime_video); + EXPECT_EQ(detect("\x00\x00\x00\x18" + "ftyp3gp4"s), + FileType::third_generation_partnership_video); + EXPECT_EQ(detect("\x00\x00\x00\x18" + "ftypheic"s), + FileType::high_efficiency_image_format); + EXPECT_EQ(detect("\x00\x00\x00\x18" + "ftypmif1"s), + FileType::high_efficiency_image_format); + EXPECT_EQ(detect("\x00\x00\x00\x18" + "ftypavif"s), + FileType::av1_image_file_format); + EXPECT_EQ(detect("\x00\x00\x00\x18" + "ftypM4A "s), + FileType::mpeg4_audio); + // an unknown brand is a video, which is what the container nearly always is + EXPECT_EQ(detect("\x00\x00\x00\x18" + "ftypxxxx"s), + FileType::mpeg4_video); +} + +TEST(magic, standalone_media_signatures) { + using namespace std::string_literals; + + EXPECT_EQ(detect("II*\x00"s), FileType::tagged_image_file_format); + EXPECT_EQ(detect("MM\x00*"s), FileType::tagged_image_file_format); + EXPECT_EQ(detect("\x1a\x45\xdf\xa3"s), FileType::matroska_video); + EXPECT_EQ(detect("OggS"s), FileType::ogg_audio); + EXPECT_EQ(detect("fLaC"s), FileType::free_lossless_audio_codec); + EXPECT_EQ(detect("ID3\x04"s), FileType::mpeg_audio); + EXPECT_EQ(detect("\xff\xfb\x90\x00"s), FileType::mpeg_audio); + // still a jpeg, not an mp3 frame sync + EXPECT_EQ(detect("\xff\xd8\xff\xdb"s), FileType::jpeg); +} + +TEST(magic, short_stream) { + // a file shorter than the head buffer may only be matched against what was + // actually read from it, never against what happened to sit behind it + std::istringstream empty(""); + EXPECT_EQ(magic::file_type(empty), FileType::unknown); + + std::istringstream one_byte("P"); + EXPECT_EQ(magic::file_type(one_byte), FileType::unknown); + + std::istringstream two_bytes("BM"); + EXPECT_EQ(magic::file_type(two_bytes), FileType::bitmap_image_file); } diff --git a/test/src/odr_test.cpp b/test/src/odr_test.cpp index ede7c43b7..6644ff7b9 100644 --- a/test/src/odr_test.cpp +++ b/test/src/odr_test.cpp @@ -25,7 +25,7 @@ namespace { std::vector every_file_type() { std::vector result; for (auto i = static_cast(FileType::unknown); - i <= static_cast(FileType::opentype_font); ++i) { + i <= static_cast(FileType::audio_video_interleave); ++i) { result.push_back(static_cast(i)); } return result; From ca01b155facc2b4bee43646fb1dc0b65fdc1d606 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 1 Aug 2026 17:21:47 +0200 Subject: [PATCH 2/3] fix: stop the android and apple packagings demanding magic.mgc Turning `ODR_WITH_LIBMAGIC` off by default took the database out of the staged data, and two packagings still treated it as mandatory: android's `build_native.py` copied it unconditionally, which is what fails the `native` job on all four ABIs, and the apple `assert_contents` listed it among the resources a framework must carry. Both now do what `apple/CMakeLists.txt` already does and take whatever the root build staged, so a deprecated libmagic build still packages the same way. `OdrAndroid` likewise only points `GlobalParams` at a database that is there, and the instrumented test stops asserting one is. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01C8zVbqCiCedPBvigwpt5ta --- .github/workflows/python.yml | 5 ++--- android/AGENTS.md | 6 ++--- android/README.md | 1 - android/build.gradle.kts | 8 +++---- android/build_native.py | 10 ++++++--- .../app/opendocument/core/OdrAndroidTest.kt | 9 +++----- .../opendocument/core/android/OdrAndroid.kt | 22 +++++++++++++++---- apple/AGENTS.md | 16 +++++++------- apple/build_xcframework.py | 1 - apple/src/ODRGlobalParams.mm | 4 ++-- docs/design/README.md | 5 +++-- python/README.md | 21 +++++++++--------- 12 files changed, 60 insertions(+), 48 deletions(-) diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml index 260d81839..b73330658 100644 --- a/.github/workflows/python.yml +++ b/.github/workflows/python.yml @@ -109,9 +109,8 @@ jobs: restore-keys: | ccache-${{ env.CACHE_FLAVOR }}-${{ matrix.host_profile }}-${{ env.CCACHE_KEY_SUFFIX }}- - # Match the dependency set that pyproject.toml enables. libmagic stays on - # and `bundle_assets` bridges its database path into the build so - # `magic.mgc` lands in `pyodr/data`. + # Match the dependency set that pyproject.toml enables. `bundle_assets` + # is what stages the renderer's css/js so they land in `pyodr/data`. - name: conan install run: > conan install . diff --git a/android/AGENTS.md b/android/AGENTS.md index 71807c385..369707888 100644 --- a/android/AGENTS.md +++ b/android/AGENTS.md @@ -66,6 +66,6 @@ calls it. finds the incompatibilities first. - The manifest stays permission-free; permissions the server needs are the app's call, and documented in `README.md` instead. -- Assets keep the `core/odrcore` + `core/libmagic` layout of the droid conan - deployer, so an app can move between the two packagings without touching its - extraction code. +- Assets keep the `core/odrcore` (+ `core/libmagic`, in a deprecated libmagic + build) layout of the droid conan deployer, so an app can move between the two + packagings without touching its extraction code. diff --git a/android/README.md b/android/README.md index a1fca43a4..001de3525 100644 --- a/android/README.md +++ b/android/README.md @@ -9,7 +9,6 @@ odr-core-android.aar ├── jni//libodr_jni.so the bindings with the core linked in ├── jni//libc++_shared.so the c++ runtime they were built against ├── assets/core/odrcore/* css/js of the html renderer -├── assets/core/libmagic/magic.mgc libmagic database └── proguard.txt keeps the classes JNI resolves by name ``` diff --git a/android/build.gradle.kts b/android/build.gradle.kts index 18452d11b..24c8f2905 100644 --- a/android/build.gradle.kts +++ b/android/build.gradle.kts @@ -104,10 +104,10 @@ android { targetCompatibility = JavaVersion.VERSION_17 } - // the instrumented apk carries the bindings and the 8 MB libmagic database, - // and pushing that onto a cold emulator outlasts ddmlib's default timeout, - // which surfaces as a ShellCommandUnresponsiveException rather than as a - // failing test + // the instrumented apk carries the bindings and the renderer assets, and + // pushing that onto a cold emulator outlasts ddmlib's default timeout, which + // surfaces as a ShellCommandUnresponsiveException rather than as a failing + // test installation { timeOutInMs = 10 * 60 * 1000 } sourceSets { diff --git a/android/build_native.py b/android/build_native.py index 79a55d426..43c5b135b 100644 --- a/android/build_native.py +++ b/android/build_native.py @@ -12,7 +12,7 @@ prebuilt/jniLibs//libodr_jni.so the bindings, core linked in prebuilt/jniLibs//libc++_shared.so from the NDK, see below prebuilt/assets/core/odrcore/* css/js of the html renderer - prebuilt/assets/core/libmagic/magic.mgc libmagic database + prebuilt/assets/core/libmagic/magic.mgc only in a deprecated libmagic build `libc++_shared.so` has to be shipped because the android profiles build against the shared c++ runtime and nothing else in a consuming app pulls it in; the @@ -117,8 +117,12 @@ def build(architecture: str, conan: str, build_profile: str, output: Path) -> No assets = output / "assets" / "core" shutil.rmtree(assets, ignore_errors=True) shutil.copytree(data, assets / "odrcore", ignore=shutil.ignore_patterns("magic.mgc")) - (assets / "libmagic").mkdir(parents=True, exist_ok=True) - shutil.copy2(data / "magic.mgc", assets / "libmagic" / "magic.mgc") + # only staged by a build that still asks for the deprecated libmagic, which + # this one does not; kept so such a build still packages the same way + magic = data / "magic.mgc" + if magic.is_file(): + (assets / "libmagic").mkdir(parents=True, exist_ok=True) + shutil.copy2(magic, assets / "libmagic" / "magic.mgc") def main() -> int: diff --git a/android/src/androidTest/java/app/opendocument/core/OdrAndroidTest.kt b/android/src/androidTest/java/app/opendocument/core/OdrAndroidTest.kt index 585c8f081..f24c7f302 100644 --- a/android/src/androidTest/java/app/opendocument/core/OdrAndroidTest.kt +++ b/android/src/androidTest/java/app/opendocument/core/OdrAndroidTest.kt @@ -33,9 +33,6 @@ class OdrAndroidTest { assertTrue("$data is not a directory", data.isDirectory) assertTrue(File(data, "document.css").isFile) assertTrue(File(data, "document.js").isFile) - - val magic = File(GlobalParams.libmagicDatabasePath()) - assertTrue("$magic is not a file", magic.isFile) } @Test @@ -46,12 +43,12 @@ class OdrAndroidTest { } @Test - fun detectsTypeWithTheBundledMagicDatabase() { + fun detectsTypeInsideTheContainer() { val directory = TestSupport.tempDir("magic") val odt = TestFiles.odtFile(directory) - // goes through libmagic, so it only works when the database extracted above - // is the one the native library actually opened + // naming the odt rather than the zip holding it means detection opened the + // container, which is the whole reason this no longer goes through libmagic assertEquals("application/vnd.oasis.opendocument.text", Odr.mimetype(odt.toString())) } } diff --git a/android/src/main/java/app/opendocument/core/android/OdrAndroid.kt b/android/src/main/java/app/opendocument/core/android/OdrAndroid.kt index f5cdca69c..405f25287 100644 --- a/android/src/main/java/app/opendocument/core/android/OdrAndroid.kt +++ b/android/src/main/java/app/opendocument/core/android/OdrAndroid.kt @@ -9,8 +9,8 @@ import java.io.FileOutputStream import java.io.IOException /** - * Makes the library usable on android: the renderer reads its CSS/JS and the libmagic database as - * plain files, and an APK holds them as assets, which are not files. + * Makes the library usable on android: the renderer reads its CSS/JS as plain files, and an APK + * holds them as assets, which are not files. * * Call [init] once before anything else touches the library: * ``` @@ -22,7 +22,9 @@ object OdrAndroid { /** Asset directory this AAR ships its runtime data under. */ private const val ASSETS = "core" - /** The libmagic database alone is 8 MB, so the default 8 KB would be a lot of syscalls. */ + /** + * A deprecated libmagic build adds an 8 MB database, so the default 8 KB is a lot of syscalls. + */ private const val BUFFER_SIZE = 64 * 1024 private var initialized = false @@ -56,10 +58,22 @@ object OdrAndroid { } GlobalParams.setOdrCoreDataPath(File(root, "odrcore").absolutePath) - GlobalParams.setLibmagicDatabasePath(File(root, "libmagic/magic.mgc").absolutePath) + pointAtLibmagicDatabase(root) initialized = true } + /** + * Only an AAR whose native library was built against the deprecated libmagic carries the + * database, and only such a library reads the path — finding nothing is the normal case. + */ + @Suppress("DEPRECATION") + private fun pointAtLibmagicDatabase(root: File) { + val database = File(root, "libmagic/magic.mgc") + if (database.isFile) { + GlobalParams.setLibmagicDatabasePath(database.absolutePath) + } + } + /** An asset directory lists its children; an asset file lists nothing. */ private fun extract(assets: AssetManager, path: String, target: File) { val children = assets.list(path) diff --git a/apple/AGENTS.md b/apple/AGENTS.md index b83409372..be11a4c53 100644 --- a/apple/AGENTS.md +++ b/apple/AGENTS.md @@ -34,17 +34,17 @@ it — a simulator binary mistagged `IOS` is the classic | `macos-arm64_x86_64` | `apple-macos-armv8` + `apple-macos-x86_64` | `assemble` also fails the build if a framework is missing its headers, module -map, `magic.mgc` or the plist's platform keys — the analogue of +map, renderer resources or the plist's platform keys — the analogue of `android/build.gradle.kts`'s `checkNative`, and for the same reason: those all publish happily and then fail at the consumer. ## Why a dynamic framework -`+load` in `src/OdrCoreBootstrap.mm` is what points odrcore at the css/js and -`magic.mgc` in this bundle, so an app never has to. In a **static** framework -nothing references that translation unit, the linker drops it, and the -bootstrap never runs — and a SwiftPM binary target gives the consumer no way to -pass `-ObjC`/`-force_load` to get it back. Two lesser reasons: `.binaryTarget` +`+load` in `src/OdrCoreBootstrap.mm` is what points odrcore at the css/js in +this bundle, so an app never has to. In a **static** framework nothing +references that translation unit, the linker drops it, and the bootstrap never +runs — and a SwiftPM binary target gives the consumer no way to pass +`-ObjC`/`-force_load` to get it back. Two lesser reasons: `.binaryTarget` has no `resources:`, so the assets have to live in the bundle; and an undefined symbol becomes a link error here instead of a crash at the consumer. @@ -130,8 +130,8 @@ which would leave the tag serving the previous version's binary. The iOS *device* slice is only ever link-checked — nothing runs it. The simulator suite is the analogue of android's instrumented job and the only place that sees what a device sees: that `+load` fired, that `NSBundle` found -`magic.mgc`, that `temp_directory_path()` is writable inside an app container. -A new binding is only covered once something in `tests/` calls it. +the renderer assets, that `temp_directory_path()` is writable inside an app +container. A new binding is only covered once something in `tests/` calls it. `tests/Fixtures/mixed-layout.odt` is 9 KB of `odt/` from OpenDocument.test, carried here because `test/data/` is fetched by `cmake/test_data.cmake` and a diff --git a/apple/build_xcframework.py b/apple/build_xcframework.py index 5069055c0..e1a18d601 100755 --- a/apple/build_xcframework.py +++ b/apple/build_xcframework.py @@ -158,7 +158,6 @@ def assert_contents(framework: Path) -> None: required = [ root / "Headers" / f"{FRAMEWORK}.h", root / "Modules" / "module.modulemap", - resources / "magic.mgc", resources / "document.css", ] missing = [path for path in required if not path.exists()] diff --git a/apple/src/ODRGlobalParams.mm b/apple/src/ODRGlobalParams.mm index db59af83f..abc114ab4 100644 --- a/apple/src/ODRGlobalParams.mm +++ b/apple/src/ODRGlobalParams.mm @@ -47,8 +47,8 @@ + (void)bootstrapFromFrameworkBundle { ODRGlobalParams.odrCoreDataPath = resources; } - // Only the path is recorded here; `magic_load` stays lazy, so the 8 MB - // database is not read until something asks for a mimetype. + // Only a framework built against the deprecated libmagic carries the + // database, so normally there is nothing to point at. NSString *const magic = [bundle pathForResource:@"magic" ofType:@"mgc"]; if (magic != nil) { ODRGlobalParams.libmagicDatabasePath = magic; diff --git a/docs/design/README.md b/docs/design/README.md index b5f5598d9..c9ea9d281 100644 --- a/docs/design/README.md +++ b/docs/design/README.md @@ -102,7 +102,8 @@ What goes with the option: `src/odr/internal/libmagic/`, the CMake block that stages `magic.mgc`, `GlobalParams::libmagic_database_path` and its java, python and objc mirrors, `project_info::has_libmagic`, - `pyodr._init_libmagic_database_path`, and the `magic.mgc` lookup in the apple - bootstrap + `pyodr._init_libmagic_database_path`, and the `magic.mgc` lookups that stay + behind for it — the apple bootstrap, `android/build_native.py` and + `OdrAndroid.pointAtLibmagicDatabase` - collect additional pdf files via the translate cli and capture the ones that break - exercise editing across all formats (odp editing appears broken via an HTML issue) diff --git a/python/README.md b/python/README.md index e588a8cb6..a651882ab 100644 --- a/python/README.md +++ b/python/README.md @@ -42,14 +42,13 @@ CMAKE_ARGS="-DCMAKE_TOOLCHAIN_FILE=$PWD/conan_toolchain.cmake" pip install . ## Runtime data -Rendering uses shipped assets (CSS/JS), and MIME detection uses libmagic's -compiled database (`magic.mgc`). Wheels bundle both under `pyodr/data` and pick -them up automatically. For in-tree builds set `ODR_CORE_DATA_PATH` (the tests -read it) and, if needed, `ODR_LIBMAGIC_DATABASE_PATH`, or call -`pyodr.GlobalParams.set_odr_core_data_path(...)` / -`set_libmagic_database_path(...)`. - -Neither is fatal when missing: without the assets, rendering fails on the -individual resource; without the database, odrcore tries the system database and -then falls back to its own magic sniffing (which sees an `.odt` as -`application/zip` rather than the ODF type). +Rendering uses shipped assets (CSS/JS). Wheels bundle them under `pyodr/data` +and pick them up automatically; for in-tree builds set `ODR_CORE_DATA_PATH` (the +tests read it) or call `pyodr.GlobalParams.set_odr_core_data_path(...)`. Missing +assets are not fatal — rendering then fails on the individual resource. + +MIME detection needs no runtime data: `mimetype` runs the open strategy, so it +names what is *inside* a zip or a compound file. `ODR_LIBMAGIC_DATABASE_PATH` +and `set_libmagic_database_path(...)` are read only by a core built with the +deprecated `ODR_WITH_LIBMAGIC`, which the wheels are not, and which could only +answer `application/zip` for an `.odt` anyway. From 7ae02b2974db493f1b24c4fd15e221ba5656098b Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 1 Aug 2026 17:42:26 +0200 Subject: [PATCH 3/3] feat: remove libmagic, keeping its public API inert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deprecating the option left a branch that could only give worse answers than the default one, and a dependency plus an 8 MB database to carry for it. The implementation, the conan requirement, the `magic.mgc` staging and `project_info::has_libmagic` are all gone, and no job asks for the option any more. Nothing about the API changes. `GlobalParams::libmagic_database_path` and its java, python and objc mirrors still store and return a path, and `ODR_WITH_LIBMAGIC` / the conan `with_libmagic` option are still accepted — conan hard-errors on an unknown option, so dropping it would break a consumer's recipe rather than warn it. They do nothing now; the CMake option says so with a deprecation message, and `docs/design` tracks removing them for a release that may break. `project_info.hpp` and `test_info.hpp` return `std::string_view` while their signatures are being touched anyway. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01C8zVbqCiCedPBvigwpt5ta --- .github/workflows/apple.yml | 4 +- .github/workflows/build_test.yml | 7 +- AGENTS.md | 9 ++- CMakeLists.txt | 69 +++---------------- android/AGENTS.md | 5 +- android/build_native.py | 9 +-- .../app/opendocument/core/OdrAndroidTest.kt | 2 +- .../opendocument/core/android/OdrAndroid.kt | 17 +---- apple/CMakeLists.txt | 3 +- apple/README.md | 4 +- apple/exported_symbols.txt | 2 +- apple/include/OdrCoreObjC/ODRGlobalParams.h | 17 ++--- apple/src/ODRGlobalParams.mm | 7 -- apple/tests/OdrCoreTests.swift | 7 -- conan.lock | 2 - conanfile.py | 16 ++--- docs/design/README.md | 16 ++--- jni/README.md | 4 +- .../app/opendocument/core/GlobalParams.java | 4 +- pyproject.toml | 8 +-- python/AGENTS.md | 5 +- python/README.md | 10 +-- python/pyodr/__init__.py | 33 ++------- python/src/bind_core.cpp | 4 +- python/tests/test_meta.py | 4 +- scripts/run-with-env.sh | 2 +- src/odr/global_params.cpp | 6 +- src/odr/global_params.hpp | 5 +- src/odr/internal/libmagic/libmagic.cpp | 50 -------------- src/odr/internal/libmagic/libmagic.hpp | 7 -- src/odr/internal/magic.cpp | 10 +-- src/odr/internal/magic.hpp | 4 +- src/odr/internal/project_info.cpp.in | 16 +---- src/odr/internal/project_info.hpp | 9 ++- src/odr/odr.cpp | 4 +- test/src/internal/magic_test.cpp | 19 ++--- test/src/test_info.cpp.in | 8 +-- test/src/test_info.hpp | 7 +- test/src/test_util.cpp | 3 +- 39 files changed, 97 insertions(+), 321 deletions(-) delete mode 100644 src/odr/internal/libmagic/libmagic.cpp delete mode 100644 src/odr/internal/libmagic/libmagic.hpp diff --git a/.github/workflows/apple.yml b/.github/workflows/apple.yml index 257bb9c10..37768edbf 100644 --- a/.github/workflows/apple.yml +++ b/.github/workflows/apple.yml @@ -200,8 +200,8 @@ jobs: # The iOS *device* slice is only ever link checked; nothing runs it. The # simulator run is the analogue of android's instrumented job and the only one - # that sees what a device sees — that `+load` fired, that `NSBundle` found - # `magic.mgc`, that a temp directory is writable inside an app container. + # that sees what a device sees — that `+load` fired, that `NSBundle` found the + # renderer assets, that a temp directory is writable inside an app container. test: needs: xcframework runs-on: macos-15 diff --git a/.github/workflows/build_test.yml b/.github/workflows/build_test.yml index 6b8000134..69abf0b57 100644 --- a/.github/workflows/build_test.yml +++ b/.github/workflows/build_test.yml @@ -44,10 +44,7 @@ jobs: matrix: include: - { os: ubuntu-24.04, build_profile: ubuntu-24.04-clang-18, host_profile: ubuntu-24.04-clang-18, bindings: true } - # the one job that still asks for the deprecated libmagic, so it keeps - # compiling and linking until it goes. everything else, `test` - # included, builds the default and detects on its own - - { os: ubuntu-24.04, build_profile: ubuntu-24.04-gcc-14, host_profile: ubuntu-24.04-gcc-14, libmagic: true } + - { os: ubuntu-24.04, build_profile: ubuntu-24.04-gcc-14, host_profile: ubuntu-24.04-gcc-14 } - { os: macos-15, build_profile: macos-15-armv8-clang-14, host_profile: macos-15-armv8-clang-14 } - { os: macos-26, build_profile: macos-26-armv8-clang-14, host_profile: macos-26-armv8-clang-14, bindings: true } - { os: windows-2022, build_profile: windows-2022-msvc-1940, host_profile: windows-2022-msvc-1940 } @@ -114,7 +111,6 @@ jobs: run: > conan install . ${{ matrix.bindings && '-o "&:with_jni=True" -o "&:with_python=True"' || '' }} - ${{ matrix.libmagic && '-o "&:with_libmagic=True"' || '' }} --profile:host '${{ matrix.host_profile }}' --profile:build '${{ matrix.build_profile }}' --build missing @@ -130,7 +126,6 @@ jobs: -DCMAKE_INSTALL_PREFIX=install -DODR_TEST=ON -DODR_BUNDLE_ASSETS=ON - ${{ matrix.libmagic && '-DODR_WITH_LIBMAGIC=ON' || '' }} ${{ matrix.bindings && '-DODR_JNI=ON -DODR_PYTHON=ON' || '' }} - name: cmake diff --git a/AGENTS.md b/AGENTS.md index ec534e85a..832afffe3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -21,8 +21,7 @@ bytes ─▶ magic/open_strategy ─▶ DecodedFile ─▶ Document ─▶ Eleme 1. **Detect** — `internal/magic.cpp` sniffs the head of the file; `internal/open_strategy.cpp` picks a `FileType` + `DecoderEngine` and builds the matching `abstract::DecodedFile`. `odr::mimetype` composes the two, so a - zip is named by what is inside it. (`internal/libmagic` is a deprecated - alternative to that last step, off by default.) + zip is named by what is inside it. 2. **Decode** — a document file yields an `abstract::Document`. 3. **Element tree** — a `Document` exposes a root `ElementIdentifier` plus an `abstract::ElementAdapter`. Public value-semantics handles (`Element`, `Slide`, @@ -90,9 +89,9 @@ cmake --build cmake-build-relwithdebinfo --target translate # CLI: file → HTM when really necessary. - **Run the test binary from the build dir** so output stays out of the repo tree. - **For debugging, prefer the `translate` CLI** on a single file over the suite. -- CMake options (`CMakeLists.txt`): `ODR_TEST`, `ODR_CLI`, `ODR_WITH_LIBMAGIC`, - `ODR_PYTHON`, `ODR_JNI`, `ODR_APPLE`, `ODR_CLANG_TIDY`. A new `.cpp` must be - added to `ODR_SOURCE_FILES`. +- CMake options (`CMakeLists.txt`): `ODR_TEST`, `ODR_CLI`, `ODR_PYTHON`, + `ODR_JNI`, `ODR_APPLE`, `ODR_CLANG_TIDY`. A new `.cpp` must be added to + `ODR_SOURCE_FILES`. - **Test data is fetched, not vendored**, and opt in: `-DODR_TEST_FETCH_DATA=ON` makes `cmake/test_data.cmake` clone the repositories pinned in `test/data.cmake` into `test/data/` (they were submodules until then; read diff --git a/CMakeLists.txt b/CMakeLists.txt index 1d984ba7b..73785b457 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -16,12 +16,11 @@ option(ODR_TEST_FETCH_DATA "Fetch the test data pinned in test/data.cmake" OFF) option(ODR_CLI "enable command line interface" ON) option(ODR_WITH_HTTP_SERVER "Build the HTTP server (requires cpp-httplib)" ON) option(ODR_CLANG_TIDY "Run clang-tidy static analysis" OFF) -# Deprecated, and off by default since our own detection took `mimetype` over: -# that reads inside zip and cfb containers, which libmagic cannot, and it costs -# neither a dependency nor the 8 MB `magic.mgc` database every consumer has to -# ship and point us at. Kept for whoever still relies on the old answers; see -# "Open tasks" in docs/design/README.md. -option(ODR_WITH_LIBMAGIC "Build with libmagic (deprecated)" OFF) +# Removed, and accepted only so a consumer still passing it keeps configuring. +# `odr::mimetype` is our own detection now: it reads inside zip and cfb +# containers, which libmagic cannot, and it costs neither a dependency nor the +# 8 MB `magic.mgc` database every consumer had to ship and point us at. +option(ODR_WITH_LIBMAGIC "Removed, does nothing (deprecated)" OFF) option(ODR_BUNDLE_ASSETS "Bundle assets during build and install" OFF) option(ODR_PYTHON "Build Python bindings" OFF) option(ODR_JNI "Build JNI bindings" OFF) @@ -279,30 +278,6 @@ set(ODR_BUILD_ODR_DATA_PATH "${CMAKE_CURRENT_BINARY_DIR}/data") file(COPY "${odr.js_SOURCE_DIR}/" DESTINATION "${ODR_BUILD_ODR_DATA_PATH}") set(ODR_INSTALL_ODR_DATA_PATH "${CMAKE_INSTALL_DATADIR}") -# Validate an injected data-file path before it is used to bundle assets. -# -# These paths (LIBMAGIC_DATABASE_PATH, ...) are consumed but -# never discovered by CMake — they must be injected from outside via `-D...=`; -# our conanfile.py bridges them from the dependency runenv. A consumer that -# builds odrcore without that bridge left the variable empty, and -# `file(COPY "${empty}/" ...)` degraded to `file(COPY "/" ...)`, i.e. an attempt -# to recursively copy the whole root filesystem (see issue #599). This guard -# fails fast with an actionable message instead of copying "/". -# -# path value of the externally-injected path variable (may be empty) -# name name of that variable, for the diagnostic -function(odr_require_data_path path name) - if (NOT path) - message(FATAL_ERROR - "ODR_BUNDLE_ASSETS is ON but ${name} is not set. Provide it " - "explicitly (e.g. -D${name}=) or build through the conan " - "recipe, or disable bundling with -DODR_BUNDLE_ASSETS=OFF.") - endif () - if (NOT EXISTS "${path}") - message(FATAL_ERROR "${name} points to a path that does not exist: '${path}'.") - endif () -endfunction() - if (ODR_WITH_HTTP_SERVER) find_package(httplib REQUIRED) target_sources(odr @@ -320,37 +295,9 @@ if (ODR_WITH_HTTP_SERVER) endif () if (ODR_WITH_LIBMAGIC) message(DEPRECATION - "ODR_WITH_LIBMAGIC is deprecated and will be removed. Our own " - "detection answers `odr::mimetype` without it, and better for " - "container formats.") - - find_package(libmagic REQUIRED) - target_sources(odr - PRIVATE - "src/odr/internal/libmagic/libmagic.cpp" - ) - target_link_libraries(odr - PRIVATE - libmagic::libmagic - ) - target_compile_definitions(odr - PRIVATE - ODR_WITH_LIBMAGIC - ) - - if (ODR_BUNDLE_ASSETS) - odr_require_data_path("${LIBMAGIC_DATABASE_PATH}" LIBMAGIC_DATABASE_PATH) - - set(ODR_BUILD_LIBMAGIC_DATABASE_PATH "${ODR_BUILD_ODR_DATA_PATH}/magic.mgc") - - file(COPY_FILE "${LIBMAGIC_DATABASE_PATH}" "${ODR_BUILD_LIBMAGIC_DATABASE_PATH}") - - set(ODR_INSTALL_LIBMAGIC_DATABASE_PATH "${ODR_INSTALL_ODR_DATA_PATH}/magic.mgc") - else () - set(ODR_BUILD_LIBMAGIC_DATABASE_PATH "${LIBMAGIC_DATABASE_PATH}") - - set(ODR_INSTALL_LIBMAGIC_DATABASE_PATH "${LIBMAGIC_DATABASE_PATH}") - endif () + "ODR_WITH_LIBMAGIC no longer does anything: libmagic is gone and " + "`odr::mimetype` is our own detection. Drop the option; " + "`GlobalParams::libmagic_database_path` is inert too.") endif () configure_file("src/odr/internal/project_info.cpp.in" "src/odr/internal/project_info.cpp") diff --git a/android/AGENTS.md b/android/AGENTS.md index 369707888..cfbf00b6c 100644 --- a/android/AGENTS.md +++ b/android/AGENTS.md @@ -66,6 +66,5 @@ calls it. finds the incompatibilities first. - The manifest stays permission-free; permissions the server needs are the app's call, and documented in `README.md` instead. -- Assets keep the `core/odrcore` (+ `core/libmagic`, in a deprecated libmagic - build) layout of the droid conan deployer, so an app can move between the two - packagings without touching its extraction code. +- Assets keep the `core/odrcore` layout of the droid conan deployer, so an app + can move between the two packagings without touching its extraction code. diff --git a/android/build_native.py b/android/build_native.py index 43c5b135b..22e34ebe9 100644 --- a/android/build_native.py +++ b/android/build_native.py @@ -12,7 +12,6 @@ prebuilt/jniLibs//libodr_jni.so the bindings, core linked in prebuilt/jniLibs//libc++_shared.so from the NDK, see below prebuilt/assets/core/odrcore/* css/js of the html renderer - prebuilt/assets/core/libmagic/magic.mgc only in a deprecated libmagic build `libc++_shared.so` has to be shipped because the android profiles build against the shared c++ runtime and nothing else in a consuming app pulls it in; the @@ -116,13 +115,7 @@ def build(architecture: str, conan: str, build_profile: str, output: Path) -> No data = cmake_dir / "data" assets = output / "assets" / "core" shutil.rmtree(assets, ignore_errors=True) - shutil.copytree(data, assets / "odrcore", ignore=shutil.ignore_patterns("magic.mgc")) - # only staged by a build that still asks for the deprecated libmagic, which - # this one does not; kept so such a build still packages the same way - magic = data / "magic.mgc" - if magic.is_file(): - (assets / "libmagic").mkdir(parents=True, exist_ok=True) - shutil.copy2(magic, assets / "libmagic" / "magic.mgc") + shutil.copytree(data, assets / "odrcore") def main() -> int: diff --git a/android/src/androidTest/java/app/opendocument/core/OdrAndroidTest.kt b/android/src/androidTest/java/app/opendocument/core/OdrAndroidTest.kt index f24c7f302..8b723c8e7 100644 --- a/android/src/androidTest/java/app/opendocument/core/OdrAndroidTest.kt +++ b/android/src/androidTest/java/app/opendocument/core/OdrAndroidTest.kt @@ -48,7 +48,7 @@ class OdrAndroidTest { val odt = TestFiles.odtFile(directory) // naming the odt rather than the zip holding it means detection opened the - // container, which is the whole reason this no longer goes through libmagic + // container assertEquals("application/vnd.oasis.opendocument.text", Odr.mimetype(odt.toString())) } } diff --git a/android/src/main/java/app/opendocument/core/android/OdrAndroid.kt b/android/src/main/java/app/opendocument/core/android/OdrAndroid.kt index 405f25287..71ce46882 100644 --- a/android/src/main/java/app/opendocument/core/android/OdrAndroid.kt +++ b/android/src/main/java/app/opendocument/core/android/OdrAndroid.kt @@ -22,9 +22,7 @@ object OdrAndroid { /** Asset directory this AAR ships its runtime data under. */ private const val ASSETS = "core" - /** - * A deprecated libmagic build adds an 8 MB database, so the default 8 KB is a lot of syscalls. - */ + /** The default 8 KB would be a lot of syscalls for assets this size. */ private const val BUFFER_SIZE = 64 * 1024 private var initialized = false @@ -58,22 +56,9 @@ object OdrAndroid { } GlobalParams.setOdrCoreDataPath(File(root, "odrcore").absolutePath) - pointAtLibmagicDatabase(root) initialized = true } - /** - * Only an AAR whose native library was built against the deprecated libmagic carries the - * database, and only such a library reads the path — finding nothing is the normal case. - */ - @Suppress("DEPRECATION") - private fun pointAtLibmagicDatabase(root: File) { - val database = File(root, "libmagic/magic.mgc") - if (database.isFile) { - GlobalParams.setLibmagicDatabasePath(database.absolutePath) - } - } - /** An asset directory lists its children; an asset file lists nothing. */ private fun extract(assets: AssetManager, path: String, target: File) { val children = assets.list(path) diff --git a/apple/CMakeLists.txt b/apple/CMakeLists.txt index 70c63accf..cb54fc50d 100644 --- a/apple/CMakeLists.txt +++ b/apple/CMakeLists.txt @@ -64,8 +64,7 @@ set(ODR_APPLE_PUBLIC_HEADERS "include/OdrCoreObjC/ODRTable.h" ) -# Whatever the root build staged: the renderer data, and `magic.mgc` beside it -# in the deprecated builds that still ask for libmagic. +# The renderer's css/js, as the root build staged them. file(GLOB ODR_APPLE_RESOURCES "${ODR_BUILD_ODR_DATA_PATH}/*") if (NOT ODR_APPLE_RESOURCES) message(FATAL_ERROR "no resources in ${ODR_BUILD_ODR_DATA_PATH}") diff --git a/apple/README.md b/apple/README.md index aeb885d68..ed89e64e9 100644 --- a/apple/README.md +++ b/apple/README.md @@ -32,8 +32,8 @@ for view in service.views { } ``` -Nothing needs configuring first. The framework points odrcore at the css, JS and -libmagic database it carries before `main` runs — that is what the `+load` in +Nothing needs configuring first. The framework points odrcore at the css and JS +it carries before `main` runs — that is what the `+load` in `OdrCoreBootstrap.mm` is for. Override it with `GlobalParams` from `application(_:didFinishLaunchingWithOptions:)` if you relocated the resources. diff --git a/apple/exported_symbols.txt b/apple/exported_symbols.txt index 6371a7d36..ed9ca00e9 100644 --- a/apple/exported_symbols.txt +++ b/apple/exported_symbols.txt @@ -1,5 +1,5 @@ # The framework's entire public surface is its ObjC classes, so export those -# and nothing else. Everything linked in — cryptopp, libmagic, all of `odr::` — +# and nothing else. Everything linked in — cryptopp, pugixml, all of `odr::` — # becomes private-extern, which keeps an app that links its own copy of any of # them from binding against ours. # diff --git a/apple/include/OdrCoreObjC/ODRGlobalParams.h b/apple/include/OdrCoreObjC/ODRGlobalParams.h index 1649aff9f..0c348d50e 100644 --- a/apple/include/OdrCoreObjC/ODRGlobalParams.h +++ b/apple/include/OdrCoreObjC/ODRGlobalParams.h @@ -4,10 +4,10 @@ NS_ASSUME_NONNULL_BEGIN /// Where odrcore looks for the files it needs at runtime. /// -/// The framework points these at its own bundle before `main` runs, so an app -/// that ships this framework unmodified never has to call anything here. Set -/// them only to override that — from `application:didFinishLaunching...`, which -/// is late enough to win. +/// The framework points `odrCoreDataPath` at its own bundle before `main` runs, +/// so an app that ships this framework unmodified never has to call anything +/// here. Set it only to override that — from +/// `application:didFinishLaunching...`, which is late enough to win. NS_SWIFT_NAME(GlobalParams) @interface ODRGlobalParams : NSObject @@ -15,13 +15,14 @@ NS_SWIFT_NAME(GlobalParams) @property(class, nonatomic, copy) NSString *odrCoreDataPath; /// The libmagic database (`magic.mgc`). /// -/// Deprecated: read only by a core built with `ODR_WITH_LIBMAGIC`, which is off -/// by default now that detection is odrcore's own and needs no database. +/// Deprecated and inert: libmagic is gone and nothing reads this. It still +/// returns whatever is set, so a caller that sets it keeps working — detection +/// is odrcore's own now and needs no database. @property(class, nonatomic, copy) NSString *libmagicDatabasePath; -/// Points the paths above at this framework's bundle. Runs automatically at +/// Points `odrCoreDataPath` at this framework's bundle. Runs automatically at /// load; public because a consumer who relocated the resources — or reset the -/// paths and wants the defaults back — needs a way to redo it. +/// path and wants the default back — needs a way to redo it. + (void)bootstrapFromFrameworkBundle; - (instancetype)init NS_UNAVAILABLE; diff --git a/apple/src/ODRGlobalParams.mm b/apple/src/ODRGlobalParams.mm index abc114ab4..e33afbdef 100644 --- a/apple/src/ODRGlobalParams.mm +++ b/apple/src/ODRGlobalParams.mm @@ -46,13 +46,6 @@ + (void)bootstrapFromFrameworkBundle { if (resources != nil) { ODRGlobalParams.odrCoreDataPath = resources; } - - // Only a framework built against the deprecated libmagic carries the - // database, so normally there is nothing to point at. - NSString *const magic = [bundle pathForResource:@"magic" ofType:@"mgc"]; - if (magic != nil) { - ODRGlobalParams.libmagicDatabasePath = magic; - } } @end diff --git a/apple/tests/OdrCoreTests.swift b/apple/tests/OdrCoreTests.swift index 9d00ceb4d..8a4e7184f 100644 --- a/apple/tests/OdrCoreTests.swift +++ b/apple/tests/OdrCoreTests.swift @@ -35,13 +35,6 @@ final class BootstrapTests: XCTestCase { "\(path) does not contain the renderer's css") } - func testLibmagicDatabaseIsBundled() { - let path = GlobalParams.libmagicDatabasePath - XCTAssertTrue( - FileManager.default.fileExists(atPath: path), - "libmagic database missing at \(path)") - } - func testLibraryIdentifiesItself() { XCTAssertFalse(Odr.identification.isEmpty) XCTAssertFalse(Odr.commitHash.isEmpty) diff --git a/conan.lock b/conan.lock index 36313d6e1..ff0ff7189 100644 --- a/conan.lock +++ b/conan.lock @@ -11,7 +11,6 @@ "pugixml/1.15#979e88f4fafbfe3585d2c0510a071cc7%1739435725.483", "nlohmann_json/3.12.0#2d634ab0ec8d9f56353e5ccef6d6612c%1744735883.94", "miniz/3.0.2#bfbce07c6654293cce27ee24129d2df7%1743673472.805", - "libmagic/5.45#791d5bad38d33272bb120994a198b1ac%1727273086.09", "gtest/1.14.0#f8f0757a574a8dd747d16af62d6eb1b7%1743410807.169", "cryptopp/8.9.0#7a51e0038756b21bc3a6b82d681d5906%1758206597.119", "cpp-httplib/0.16.3#7aa89fbb81ffd19539a49fc132502966%1748426320.106", @@ -24,7 +23,6 @@ "xz_utils/5.8.3#a8432fead347c69d8b2737c35f936132%1775752656.4", "m4/1.4.19#34c4bbc3eeebe98ca6edf2f52d602e7d%1777282960.259", "libtool/2.4.7#14e7739cc128bc1623d2ed318008e47e%1755679003.847", - "libmagic/5.45#791d5bad38d33272bb120994a198b1ac%1727273086.09", "gnu-config/cci.20210814#466e9d4d7779e1c142443f7ea44b4284%1762363589.329", "cmake/4.3.2#5bae0b181463ca4dc3e924a981367ee6%1779785720.872", "cmake/3.31.12#173a926abc2b77f03c826b6fd6539426%1779785723.158", diff --git a/conanfile.py b/conanfile.py index a2edcb9a4..100abcc4d 100644 --- a/conanfile.py +++ b/conanfile.py @@ -1,7 +1,6 @@ from conan import ConanFile from conan.tools.build import check_min_cppstd from conan.tools.cmake import CMakeToolchain, CMakeDeps, CMake -from conan.tools.env import Environment class OpenDocumentCoreConan(ConanFile): @@ -17,7 +16,8 @@ class OpenDocumentCoreConan(ConanFile): options = { "shared": [True, False], "fPIC": [True, False], - # deprecated, see the option in CMakeLists.txt + # removed and inert, kept only so a consumer still passing it does not + # hard-fail on an unknown option; see CMakeLists.txt "with_libmagic": [True, False], "with_http_server": [True, False], "with_cli": [True, False], @@ -56,8 +56,6 @@ def requirements(self): if self.options.get_safe("with_http_server", False): self.requires("cpp-httplib/0.16.3") self.requires("argon2/20190702-odr") - if self.options.get_safe("with_libmagic", False): - self.requires("libmagic/5.45") if self.options.get_safe("with_python", False): self.requires("pybind11/2.13.6") @@ -76,6 +74,8 @@ def generate(self): tc = CMakeToolchain(self) tc.variables["CMAKE_PROJECT_VERSION"] = self.version tc.variables["ODR_TEST"] = False + # forwarded only so the CMake deprecation warning reaches a consumer + # who still sets it; neither end does anything with it tc.variables["ODR_WITH_LIBMAGIC"] = self.options.get_safe("with_libmagic", False) tc.variables["ODR_WITH_HTTP_SERVER"] = self.options.get_safe("with_http_server", False) tc.variables["ODR_CLI"] = self.options.get_safe("with_cli", True) @@ -84,14 +84,6 @@ def generate(self): tc.variables["ODR_APPLE"] = self.options.get_safe("with_apple", False) tc.variables["ODR_BUNDLE_ASSETS"] = self.options.get_safe("bundle_assets", False) - # Get runenv info, exported by package_info() of dependencies - # We need to obtain MAGIC - runenv_info = Environment() - for dep in self.dependencies.host.topological_sort.values(): - runenv_info.compose_env(dep.runenv_info) - envvars = runenv_info.vars(self) - tc.variables["LIBMAGIC_DATABASE_PATH"] = envvars.get("MAGIC") - tc.generate() deps = CMakeDeps(self) diff --git a/docs/design/README.md b/docs/design/README.md index c9ea9d281..c0144ed7b 100644 --- a/docs/design/README.md +++ b/docs/design/README.md @@ -95,15 +95,11 @@ ### Open tasks -- remove libmagic. `ODR_WITH_LIBMAGIC` and the conan `with_libmagic` option are - deprecated and off by default: `odr::mimetype` runs the open strategy instead, - which names what is *inside* a zip or a compound file where libmagic could only - answer `application/zip`, and which gets `svm` right where libmagic does not. - What goes with the option: `src/odr/internal/libmagic/`, the CMake block that - stages `magic.mgc`, `GlobalParams::libmagic_database_path` and its java, - python and objc mirrors, `project_info::has_libmagic`, - `pyodr._init_libmagic_database_path`, and the `magic.mgc` lookups that stay - behind for it — the apple bootstrap, `android/build_native.py` and - `OdrAndroid.pointAtLibmagicDatabase` +- drop the last inert traces of libmagic once consumers have moved off them: + `GlobalParams::libmagic_database_path` and its java, python and objc mirrors + still store and return a path nothing reads, and `ODR_WITH_LIBMAGIC` / + the conan `with_libmagic` option are still accepted so a build that sets one + keeps configuring. Removing them is the breaking change this deliberately + is not. - collect additional pdf files via the translate cli and capture the ones that break - exercise editing across all formats (odp editing appears broken via an HTML issue) diff --git a/jni/README.md b/jni/README.md index d8072bcb9..dff3307ff 100644 --- a/jni/README.md +++ b/jni/README.md @@ -74,8 +74,8 @@ standalone against an installed `odrcore` package. Rendering uses shipped assets (CSS/JS). Point the library at them via `GlobalParams.setOdrCoreDataPath(...)` or the `ODR_CORE_DATA_PATH` environment -variable (the tests read it; for in-tree builds it is `build/data`). The -optional libmagic backend has its own path on `GlobalParams`. +variable (the tests read it; for in-tree builds it is `build/data`). Detection +needs no runtime data of its own. ## Notes diff --git a/jni/java/app/opendocument/core/GlobalParams.java b/jni/java/app/opendocument/core/GlobalParams.java index 010821329..e8fb53dca 100644 --- a/jni/java/app/opendocument/core/GlobalParams.java +++ b/jni/java/app/opendocument/core/GlobalParams.java @@ -9,8 +9,8 @@ public final class GlobalParams { public static native String odrCoreDataPath(); /** - * @deprecated Read only by a native library built with the deprecated {@code - * ODR_WITH_LIBMAGIC}. Detection is our own otherwise and needs no database. + * @deprecated Inert: libmagic is gone and nothing reads this. It still returns whatever was set, + * so a caller that sets it keeps working — detection is our own now and needs no database. */ @Deprecated public static native String libmagicDatabasePath(); diff --git a/pyproject.toml b/pyproject.toml index 1fb014a48..6c40e8a64 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -61,11 +61,9 @@ ODR_PYTHON = "ON" ODR_CLI = "OFF" ODR_TEST = "OFF" BUILD_SHARED_LIBS = "OFF" -# libmagic is deprecated and stays off: `mimetype` runs the open strategy now, -# which names what is *inside* a zip or a compound file where libmagic could -# only answer `application/zip`. The wheel no longer carries `magic.mgc` for it -# (~0.4 MB deflated of 8.5 MB), and `pyodr/__init__.py` finding no database is -# the normal case rather than a problem. +# Stages the renderer's css/js so `python/CMakeLists.txt` can install them as +# `pyodr/data`. The wheel used to carry `magic.mgc` here too (~0.4 MB deflated +# of 8.5 MB); libmagic is gone and `mimetype` needs no database. ODR_BUNDLE_ASSETS = "ON" # Section must exist for setuptools-scm to pick up its pyproject config. diff --git a/python/AGENTS.md b/python/AGENTS.md index 1baa74880..e870f8610 100644 --- a/python/AGENTS.md +++ b/python/AGENTS.md @@ -35,9 +35,8 @@ pybind11 bindings for the public C++ API (`src/odr/*.hpp`), packaged as returning `bytes`/`str` via `std::ostringstream`. - New public C++ API? Extend the matching `bind_*.cpp` and add a pytest. - C++ sources follow the repo clang-format; python is formatted with `black`. -- Tests must stay hermetic: build inputs inline in `tests/conftest.py`; +- Tests must stay hermetic: build inputs inline in `tests/conftest.py`, and HTML-rendering tests take the `core_data_path` fixture (skips when assets are - missing), and tests needing libmagic skip on `_libmagic_database()` in - `tests/test_meta.py` (empty when the build has no database). + missing). - Build/test loop: see `python/README.md`; CI lives in `.github/workflows/python.yml`. diff --git a/python/README.md b/python/README.md index a651882ab..fef1bfc78 100644 --- a/python/README.md +++ b/python/README.md @@ -33,7 +33,7 @@ PYTHONPATH=build/python ODR_CORE_DATA_PATH=build/data python -m pytest python/te `pip install .` from the repository root builds a wheel via scikit-build-core (see the root `pyproject.toml`); run `conan install` first and point `CMAKE_ARGS` at the generated `conan_toolchain.cmake` so the C++ dependencies -resolve. Wheels bundle libmagic's database, so match that option: +resolve, and match the wheel's asset bundling: ```bash conan install . -o '&:with_python=True' -o '&:bundle_assets=True' --build missing @@ -48,7 +48,7 @@ tests read it) or call `pyodr.GlobalParams.set_odr_core_data_path(...)`. Missing assets are not fatal — rendering then fails on the individual resource. MIME detection needs no runtime data: `mimetype` runs the open strategy, so it -names what is *inside* a zip or a compound file. `ODR_LIBMAGIC_DATABASE_PATH` -and `set_libmagic_database_path(...)` are read only by a core built with the -deprecated `ODR_WITH_LIBMAGIC`, which the wheels are not, and which could only -answer `application/zip` for an `.odt` anyway. +names what is *inside* a zip or a compound file. `libmagic_database_path()` and +`set_libmagic_database_path(...)` are deprecated leftovers of the libmagic +backend that used to answer this and could only say `application/zip` for an +`.odt`; they still store and return a path, but nothing reads it. diff --git a/python/pyodr/__init__.py b/python/pyodr/__init__.py index b871bb480..3ca918b71 100644 --- a/python/pyodr/__init__.py +++ b/python/pyodr/__init__.py @@ -26,10 +26,10 @@ def _is_configured(path: str) -> bool: - # The compiled-in defaults are install-relative guesses ("share", - # "share/magic.mgc"). Resolving those against the process working directory - # would make the outcome depend on where the interpreter was launched, so - # only an absolute path that exists counts as already configured. + # The compiled-in default is an install-relative guess ("share"). Resolving + # that against the process working directory would make the outcome depend + # on where the interpreter was launched, so only an absolute path that + # exists counts as already configured. return bool(path) and os.path.isabs(path) and os.path.exists(path) @@ -47,25 +47,6 @@ def _init_odr_core_data_path() -> None: _core.GlobalParams.set_odr_core_data_path(str(_BUNDLED_DATA_PATH)) -def _init_libmagic_database_path() -> None: - # Deprecated, and a no-op for the wheels we ship: only a core built with - # `ODR_WITH_LIBMAGIC` reads this, and that is off by default now that - # detection is odrcore's own. Same resolution order as above, for a file - # rather than a directory, and finding nothing is the normal case. - if _is_configured(_core.GlobalParams.libmagic_database_path()): - return - env_path = os.environ.get("ODR_LIBMAGIC_DATABASE_PATH") - if env_path: - _core.GlobalParams.set_libmagic_database_path(env_path) - return - database_path = _BUNDLED_DATA_PATH / "magic.mgc" - if database_path.is_file(): - _core.GlobalParams.set_libmagic_database_path(str(database_path)) - - -def _init_data_paths() -> None: - _init_odr_core_data_path() - _init_libmagic_database_path() - - -_init_data_paths() +# `GlobalParams.libmagic_database_path` is left alone: libmagic is gone, so +# there is no database to resolve and nothing that would read one. +_init_odr_core_data_path() diff --git a/python/src/bind_core.cpp b/python/src/bind_core.cpp index 8a711ca65..b11063e0d 100644 --- a/python/src/bind_core.cpp +++ b/python/src/bind_core.cpp @@ -25,8 +25,8 @@ void odr_python::bind_core(py::module_ &m) { py::class_(m, "GlobalParams", "Global resource paths of the library.") .def_static("odr_core_data_path", &odr::GlobalParams::odr_core_data_path) - // the two libmagic paths are deprecated: only a core built with the - // deprecated `ODR_WITH_LIBMAGIC` reads them + // the two libmagic paths are deprecated and inert: libmagic is gone, so + // nothing reads back what they store .def_static("libmagic_database_path", &odr::GlobalParams::libmagic_database_path) .def_static("set_odr_core_data_path", diff --git a/python/tests/test_meta.py b/python/tests/test_meta.py index f6ad88472..01cde621f 100644 --- a/python/tests/test_meta.py +++ b/python/tests/test_meta.py @@ -120,11 +120,9 @@ def test_decoded_file_capabilities(odt_path): def test_global_params(): assert isinstance(pyodr.GlobalParams.odr_core_data_path(), str) - assert isinstance(pyodr.GlobalParams.libmagic_database_path(), str) def test_mimetype_names_what_is_inside_the_container(odt_path): # An ODF file is a ZIP, and the answer worth having is the one from inside - # it. Detection opens the container to get there, so this holds with or - # without the deprecated libmagic. + # it. Detection opens the container to get there. assert pyodr.mimetype(str(odt_path)) == "application/vnd.oasis.opendocument.text" diff --git a/scripts/run-with-env.sh b/scripts/run-with-env.sh index 4a5fa8441..f44735949 100755 --- a/scripts/run-with-env.sh +++ b/scripts/run-with-env.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # Load the Conan runtime env from .vscode/.env (see gen-vscode-env.py) and exec # the given command with it. Used by VS Code tasks so the test binary and CLI -# tools can find the libmagic database. +# tools run against the dependency runtime the build was configured with. # # Usage: scripts/run-with-env.sh [args...] set -euo pipefail diff --git a/src/odr/global_params.cpp b/src/odr/global_params.cpp index 83983c99c..d7a5c11cc 100644 --- a/src/odr/global_params.cpp +++ b/src/odr/global_params.cpp @@ -26,9 +26,9 @@ void GlobalParams::set_libmagic_database_path(const std::string &path) { instance().m_libmagic_database_path = path; } +// `m_libmagic_database_path` starts empty: nothing reads it, and there is no +// longer a database path to default it to. GlobalParams::GlobalParams() - : m_odr_core_data_path{internal::project_info::odr_data_path()}, - m_libmagic_database_path{ - internal::project_info::libmagic_database_path()} {} + : m_odr_core_data_path{internal::project_info::odr_data_path()} {} } // namespace odr diff --git a/src/odr/global_params.hpp b/src/odr/global_params.hpp index 3f9b1fb8e..1db36dd07 100644 --- a/src/odr/global_params.hpp +++ b/src/odr/global_params.hpp @@ -7,8 +7,9 @@ namespace odr { class GlobalParams final { public: static const std::string &odr_core_data_path(); - /// @deprecated Read only by a build with the deprecated `ODR_WITH_LIBMAGIC`. - /// Detection is our own otherwise, and looks at nothing outside the file. + /// @deprecated Inert: libmagic is gone and nothing reads this. It still + /// stores and returns whatever is set, so a caller that sets it keeps + /// working — detection is our own now, and looks at nothing outside the file. static const std::string &libmagic_database_path(); static void set_odr_core_data_path(const std::string &path); diff --git a/src/odr/internal/libmagic/libmagic.cpp b/src/odr/internal/libmagic/libmagic.cpp deleted file mode 100644 index 4c2bc2126..000000000 --- a/src/odr/internal/libmagic/libmagic.cpp +++ /dev/null @@ -1,50 +0,0 @@ -#include - -#include - -#include - -#include - -namespace odr::internal { - -namespace { - -void magic_deleter(const magic_t magic_cookie) { - if (magic_cookie != nullptr) { - magic_close(magic_cookie); - } -} - -magic_t get_magic_cookie() { - using Holder = - std::unique_ptr, decltype(&magic_deleter)>; - static Holder magic_cookie(nullptr, &magic_deleter); - - if (magic_cookie != nullptr) { - return magic_cookie.get(); - } - - magic_cookie = Holder(magic_open(MAGIC_MIME_TYPE), &magic_deleter); - if (magic_cookie == nullptr) { - throw std::runtime_error("magic_open failed"); - } - if (magic_load(magic_cookie.get(), - GlobalParams::libmagic_database_path().c_str()) == 0) { - return magic_cookie.get(); - } - if (magic_load(magic_cookie.get(), nullptr) == 0) { - return magic_cookie.get(); - } - magic_cookie.reset(); - throw std::runtime_error("magic_load failed"); -} - -} // namespace - -const char *libmagic::mimetype(const std::string &path) { - const magic_t magic_cookie = get_magic_cookie(); - return magic_file(magic_cookie, path.c_str()); -} - -} // namespace odr::internal diff --git a/src/odr/internal/libmagic/libmagic.hpp b/src/odr/internal/libmagic/libmagic.hpp deleted file mode 100644 index 17f5a7c9a..000000000 --- a/src/odr/internal/libmagic/libmagic.hpp +++ /dev/null @@ -1,7 +0,0 @@ -#pragma once - -#include - -namespace odr::internal::libmagic { -const char *mimetype(const std::string &path); -} diff --git a/src/odr/internal/magic.cpp b/src/odr/internal/magic.cpp index 5595bfe44..4a288939d 100644 --- a/src/odr/internal/magic.cpp +++ b/src/odr/internal/magic.cpp @@ -6,7 +6,6 @@ #include #include -#include #include #include @@ -187,13 +186,7 @@ FileType magic::file_type(const File &file) { } std::string_view magic::mimetype(const std::string &path, - [[maybe_unused]] const Logger &logger) { -#ifdef ODR_WITH_LIBMAGIC - // deprecated, and no longer what a default build does - see the option in - // CMakeLists.txt. it cannot see inside a zip or a compound file binary, so - // it answers `application/zip` where the branch below names the document - return libmagic::mimetype(path); -#else + const Logger &logger) { // the signature table above is not enough on its own: a zip and a compound // file binary say nothing about which document they hold, and the answer for // those only comes out of opening them. that is what `list_file_types` does, @@ -205,7 +198,6 @@ std::string_view magic::mimetype(const std::string &path, } return odr::mimetype_by_file_type(file_types.back()); -#endif } } // namespace odr::internal diff --git a/src/odr/internal/magic.hpp b/src/odr/internal/magic.hpp index cf780e3e9..14e23f58e 100644 --- a/src/odr/internal/magic.hpp +++ b/src/odr/internal/magic.hpp @@ -19,7 +19,7 @@ FileType file_type(std::istream &in); FileType file_type(const abstract::File &file); FileType file_type(const File &file); -/// Without libmagic this opens the file to tell what a container holds, so it -/// needs a logger like the rest of the open strategy. +/// Opens the file to tell what a container holds, so it needs a logger like the +/// rest of the open strategy. std::string_view mimetype(const std::string &path, const Logger &logger); } // namespace odr::internal::magic diff --git a/src/odr/internal/project_info.cpp.in b/src/odr/internal/project_info.cpp.in index 9b463e284..0712cf25f 100644 --- a/src/odr/internal/project_info.cpp.in +++ b/src/odr/internal/project_info.cpp.in @@ -2,7 +2,7 @@ namespace odr::internal { -const char *project_info::version() noexcept { +std::string_view project_info::version() noexcept { return "${CMAKE_PROJECT_VERSION}"; } @@ -14,20 +14,8 @@ bool project_info::is_debug() noexcept { #endif } -bool project_info::has_libmagic() noexcept { -#ifdef ODR_WITH_LIBMAGIC - return true; -#else - return false; -#endif -} - -const char *project_info::odr_data_path() noexcept { +std::string_view project_info::odr_data_path() noexcept { return "${ODR_INSTALL_ODR_DATA_PATH}"; } -const char *project_info::libmagic_database_path() noexcept { - return "${ODR_INSTALL_LIBMAGIC_DATABASE_PATH}"; -} - } // namespace odr::internal diff --git a/src/odr/internal/project_info.hpp b/src/odr/internal/project_info.hpp index a99cc7bb4..4183979be 100644 --- a/src/odr/internal/project_info.hpp +++ b/src/odr/internal/project_info.hpp @@ -1,13 +1,12 @@ #pragma once +#include + namespace odr::internal::project_info { -const char *version() noexcept; +std::string_view version() noexcept; bool is_debug() noexcept; -bool has_libmagic() noexcept; - -const char *odr_data_path() noexcept; -const char *libmagic_database_path() noexcept; +std::string_view odr_data_path() noexcept; } // namespace odr::internal::project_info diff --git a/src/odr/odr.cpp b/src/odr/odr.cpp index 915ecb768..7a19f201e 100644 --- a/src/odr/odr.cpp +++ b/src/odr/odr.cpp @@ -17,7 +17,9 @@ using odr::internal::file_type_table::Row; namespace table = odr::internal::file_type_table; } // namespace -std::string odr::version() { return internal::project_info::version(); } +std::string odr::version() { + return std::string(internal::project_info::version()); +} std::string odr::commit_hash() { return internal::git_info::commit_hash(); } diff --git a/test/src/internal/magic_test.cpp b/test/src/internal/magic_test.cpp index 9d07795f3..f3c3a95ea 100644 --- a/test/src/internal/magic_test.cpp +++ b/test/src/internal/magic_test.cpp @@ -2,7 +2,6 @@ #include #include -#include #include @@ -14,11 +13,8 @@ using namespace odr; using namespace odr::internal; using namespace odr::test; -// the mimetype comes from the open strategy, and from libmagic only in a build -// that still asks for the deprecated option. both answers are asserted, because -// the two agree on everything but `svm` - and because the open strategy branch -// used to be unreachable in every job that runs the tests, and stayed broken -// for it +// the mimetype comes from the open strategy: the signature table names the +// container, and opening it names what is inside TEST(magic, odt) { const File file(TestData::test_file_path("odr-public/odt/about.odt")); @@ -41,15 +37,8 @@ TEST(magic, svm) { const File file(TestData::test_file_path("odr-public/svm/chart-1.svm")); EXPECT_EQ(magic::file_type(*file.impl()), FileType::starview_metafile); - // the one format the two disagree on, and the one where our own table is - // right - if (project_info::has_libmagic()) { - EXPECT_EQ(magic::mimetype(file.disk_path().value(), Logger::null()), - "application/octet-stream"); - } else { - EXPECT_EQ(magic::mimetype(file.disk_path().value(), Logger::null()), - "application/x-starview-metafile"); - } + EXPECT_EQ(magic::mimetype(file.disk_path().value(), Logger::null()), + "application/x-starview-metafile"); } TEST(magic, odf) { diff --git a/test/src/test_info.cpp.in b/test/src/test_info.cpp.in index 0653221b6..58b58b604 100644 --- a/test/src/test_info.cpp.in +++ b/test/src/test_info.cpp.in @@ -2,14 +2,10 @@ namespace odr::test { -const char *info::odr_data_path() noexcept { +std::string_view info::odr_data_path() noexcept { return "${ODR_BUILD_ODR_DATA_PATH}"; } -const char *info::libmagic_database_path() noexcept { - return "${ODR_BUILD_LIBMAGIC_DATABASE_PATH}"; -} - -const char *info::odr_test_data_path() noexcept { return "${ODR_TEST_DATA_DIRECTORY}"; } +std::string_view info::odr_test_data_path() noexcept { return "${ODR_TEST_DATA_DIRECTORY}"; } } // namespace odr::test diff --git a/test/src/test_info.hpp b/test/src/test_info.hpp index 8f5d89d3b..1bdc5f238 100644 --- a/test/src/test_info.hpp +++ b/test/src/test_info.hpp @@ -1,10 +1,11 @@ #pragma once +#include + namespace odr::test::info { -const char *odr_data_path() noexcept; -const char *libmagic_database_path() noexcept; +std::string_view odr_data_path() noexcept; -const char *odr_test_data_path() noexcept; +std::string_view odr_test_data_path() noexcept; } // namespace odr::test::info diff --git a/test/src/test_util.cpp b/test/src/test_util.cpp index f9bc53007..93f557c5c 100644 --- a/test/src/test_util.cpp +++ b/test/src/test_util.cpp @@ -22,8 +22,7 @@ namespace fs = std::filesystem; namespace odr { void test::set_global_params() { - GlobalParams::set_odr_core_data_path(info::odr_data_path()); - GlobalParams::set_libmagic_database_path(info::libmagic_database_path()); + GlobalParams::set_odr_core_data_path(std::string(info::odr_data_path())); } } // namespace odr