Linux: portable manylinux build, vcpkg for Linux, slim profile expansion#1840
Open
Linux: portable manylinux build, vcpkg for Linux, slim profile expansion#1840
Conversation
- Add manylinux_2_28 CI workflow producing portable linux-x64 NuGet packages - Replace hand-built static deps with vcpkg (image libs + Tesseract/Leptonica); keep FFmpeg-only build_static_deps.sh for FFmpeg - Add cmake/triplets/x64-linux-static.cmake; remove platform=windows from vcpkg.json - WITH_GTK=OFF for all Linux builds; extend Tesseract static linkage to Linux - Merge ubuntu.yml + ubuntu-slim.yml into a single two-job workflow - Delete ubuntu-slim.yml workflow - Deprecate ubuntu.22.04/24.04 NuGet packages; update README and nuget READMEs - Expand slim profile: add ml, video, stitching, barcode (no external deps) - Update README: top-level statement sample code, remove UWP/ubuntu table rows
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a manylinux_2_28 CI workflow and supporting tooling to build, cache, test, and package full and slim OpenCvSharpExtern (with static FFmpeg), removes the ubuntu-slim workflow, deprecates Ubuntu-specific NuGet runtimes in favor of portable linux-x64 packages, and expands the slim OpenCV module list. (47 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor GitHubActions as "GitHub Actions"
participant Container as "manylinux_2_28"
participant VCPKG as "vcpkg (submodule)"
participant FFmpeg as "FFmpeg build script"
participant OpenCV as "OpenCV build"
participant Extern as "OpenCvSharpExtern build"
participant Artifacts as "Artifact storage"
participant TestJob as "Test job"
participant NuGetJob as "NuGet packaging job"
GitHubActions->>Container: start manylinux workflow
Container->>VCPKG: bootstrap & restore/install packages
Container->>FFmpeg: run build_static_deps.sh (configure & build)
FFmpeg-->>Container: install static FFmpeg (/opt/ffmpeg)
Container->>OpenCV: configure & build full/slim (with vcpkg)
OpenCV-->>Container: install/OpenCV artifacts
Container->>Extern: build OpenCvSharpExtern (full & slim)
Extern-->>Artifacts: upload `.so` artifacts
GitHubActions->>TestJob: trigger test job (downloads artifacts)
TestJob->>Artifacts: download artifacts & run tests
GitHubActions->>NuGetJob: trigger packaging (on main)
NuGetJob->>Artifacts: download artifacts & create/publish NuGet packages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
cmake/opencv_build_options_slim.cmake (1)
1-1: Update comment to reflect broader Linux usage.The comment states "Ubuntu only" but this file is now also used by the manylinux workflow for portable Linux builds.
📝 Suggested comment update
-# CMake initial cache for OpenCV slim build (Ubuntu only). +# CMake initial cache for OpenCV slim build (Linux).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/opencv_build_options_slim.cmake` at line 1, The file header comment currently says "Ubuntu only"; update that top comment in cmake/opencv_build_options_slim.cmake to reflect broader Linux usage (e.g., change "Ubuntu only" to "Linux (including manylinux)" or similar) so it no longer inaccurately restricts the file to Ubuntu—edit the initial comment line to mention general Linux or manylinux support..github/workflows/manylinux.yml (2)
140-150: Quote${GITHUB_WORKSPACE}in shell scripts to prevent word splitting.Static analysis (shellcheck SC2086) flags multiple unquoted variable expansions. While
GITHUB_WORKSPACEis unlikely to contain spaces in practice, quoting is a defensive best practice.🛡️ Suggested fix for lines 141-150
- name: Verify no unresolved dependencies (full) run: | echo "=== ldd output ===" - ldd ${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so + ldd "${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so" echo "=== Checking for 'not found' ===" - if ldd ${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so | grep -q "not found"; then + if ldd "${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so" | grep -q "not found"; then echo "ERROR: unresolved dependencies found" - ldd ${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so | grep "not found" + ldd "${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so" | grep "not found" exit 1 fi echo "OK: all dependencies resolved"Similar quoting should be applied to other
${GITHUB_WORKSPACE}usages in lines 54-57, 97-116, 129-138, 184-192, 204-215, 218-227, 265-271, 274-279, and 318-333.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/manylinux.yml around lines 140 - 150, The shell step "Verify no unresolved dependencies (full)" uses unquoted ${GITHUB_WORKSPACE} in ldd invocations which can cause word-splitting; update every ldd and similar shell expansion in this workflow to quote the variable like "${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so" and other uses of ${GITHUB_WORKSPACE} elsewhere in the manylinux.yml run steps so all occurrences are safely quoted (e.g., in the steps that reference ldd, cp, or echo with GITHUB_WORKSPACE).
265-271: Test step installs library to/usr/lib/but also setsLD_LIBRARY_PATH=.Line 270 copies the library to
/usr/lib/withsudo, and line 271 also setsLD_LIBRARY_PATH=.. The system path install may be unnecessary since the test already relies onLD_LIBRARY_PATH.💡 Consider removing redundant system install
- name: Test (full) run: | - cd ${GITHUB_WORKSPACE}/test/OpenCvSharp.Tests + cd "${GITHUB_WORKSPACE}/test/OpenCvSharp.Tests" dotnet build -c Release -f net8.0 - cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so bin/Release/net8.0/ - cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so ./ - sudo cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so /usr/lib/ + cp "${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so" bin/Release/net8.0/ + cp "${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so" ./ LD_LIBRARY_PATH=. dotnet test OpenCvSharp.Tests.csproj -c Release -f net8.0 --runtime linux-x64 --logger "trx;LogFileName=test-results.trx" < /dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/manylinux.yml around lines 265 - 271, The workflow currently both installs the native library to the system with the command "sudo cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so /usr/lib/" and also relies on LD_LIBRARY_PATH=. for tests; remove the redundant system install by deleting the "sudo cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so /usr/lib/" step so the tests rely solely on LD_LIBRARY_PATH=. (If you prefer system-wide install instead, remove the "LD_LIBRARY_PATH=." prefix from the "LD_LIBRARY_PATH=. dotnet test ..." line and keep the sudo cp line.)docker/manylinux/build_static_deps.sh (1)
33-35: Consider using PGP signature verification instead of SHA256 checksums.While the supply chain risk concern is valid, FFmpeg does not publish SHA256 checksums for modern releases. Instead, the project provides PGP-signed
.ascfiles and officially recommends verifying releases with GPG:curl -s https://ffmpeg.org/ffmpeg-devel.asc | gpg --import curl -fsSL "https://ffmpeg.org/releases/ffmpeg-${FFMPEG_VERSION}.tar.xz" -o ffmpeg.tar.xz curl -fsSL "https://ffmpeg.org/releases/ffmpeg-${FFMPEG_VERSION}.tar.xz.asc" -o ffmpeg.tar.xz.asc gpg --verify ffmpeg.tar.xz.asc ffmpeg.tar.xz tar xf ffmpeg.tar.xzThis approach aligns with FFmpeg's official verification guidance and requires no manual hash maintenance across version bumps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/manylinux/build_static_deps.sh` around lines 33 - 35, Replace the current unchecked download of FFmpeg in build_static_deps.sh (the curl/tar lines using FFMPEG_VERSION) with GPG-based verification: import FFmpeg's public key (e.g., fetch ffmpeg-devel.asc and gpg --import), download both ffmpeg-${FFMPEG_VERSION}.tar.xz and its corresponding .asc signature, run gpg --verify on the .asc against the tar.xz and abort the script if verification fails before running tar xf; update the block around the curl -fsSL "https://ffmpeg.org/releases/ffmpeg-${FFMPEG_VERSION}.tar.xz" -o ffmpeg.tar.xz lines to perform these steps and ensure failures are handled with set -e or explicit exit.nuget/README.runtime.md (1)
24-31: Slim profile table may not belong in runtime README per coding guidelines.The coding guidelines specify that "Slim profile module coverage table (managed only)" should only appear in the managed package README. Consider removing lines 24-31 or keeping only a brief mention that slim packages have a reduced module set with a link to the managed README for details.
As per coding guidelines: "NuGet README files must consist of only: Overview and supported platforms, Installation instructions (Quick Start), Requirements, Slim profile module coverage table (managed only)..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuget/README.runtime.md` around lines 24 - 31, The "Slim Profile" table (the markdown table under the "Slim Profile" heading) should be removed from the runtime README because the slim profile module coverage table belongs only in the managed package README; instead replace it with a single-sentence note that slim packages include a reduced set of OpenCV modules and add a link to the managed README for full module coverage details (update the "Slim Profile" heading and content accordingly, referencing the existing "Slim Profile" section/table to locate the change)..github/workflows/ubuntu.yml (3)
110-117: Quote${GITHUB_WORKSPACE}for shell best practices.Static analysis flagged unquoted variable expansions (SC2086). While
GITHUB_WORKSPACEis controlled by GitHub Actions and won't contain spaces, quoting is a defensive practice.♻️ Proposed fix
- name: Test run: | - cd ${GITHUB_WORKSPACE}/test/OpenCvSharp.Tests + cd "${GITHUB_WORKSPACE}/test/OpenCvSharp.Tests" dotnet build -c Release -f net8.0 - cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so bin/Release/net8.0/ - cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so ./ - sudo cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so /usr/lib/ + cp "${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so" bin/Release/net8.0/ + cp "${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so" ./ + sudo cp "${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so" /usr/lib/ LD_LIBRARY_PATH=. dotnet test OpenCvSharp.Tests.csproj -c Release -f net8.0 --runtime linux-x64 --logger "trx;LogFileName=test-results.trx" < /dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ubuntu.yml around lines 110 - 117, Shell variable expansions in the workflow steps (commands like cd ${GITHUB_WORKSPACE}/test/OpenCvSharp.Tests, cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so, sudo cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so and LD_LIBRARY_PATH=. dotnet test ...) are unquoted; update each occurrence to use quoted expansions (e.g. "${GITHUB_WORKSPACE}" or "${GITHUB_WORKSPACE}/path") to prevent word-splitting and satisfy SC2086 while leaving behavior unchanged.
182-194: Quote variable expansions in slim build step.Same SC2086 pattern as the full profile steps.
♻️ Proposed fix
- name: Build OpenCvSharpExtern (slim) run: | cmake \ -S src \ -B src/build-slim \ -D CMAKE_BUILD_TYPE=Release \ - -D CMAKE_PREFIX_PATH=${GITHUB_WORKSPACE}/opencv_artifacts_slim \ + -D CMAKE_PREFIX_PATH="${GITHUB_WORKSPACE}/opencv_artifacts_slim" \ -D NO_CONTRIB=ON \ -D NO_VIDEOIO=ON \ -D NO_HIGHGUI=ON \ -D NO_DNN=ON cmake --build src/build-slim -j - cp src/build-slim/OpenCvSharpExtern/libOpenCvSharpExtern.so ${GITHUB_WORKSPACE}/nuget/ + cp src/build-slim/OpenCvSharpExtern/libOpenCvSharpExtern.so "${GITHUB_WORKSPACE}/nuget/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ubuntu.yml around lines 182 - 194, The slim build step "Build OpenCvSharpExtern (slim)" uses unquoted variable expansions (SC2086); update the cmake invocation to quote the CMAKE_PREFIX_PATH value (e.g., -D CMAKE_PREFIX_PATH="${GITHUB_WORKSPACE}/opencv_artifacts_slim") and quote the cp destination and source paths so expansions like ${GITHUB_WORKSPACE}/nuget and the libOpenCvSharpExtern.so path are wrapped in quotes (e.g., cp "src/build-slim/OpenCvSharpExtern/libOpenCvSharpExtern.so" "${GITHUB_WORKSPACE}/nuget/") to avoid word-splitting and globbing issues.
196-207: Quote variables in slim check/smoke test steps.♻️ Proposed fix
- name: Check OpenCvSharpExtern dependencies (slim) run: | - cd ${GITHUB_WORKSPACE}/nuget/ + cd "${GITHUB_WORKSPACE}/nuget/" ldd -r libOpenCvSharpExtern.so ! ldd libOpenCvSharpExtern.so | grep -q "not found" - name: Smoke test OpenCvSharpExtern (slim) run: | - cd ${GITHUB_WORKSPACE}/nuget/ + cd "${GITHUB_WORKSPACE}/nuget/" echo -ne "#include <stdio.h> \n int core_Mat_sizeof(); int main(){ int i = core_Mat_sizeof(); printf(\"sizeof(Mat) = %d\", i); return 0; }" > test.c gcc -I./ -L./ test.c -o test -lOpenCvSharpExtern LD_LIBRARY_PATH=. ./test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ubuntu.yml around lines 196 - 207, The workflow steps use unquoted shell variables and an unquoted echo string which can break if paths contain spaces; update the "Check OpenCvSharpExtern dependencies (slim)" and "Smoke test OpenCvSharpExtern (slim)" steps to quote ${GITHUB_WORKSPACE} as "$GITHUB_WORKSPACE" everywhere, and change the test source write to use a single-quoted or printf form (e.g. printf '%s' '...c source...' > test.c) so the C source isn't subject to shell expansion; ensure other uses like LD_LIBRARY_PATH are either exported or quoted when referenced (e.g. LD_LIBRARY_PATH=. ./test remains but any variable should be "$VAR") to make the steps robust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/manylinux.yml:
- Around line 18-23: The workflow comment incorrectly references a non-existent
path "/opt/deps"; update the comment to mention the actual cached dependency
locations used in the script (e.g., "/opt/vcpkg/installed" for vcpkg packages
and "/opt/ffmpeg" for FFmpeg) so the documentation matches the implementation;
search for the comment block in manylinux.yml and replace "/opt/deps" with the
correct paths (or a brief list: "/opt/vcpkg/installed, /opt/ffmpeg") to ensure
accuracy.
In `@cmake/triplets/x64-linux-static.cmake`:
- Around line 1-5: Add the missing triplet variable VCPKG_CMAKE_SYSTEM_NAME and
set it to Linux so vcpkg/CMake sees the correct target system; update the
triplet (which currently sets VCPKG_TARGET_ARCHITECTURE, VCPKG_LIBRARY_LINKAGE,
and VCPKG_BUILD_TYPE) to also define VCPKG_CMAKE_SYSTEM_NAME Linux to ensure
ports that inspect CMAKE_SYSTEM_NAME treat this as a Linux build.
---
Nitpick comments:
In @.github/workflows/manylinux.yml:
- Around line 140-150: The shell step "Verify no unresolved dependencies (full)"
uses unquoted ${GITHUB_WORKSPACE} in ldd invocations which can cause
word-splitting; update every ldd and similar shell expansion in this workflow to
quote the variable like "${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so" and other
uses of ${GITHUB_WORKSPACE} elsewhere in the manylinux.yml run steps so all
occurrences are safely quoted (e.g., in the steps that reference ldd, cp, or
echo with GITHUB_WORKSPACE).
- Around line 265-271: The workflow currently both installs the native library
to the system with the command "sudo cp
${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so /usr/lib/" and also relies on
LD_LIBRARY_PATH=. for tests; remove the redundant system install by deleting the
"sudo cp ${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so /usr/lib/" step so
the tests rely solely on LD_LIBRARY_PATH=. (If you prefer system-wide install
instead, remove the "LD_LIBRARY_PATH=." prefix from the "LD_LIBRARY_PATH=.
dotnet test ..." line and keep the sudo cp line.)
In @.github/workflows/ubuntu.yml:
- Around line 110-117: Shell variable expansions in the workflow steps (commands
like cd ${GITHUB_WORKSPACE}/test/OpenCvSharp.Tests, cp
${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so, sudo cp
${GITHUB_WORKSPACE}/nuget/libOpenCvSharpExtern.so and LD_LIBRARY_PATH=. dotnet
test ...) are unquoted; update each occurrence to use quoted expansions (e.g.
"${GITHUB_WORKSPACE}" or "${GITHUB_WORKSPACE}/path") to prevent word-splitting
and satisfy SC2086 while leaving behavior unchanged.
- Around line 182-194: The slim build step "Build OpenCvSharpExtern (slim)" uses
unquoted variable expansions (SC2086); update the cmake invocation to quote the
CMAKE_PREFIX_PATH value (e.g., -D
CMAKE_PREFIX_PATH="${GITHUB_WORKSPACE}/opencv_artifacts_slim") and quote the cp
destination and source paths so expansions like ${GITHUB_WORKSPACE}/nuget and
the libOpenCvSharpExtern.so path are wrapped in quotes (e.g., cp
"src/build-slim/OpenCvSharpExtern/libOpenCvSharpExtern.so"
"${GITHUB_WORKSPACE}/nuget/") to avoid word-splitting and globbing issues.
- Around line 196-207: The workflow steps use unquoted shell variables and an
unquoted echo string which can break if paths contain spaces; update the "Check
OpenCvSharpExtern dependencies (slim)" and "Smoke test OpenCvSharpExtern (slim)"
steps to quote ${GITHUB_WORKSPACE} as "$GITHUB_WORKSPACE" everywhere, and change
the test source write to use a single-quoted or printf form (e.g. printf '%s'
'...c source...' > test.c) so the C source isn't subject to shell expansion;
ensure other uses like LD_LIBRARY_PATH are either exported or quoted when
referenced (e.g. LD_LIBRARY_PATH=. ./test remains but any variable should be
"$VAR") to make the steps robust.
In `@cmake/opencv_build_options_slim.cmake`:
- Line 1: The file header comment currently says "Ubuntu only"; update that top
comment in cmake/opencv_build_options_slim.cmake to reflect broader Linux usage
(e.g., change "Ubuntu only" to "Linux (including manylinux)" or similar) so it
no longer inaccurately restricts the file to Ubuntu—edit the initial comment
line to mention general Linux or manylinux support.
In `@docker/manylinux/build_static_deps.sh`:
- Around line 33-35: Replace the current unchecked download of FFmpeg in
build_static_deps.sh (the curl/tar lines using FFMPEG_VERSION) with GPG-based
verification: import FFmpeg's public key (e.g., fetch ffmpeg-devel.asc and gpg
--import), download both ffmpeg-${FFMPEG_VERSION}.tar.xz and its corresponding
.asc signature, run gpg --verify on the .asc against the tar.xz and abort the
script if verification fails before running tar xf; update the block around the
curl -fsSL "https://ffmpeg.org/releases/ffmpeg-${FFMPEG_VERSION}.tar.xz" -o
ffmpeg.tar.xz lines to perform these steps and ensure failures are handled with
set -e or explicit exit.
In `@nuget/README.runtime.md`:
- Around line 24-31: The "Slim Profile" table (the markdown table under the
"Slim Profile" heading) should be removed from the runtime README because the
slim profile module coverage table belongs only in the managed package README;
instead replace it with a single-sentence note that slim packages include a
reduced set of OpenCV modules and add a link to the managed README for full
module coverage details (update the "Slim Profile" heading and content
accordingly, referencing the existing "Slim Profile" section/table to locate the
change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80696e24-2428-4432-93b3-3f8afd2a2f19
📒 Files selected for processing (17)
.github/workflows/manylinux.yml.github/workflows/ubuntu-slim.yml.github/workflows/ubuntu.yml.github/workflows/windows.ymlREADME.mdcmake/opencv_build_options.cmakecmake/opencv_build_options_slim.cmakecmake/triplets/x64-linux-static.cmakedocker/manylinux/build_static_deps.shnuget/OpenCvSharp4.official.runtime.ubuntu.22.04-x64.csprojnuget/OpenCvSharp4.official.runtime.ubuntu.22.04-x64.slim.csprojnuget/OpenCvSharp4.official.runtime.ubuntu.24.04-x64.csprojnuget/OpenCvSharp4.official.runtime.ubuntu.24.04-x64.slim.csprojnuget/README.managed.mdnuget/README.runtime.mdsrc/OpenCvSharpExtern/CMakeLists.txtvcpkg.json
💤 Files with no reviewable changes (2)
- .github/workflows/windows.yml
- .github/workflows/ubuntu-slim.yml
…ux only, bust slim OpenCV cache key
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/manylinux.yml (1)
174-181: Make the manylinux slim cache key track the slim build inputs.This key never changes when
cmake/opencv_build_options_slim.cmakeor the slim-specific CMake flags in this workflow change, so the job can silently reuse an old OpenCV archive with the wrong module set. The Ubuntu slim job already hashes the slim options file; manylinux should track the same input.Suggested cache-key update
- key: opencv-${{ env.OPENCV_VERSION }}-manylinux2_28-slim + key: opencv-${{ env.OPENCV_VERSION }}-manylinux2_28-slim-${{ hashFiles('cmake/opencv_build_options_slim.cmake', '.github/workflows/manylinux.yml') }}Also applies to: 195-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/manylinux.yml around lines 174 - 181, The slim OpenCV cache key (key: opencv-${{ env.OPENCV_VERSION }}-manylinux2_28-slim) in the "Restore OpenCV cache (slim)" step (id: opencv-cache) doesn't include the slim-build inputs; update the key to incorporate the hash of the slim options file (e.g. append -${{ hashFiles('cmake/opencv_build_options_slim.cmake') }}) so changes to cmake/opencv_build_options_slim.cmake (and any slim-specific workflow flags) invalidate the cache; make the identical change to the corresponding slim cache save/restore pair elsewhere in the workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/manylinux.yml:
- Around line 141-151: Update the manylinux workflow step that inspects
libOpenCvSharpExtern.so to fail when any unexpected DT_NEEDED entries appear as
well as when unresolved "not found" dependencies are present: run a command
(e.g., readelf -d or ldd -v) to extract DT_NEEDED entries for
libOpenCvSharpExtern.so, compare them against an allowlist of expected libraries
(allowlist should include only the known portable deps), and if any required
library is not in the allowlist exit nonzero; keep the existing
unresolved-dependency grep ("not found") check as a second guard. Ensure you
apply the same change to the slim check (the other block around lines 218-228)
so both artifact checks enforce the allowlist.
- Around line 183-214: The slim OpenCV build step ("Build OpenCV (slim)") is
missing the OPENCV_EXTRA_MODULES_PATH so contrib modules like barcode (declared
in cmake/opencv_build_options_slim.cmake's BUILD_LIST) are not found; update the
cmake invocation used in that step to pass -D
OPENCV_EXTRA_MODULES_PATH=${GITHUB_WORKSPACE}/opencv_contrib/modules and also
add the same -D OPENCV_EXTRA_MODULES_PATH to the corresponding slim OpenCV build
in the ubuntu.yml; keep the existing flags (e.g. -D CMAKE_INSTALL_PREFIX=..., -D
WITH_TBB=OFF) and do not remove NO_CONTRIB in the OpenCvSharpExtern cmake call.
- Around line 40-43: Replace the depth-1 clone of vcpkg with a clone followed by
an explicit checkout of the pinned baseline commit read from vcpkg.json: after
cloning https://github.com/microsoft/vcpkg.git to /opt/vcpkg and running
/opt/vcpkg/bootstrap-vcpkg.sh, parse the builtin-baseline value from the
repository's vcpkg.json and run git checkout <builtin-baseline> (or equivalent)
in /opt/vcpkg so the checked-out commit matches the pinned baseline declared in
vcpkg.json and ensures reproducible installs.
---
Nitpick comments:
In @.github/workflows/manylinux.yml:
- Around line 174-181: The slim OpenCV cache key (key: opencv-${{
env.OPENCV_VERSION }}-manylinux2_28-slim) in the "Restore OpenCV cache (slim)"
step (id: opencv-cache) doesn't include the slim-build inputs; update the key to
incorporate the hash of the slim options file (e.g. append -${{
hashFiles('cmake/opencv_build_options_slim.cmake') }}) so changes to
cmake/opencv_build_options_slim.cmake (and any slim-specific workflow flags)
invalidate the cache; make the identical change to the corresponding slim cache
save/restore pair elsewhere in the workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14d3ec67-e37a-43c4-9b92-dbcfc8e34f17
📒 Files selected for processing (4)
.github/workflows/manylinux.yml.github/workflows/ubuntu.ymlcmake/opencv_build_options.cmakecmake/triplets/x64-linux-static.cmake
🚧 Files skipped from review as they are similar to previous changes (2)
- cmake/opencv_build_options.cmake
- cmake/triplets/x64-linux-static.cmake
There was a problem hiding this comment.
♻️ Duplicate comments (3)
.github/workflows/manylinux.yml (3)
141-151:⚠️ Potential issue | 🟠 MajorReject unexpected shared libraries in both portability checks.
Both checks only fail on
not found. That still passes if the.soaccidentally links to distro-specific libraries that happen to exist in the manylinux container. Gate these steps on an allowlist ofDT_NEEDEDentries as well, then keep the unresolved-dependency grep as a second guard.Suggested hardening
- - name: Verify no unresolved dependencies (full) + - name: Verify portable dependency set (full) run: | - echo "=== ldd output ===" - ldd ${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so - echo "=== Checking for 'not found' ===" - if ldd ${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so | grep -q "not found"; then + so=${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so + allowed='^(lib(stdc\+\+|gcc_s|m|c|pthread|dl|rt)\.so(\..*)?)$' + bad=$(readelf -d "$so" | awk '/NEEDED/ {gsub(/\[|\]/, "", $5); print $5}' | grep -Ev "$allowed" || true) + if [ -n "$bad" ]; then + echo "ERROR: unexpected dynamic dependencies:" + echo "$bad" + exit 1 + fi + if ldd "$so" | grep -q "not found"; then echo "ERROR: unresolved dependencies found" - ldd ${GITHUB_WORKSPACE}/libOpenCvSharpExtern.so | grep "not found" + ldd "$so" | grep "not found" exit 1 fiApply the same pattern to the slim check too.
Also applies to: 218-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/manylinux.yml around lines 141 - 151, The portability check only verifies "not found" from ldd, allowing unexpected distro-specific shared libraries to slip through; modify the step that runs ldd on libOpenCvSharpExtern.so to also extract DT_NEEDED entries (e.g., via readelf -d or ldd parsing), compare them against an allowlist of expected names, fail if any unexpected DT_NEEDED are present, and then keep the existing grep -q "not found" unresolved-dependencies guard; apply the same allowlist+unresolved check pattern to the slim check block as well so both portability jobs reject unexpected shared libraries.
183-191:⚠️ Potential issue | 🟠 MajorPass
OPENCV_EXTRA_MODULES_PATHin the slim OpenCV configure step.The full build does this on Line 102, but the slim configure step does not. If
cmake/opencv_build_options_slim.cmakenow requestsbarcode, OpenCV will not find that contrib module without the extra-modules path.#!/bin/bash set -euo pipefail echo "=== Slim workflow configure step ===" rg -n -C2 'Build OpenCV \(slim\)|OPENCV_EXTRA_MODULES_PATH|opencv_build_options_slim' .github/workflows/manylinux.yml echo echo "=== Slim build list entries mentioning barcode ===" rg -n -C2 'BUILD_LIST|barcode' cmake/opencv_build_options_slim.cmake🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/manylinux.yml around lines 183 - 191, The slim OpenCV configure step "Build OpenCV (slim)" omits OPENCV_EXTRA_MODULES_PATH, so contrib modules like barcode declared in cmake/opencv_build_options_slim.cmake won't be found; update the cmake invocation inside that step to pass the same OPENCV_EXTRA_MODULES_PATH environment variable used in the full build (i.e., add -D OPENCV_EXTRA_MODULES_PATH=${GITHUB_WORKSPACE}/opencv_contrib/modules or the existing variable name used elsewhere) so the slim configure can locate contrib modules referenced by cmake/opencv_build_options_slim.cmake.
40-43:⚠️ Potential issue | 🟠 MajorPin vcpkg to
builtin-baselineinstead of cloning a moving HEAD.
vcpkg.jsonalready declares the exact baseline commit, but Line 42 still shallow-clones whateverHEADis at runtime. Once upstream moves past that commit, cache misses stop being reproducible and can fail because the pinned baseline is not present in the shallow clone.#!/bin/bash set -euo pipefail baseline=$(python - <<'PY' import json with open("vcpkg.json", "r", encoding="utf-8") as f: print(json.load(f)["builtin-baseline"]) PY ) echo "=== Workflow step ===" rg -n -C1 'git clone .*vcpkg|checkout.*VCPKG_BASELINE' .github/workflows/manylinux.yml echo echo "=== Baseline vs upstream HEAD ===" head=$(git ls-remote https://github.com/microsoft/vcpkg.git HEAD | awk '{print $1}') printf 'builtin-baseline: %s\nremote HEAD: %s\n' "$baseline" "$head" echo echo "=== Does a depth-1 clone contain the pinned baseline? ===" tmpdir=$(mktemp -d) trap 'rm -rf "$tmpdir"' EXIT git clone --filter=blob:none --depth=1 https://github.com/microsoft/vcpkg.git "$tmpdir/vcpkg" >/dev/null 2>&1 if git -C "$tmpdir/vcpkg" cat-file -e "${baseline}^{commit}" 2>/dev/null; then echo "baseline commit is present in the shallow clone" else echo "baseline commit is missing from the shallow clone" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/manylinux.yml around lines 40 - 43, The workflow clones vcpkg at moving HEAD which can miss the pinned commit in vcpkg.json; update the "Bootstrap vcpkg" step in .github/workflows/manylinux.yml so the repository is checked out at the exact builtin-baseline commit instead of a depth-1 HEAD. Read the commit from vcpkg.json into an env var (e.g. VCPKG_BASELINE) in the workflow, clone the repo to /opt/vcpkg, then fetch and checkout that specific commit (or fetch the single commit by hash) before running /opt/vcpkg/bootstrap-vcpkg.sh so the pinned baseline referenced in vcpkg.json is guaranteed present; adjust the step that currently runs the git clone and bootstrap commands (step name "Bootstrap vcpkg") to perform this deterministic checkout.
🧹 Nitpick comments (1)
.github/workflows/manylinux.yml (1)
174-181: Key the slim OpenCV cache on the slim build configuration.The Line 181/202 key only includes
OPENCV_VERSION, so edits tocmake/opencv_build_options_slim.cmakecan restore an older slim build and skip recompiling. That is especially risky in this PR because the slim module list changed.Suggested cache-key update
- key: opencv-${{ env.OPENCV_VERSION }}-manylinux2_28-slim + key: opencv-${{ env.OPENCV_VERSION }}-manylinux2_28-slim-v${{ env.DEPS_VERSION }}-${{ hashFiles('cmake/opencv_build_options_slim.cmake') }}Apply the same change in the save step.
Also applies to: 195-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/manylinux.yml around lines 174 - 181, The slim OpenCV cache key ("opencv-${{ env.OPENCV_VERSION }}-manylinux2_28-slim") used in the "Restore OpenCV cache (slim)" step (and the corresponding save step) is missing the slim build configuration, so changes to cmake/opencv_build_options_slim.cmake can incorrectly hit the cache; update both the restore and save steps' key to incorporate a unique identifier for that config (for example append a hash or the file contents placeholder for cmake/opencv_build_options_slim.cmake) so the cache key changes whenever that file is edited.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/manylinux.yml:
- Around line 141-151: The portability check only verifies "not found" from ldd,
allowing unexpected distro-specific shared libraries to slip through; modify the
step that runs ldd on libOpenCvSharpExtern.so to also extract DT_NEEDED entries
(e.g., via readelf -d or ldd parsing), compare them against an allowlist of
expected names, fail if any unexpected DT_NEEDED are present, and then keep the
existing grep -q "not found" unresolved-dependencies guard; apply the same
allowlist+unresolved check pattern to the slim check block as well so both
portability jobs reject unexpected shared libraries.
- Around line 183-191: The slim OpenCV configure step "Build OpenCV (slim)"
omits OPENCV_EXTRA_MODULES_PATH, so contrib modules like barcode declared in
cmake/opencv_build_options_slim.cmake won't be found; update the cmake
invocation inside that step to pass the same OPENCV_EXTRA_MODULES_PATH
environment variable used in the full build (i.e., add -D
OPENCV_EXTRA_MODULES_PATH=${GITHUB_WORKSPACE}/opencv_contrib/modules or the
existing variable name used elsewhere) so the slim configure can locate contrib
modules referenced by cmake/opencv_build_options_slim.cmake.
- Around line 40-43: The workflow clones vcpkg at moving HEAD which can miss the
pinned commit in vcpkg.json; update the "Bootstrap vcpkg" step in
.github/workflows/manylinux.yml so the repository is checked out at the exact
builtin-baseline commit instead of a depth-1 HEAD. Read the commit from
vcpkg.json into an env var (e.g. VCPKG_BASELINE) in the workflow, clone the repo
to /opt/vcpkg, then fetch and checkout that specific commit (or fetch the single
commit by hash) before running /opt/vcpkg/bootstrap-vcpkg.sh so the pinned
baseline referenced in vcpkg.json is guaranteed present; adjust the step that
currently runs the git clone and bootstrap commands (step name "Bootstrap
vcpkg") to perform this deterministic checkout.
---
Nitpick comments:
In @.github/workflows/manylinux.yml:
- Around line 174-181: The slim OpenCV cache key ("opencv-${{ env.OPENCV_VERSION
}}-manylinux2_28-slim") used in the "Restore OpenCV cache (slim)" step (and the
corresponding save step) is missing the slim build configuration, so changes to
cmake/opencv_build_options_slim.cmake can incorrectly hit the cache; update both
the restore and save steps' key to incorporate a unique identifier for that
config (for example append a hash or the file contents placeholder for
cmake/opencv_build_options_slim.cmake) so the cache key changes whenever that
file is edited.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5feb8f49-64dd-4de2-be4a-0980cf22f9a9
📒 Files selected for processing (1)
.github/workflows/manylinux.yml
…sdata + system tesseract CLI test)
…ns empty OCR result)
…o bust cache, add nm check
…h not resolved correctly by static Tesseract)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by CodeRabbit
New Features
Deprecations
Documentation
CI / Packaging