build: add CMake options for system library linking #1761
Conversation
Fixes CCExtractor#1719 - build was failing with --enable-hardsubx due to missing tesseract library linking. Added pkg_check_modules for tesseract and leptonica in the HARDSUBX section of CMakeLists.txt. Tested with: cmake -DWITH_HARDSUBX=ON -DWITH_OCR=ON -DWITH_FFMPEG=ON
Adds USE_SYSTEM_ZLIB, USE_SYSTEM_LIBPNG, and USE_SYSTEM_FREETYPE options as foundation for issue CCExtractor#1718 - allowing system library linking instead of bundled libraries. Note: Options are OFF by default and don't change current behavior yet. Implementation of actual system library detection will follow.
src/CMakeLists.txt
Outdated
| option (WITH_FFMPEG "Build using FFmpeg demuxer and decoder" OFF) | ||
| option (WITH_OCR "Build with OCR (Optical Character Recognition) feature" OFF) | ||
| option (WITH_HARDSUBX "Build with support for burned-in subtitles" OFF) | ||
| option(USE_SYSTEM_ZLIB "Use system zlib instead of bundled" OFF) |
There was a problem hiding this comment.
The commit messages mentions USE_SYSTEM_LIBPNG, and USE_SYSTEM_FREETYPE, but I only see USE_SYSTEM_ZLIB; I assume it's still a work in progress? Thanks for your efforts addressing this!
There was a problem hiding this comment.
Thanks for pointing that out — you’re right. The options for libpng and freetype weren’t fully wired yet.
I’m cleaning up the CMake logic so each USE_SYSTEM_* flag properly switches between system and bundled sources (starting with removing the unconditional vendored freetype/libpng paths). I’ll update the PR mostly by today.
|
Fixed the CMake conditional nesting and cleaned up how the existing USE_SYSTEM_* options are handled. |
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Thanks for working on this feature! System library linking is useful for distribution packagers.
However, there are several issues that need to be addressed before this can be merged:
Issues to Fix
1. Duplicate Option Definition
USE_SYSTEM_ZLIB is defined twice (lines 8 and 13):
option(USE_SYSTEM_ZLIB "Use system zlib instead of bundled" OFF)
# ...
option (USE_SYSTEM_ZLIB "Use system zlib instead of bundled" OFF)Remove the duplicate.
2. Incomplete FreeType Implementation
USE_SYSTEM_FREETYPE option exists but the implementation is incomplete:
- Only removes
FT2_BUILD_LIBRARYdefinition - Missing
find_package(Freetype REQUIRED)call when enabled FREETYPE_SOURCEis still defined and included unconditionally
Please add the full implementation or remove the option until it's ready.
3. Misplaced Source Directories in ZLIB Block
Inside the else() block for USE_SYSTEM_ZLIB, there are unrelated source directories:
aux_source_directory(${PROJECT_SOURCE_DIR}/thirdparty/lib_hash/ SOURCEFILE)
aux_source_directory(${PROJECT_SOURCE_DIR}/thirdparty/libpng/ SOURCEFILE)These should not be conditional on zlib selection.
4. Duplicate macOS arm64 Handling
The macOS arm64 libpng handling appears twice - once at the top level and once inside the USE_SYSTEM_LIBPNG else block.
5. Formatting
Mixed indentation (spaces vs tabs). Please run through the project's formatting.
Summary
The concept is good but the implementation needs cleanup. Please address these issues and I'll be happy to approve.
|
Thanks for the detailed review — all points addressed |
3 participants
Partial fix for #1718
Adds CMake options for system library linking as foundation for full implementation:
Current behaviour: Options are OFF by default, existing bundled libraries still used
Future work: Implement actual system library detection and linking
This is phase 1 of addressing #1718 - adding the configuration framework before implementing the full system library support.