Repository navigation
Add real OpenBLAS/LAPACK acceleration to Ubuntu Docker builds; bump Analyzer.Testing - #2021
Conversation
Follow-ups from the Renovate Dependency Dashboard (#1894): - Update Microsoft.CodeAnalysis.CSharp.Analyzer.Testing to the latest 1.1.4 - Update the ubuntu base image tag from noble to resolute in the two OpenCV/OpenCvSharp build Dockerfiles Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates OpenCV Docker build images and runtime library installation steps, fixes nested vector copy helpers for empty inputs and byte sizing, and bumps a NuGet test dependency in the analyzer test project. ChangesPackaging updates
Nested vector copy helpers
Analyzer test dependency update
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
♻️ Duplicate comments (1)
packaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfile (1)
5-5: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSame cross-LTS base image concern as the slim variant.
This mirrors the issue in the sibling
-slimDockerfile:ubuntu:noble→ubuntu:resoluteis a two-release jump (24.04 → 26.04), not a routine tag bump, and conflicts with the directory'subuntu24-*naming plus the PR's stated intent of only reacting to Renovate's flagged updates. The-D CMAKE_ASM_COMPILER=""workaround and extensive apt dependency list (libtbb-dev,libgtk-3-dev,libavcodec-dev, etc.) were presumably validated on noble; a resolute rebuild could shift compiler/toolchain behavior or package versions in ways that invalidate that workaround or introduce new build/link issues.See the companion comment on the
-slimDockerfile for full rationale; this is a single root-cause concern across both files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfile` at line 5, The Dockerfile base image was bumped from ubuntu:noble to ubuntu:resolute in the opencv-builder stage, which is a cross-LTS change rather than a routine update. Revert the FROM image in this Dockerfile to the original noble-based Ubuntu 24.x line so it stays aligned with the ubuntu24-* package naming and the validated OpenCV build/toolchain assumptions. Keep the rest of the builder setup, including the CMAKE_ASM_COMPILER workaround and apt dependency list, unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfile`:
- Line 5: The Dockerfile base image was bumped from ubuntu:noble to
ubuntu:resolute in the opencv-builder stage, which is a cross-LTS change rather
than a routine update. Revert the FROM image in this Dockerfile to the original
noble-based Ubuntu 24.x line so it stays aligned with the ubuntu24-* package
naming and the validated OpenCV build/toolchain assumptions. Keep the rest of
the builder setup, including the CMAKE_ASM_COMPILER workaround and apt
dependency list, unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53a1804a-76e5-4301-a7fd-51f11cdbbfa4
📒 Files selected for processing (3)
packaging/docker/ubuntu24-dotnet10-opencv5.0.0-slim/Dockerfilepackaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfiletest/OpenCvSharp.Analyzers.Tests/OpenCvSharp.Analyzers.Tests.csproj
libatlas-base-dev is gone on ubuntu:resolute (Debian/Ubuntu have been phasing out ATLAS in favor of OpenBLAS), which is what broke the Docker Test CI job after the ubuntu:resolute bump. Rather than just swapping in a same-effect replacement, switch to libopenblas-dev + liblapacke-dev. Unlike vcpkg's openblas port (used on Windows/macOS/manylinux), Ubuntu's apt package is built with Fortran/LAPACK support, so OpenCV's cmake actually detects and links a real LAPACK implementation instead of falling back to its built-in implementation - verified locally: Lapack: YES (LAPACK/Generic /usr/lib/x86_64-linux-gnu/libopenblas.so ...) Applied to both the full and slim Dockerfiles, since both build modules (calib, ml, ptcloud, ...) that rely on LAPACK for SVD/solve. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
shimat
changed the title
…stages CI failed after the OpenBLAS switch: libOpenCvSharpExtern.so now needs libopenblas.so.0/liblapacke.so.3 at runtime, but the test-native/ test-dotnet/final*/ stages only copy /usr/lib wholesale from the (ubuntu:resolute-based) opencv-builder stage. That copy misses the update-alternatives symlinks (owned by /etc/alternatives, not /usr/lib) that make the versioned .so names resolvable, so the linker/loader can't find them: /usr/bin/ld: warning: libopenblas.so.0, needed by libOpenCvSharpExtern.so, not found Fix: apt-get install the runtime packages (libopenblas0, liblapacke - note the runtime package has no version suffix, unlike the -dev package) fresh in each downstream stage, which is based on mcr.microsoft.com/dotnet/sdk|aspnet:10.0-noble. This mirrors how gcc is already installed fresh in test-native rather than copied. Verified locally: a .so built on ubuntu:resolute against libopenblas-dev/liblapacke-dev, copied onto a noble-based image with just libopenblas0/liblapacke installed, links and runs correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfile (1)
5-5: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAlign the builder and runtime Ubuntu bases.
opencv-builderusesubuntu:resolutewhile the test and final stages stay on...:10.0-noble; copyinglibOpenCvSharpExtern.soand related artifacts across that 26.04→24.04 boundary can leave the final images missing the glibc symbols those binaries were linked against. Move the runtime stages toresoluteor keep the builder onnoble.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfile` at line 5, The Ubuntu base mismatch in the Dockerfile can break compatibility between the built OpenCV artifacts and the runtime images. Make the builder stage and the test/final stages use the same Ubuntu release by either switching the runtime stages from the current 10.0-noble base to resolute or changing opencv-builder to noble; keep the existing stage names and image copy flow intact while aligning the base used by the Dockerfile stages that produce and consume libOpenCvSharpExtern.so.packaging/docker/ubuntu24-dotnet10-opencv5.0.0-slim/Dockerfile (1)
5-5: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep the builder and runtime on the same Ubuntu release.
opencv-buildernow usesubuntu:resolute, while the runtime stages stay onnoble; copying those native artifacts into an older runtime can triggerGLIBC_x.xx/GLIBCXX_x.xxloader failures. If resolute is required, bump the runtime images too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packaging/docker/ubuntu24-dotnet10-opencv5.0.0-slim/Dockerfile` at line 5, Keep the OpenCV builder and runtime on the same Ubuntu release: align the ubuntu:resolute base used by opencv-builder with the runtime stages, or downgrade opencv-builder to match the existing noble images. Update the Dockerfile stages consistently so native artifacts built in the builder are copied into a runtime with the same libc/libstdc++ baseline.
🧹 Nitpick comments (2)
packaging/docker/ubuntu24-dotnet10-opencv5.0.0-slim/Dockerfile (1)
95-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRuntime OpenBLAS/LAPACKE install duplicated across four stages.
The same
apt-get update && apt-get -y install --no-install-recommends ... libopenblas0 liblapacke && rm -rf /var/lib/apt/lists/*pattern is repeated intest-native,final-sdk, andfinal. Given Dockerfile stage isolation this duplication is largely unavoidable without restructuring, but consider introducing a shared intermediate runtime-base stage (FROM mcr.microsoft.com/dotnet/sdk:10.0-noble AS runtime-depswith the install, thenFROM runtime-deps AS test-native, etc.) to keep the package list defined once.Also applies to: 114-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packaging/docker/ubuntu24-dotnet10-opencv5.0.0-slim/Dockerfile` around lines 95 - 99, The OpenBLAS/LAPACKE runtime install is duplicated in multiple Docker stages, so consolidate the repeated package setup into a shared intermediate stage. Create a reusable runtime-deps stage in the Dockerfile that performs the apt-get install for gcc, libopenblas0, and liblapacke once, then base test-native, final-sdk, and final on that stage to keep the package list defined in one place while preserving stage isolation.packaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfile (1)
113-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame duplication pattern as the slim Dockerfile.
The
libopenblas0/liblapackeinstall-then-copy pattern repeats acrosstest-native,test-dotnet,final-sdk, andfinal. As with the slim variant, a shared intermediate stage could reduce this duplication, but the current per-stage approach is functionally correct given the commit-message rationale on/etc/alternativessymlinks.Also applies to: 132-134, 146-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfile` around lines 113 - 117, The `libopenblas0`/`liblapacke` install step is duplicated across the `test-native`, `test-dotnet`, `final-sdk`, and `final` stages in the Dockerfile. Refactor this repeated logic into a shared intermediate stage or common reusable setup block, then have those stages inherit from it so the `apt-get install` sequence is defined once while preserving the existing `/etc/alternatives` symlink handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packaging/docker/ubuntu24-dotnet10-opencv5.0.0-slim/Dockerfile`:
- Line 5: Keep the OpenCV builder and runtime on the same Ubuntu release: align
the ubuntu:resolute base used by opencv-builder with the runtime stages, or
downgrade opencv-builder to match the existing noble images. Update the
Dockerfile stages consistently so native artifacts built in the builder are
copied into a runtime with the same libc/libstdc++ baseline.
In `@packaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfile`:
- Line 5: The Ubuntu base mismatch in the Dockerfile can break compatibility
between the built OpenCV artifacts and the runtime images. Make the builder
stage and the test/final stages use the same Ubuntu release by either switching
the runtime stages from the current 10.0-noble base to resolute or changing
opencv-builder to noble; keep the existing stage names and image copy flow
intact while aligning the base used by the Dockerfile stages that produce and
consume libOpenCvSharpExtern.so.
---
Nitpick comments:
In `@packaging/docker/ubuntu24-dotnet10-opencv5.0.0-slim/Dockerfile`:
- Around line 95-99: The OpenBLAS/LAPACKE runtime install is duplicated in
multiple Docker stages, so consolidate the repeated package setup into a shared
intermediate stage. Create a reusable runtime-deps stage in the Dockerfile that
performs the apt-get install for gcc, libopenblas0, and liblapacke once, then
base test-native, final-sdk, and final on that stage to keep the package list
defined in one place while preserving stage isolation.
In `@packaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfile`:
- Around line 113-117: The `libopenblas0`/`liblapacke` install step is
duplicated across the `test-native`, `test-dotnet`, `final-sdk`, and `final`
stages in the Dockerfile. Refactor this repeated logic into a shared
intermediate stage or common reusable setup block, then have those stages
inherit from it so the `apt-get install` sequence is defined once while
preserving the existing `/etc/alternatives` symlink handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06f03080-38de-449a-92eb-ad70886954f9
📒 Files selected for processing (2)
packaging/docker/ubuntu24-dotnet10-opencv5.0.0-slim/Dockerfilepackaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfile
…llel_for_ Identified via --blame-crash: OpenCvSharp.Tests.Stitching.FeaturesMatcherTest. ApplyMultipleImageFeatures crashed the test-dotnet stage's dotnet test run with exit code 134 (SIGABRT), 60+s into the run with no managed exception - the signature of a native-level abort, not a test failure. BestOf2NearestMatcher matches image pairs across threads via OpenCV's own parallel_for_, and each match calls into LAPACK/SVD for homography estimation. OpenBLAS's own pthread-based internal thread pool is not safe to call concurrently from an already-multithreaded host application without capping its thread count - a well-documented OpenBLAS/nested-parallelism pitfall. Built-in unaccelerated LAPACK (used before this PR's OpenBLAS switch) never hit this because it has no internal threading of its own. Fix: set OPENBLAS_NUM_THREADS=1 wherever the built library actually runs (test-native, test-dotnet, final-sdk, final in both Dockerfiles). OpenCV's own parallel_for_ still runs multi-threaded; only OpenBLAS's internal per-call threading is disabled, which is a net win anyway since the individual matrices here are small enough that OpenBLAS's own threading would just add overhead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Root-caused via gdb backtrace: the earlier "OpenBLAS threading" theory for the SIGABRT crashes in the Ubuntu Docker full test suite was wrong. The actual crash is: std::__glibcxx_assert_fail -> std::vector<unsigned char>::operator[] -> vector_vector_uchar_copy (libOpenCvSharpExtern.so) vector_vector_uchar_copy used `sizeof(int) * elem.size()` to compute the copy length for a `std::vector<uchar>` element - a copy-paste leftover from vector_vector_int_copy. This copied 4x too many bytes into the destination buffer on every non-empty call, corrupting heap memory (silently on most allocators, until the corruption is later detected elsewhere - which explains why the crash appeared to happen at wildly different, seemingly test-order-dependent points across runs). Separately, `&elem[0]` on an empty inner vector is undefined behavior; Ubuntu resolute's newer libstdc++ (built with _GLIBCXX_ASSERTIONS) is what turned this from silent UB into a hard abort() - this bug has likely existed since VectorOfVectorByte was introduced and is unrelated to the OpenBLAS/Docker changes in this PR; it was only surfaced by them. Fix: use sizeof(uchar) instead of sizeof(int), and skip empty elements before taking &elem[0] in all three copy functions (uchar/int/double) that share this pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Follow-ups from the Renovate Dependency Dashboard (#1894):
Microsoft.CodeAnalysis.CSharp.Analyzer.Testing1.1.3 → 1.1.4 inOpenCvSharp.Analyzers.Testsubuntubase image tag fromnobletoresolutein the two OpenCV/OpenCvSharp build Dockerfiles (packaging/docker/ubuntu24-dotnet10-opencv5.0.0and-slim)The
resolutebump initially broke CI:libatlas-base-devno longer exists on Ubuntu 26.04 (Debian/Ubuntu have been phasing ATLAS out in favor of OpenBLAS for years;noblestill carried it as a transitional package).Rather than reaching for an equivalent no-op replacement, this swaps in
libopenblas-dev+liblapacke-devfor real acceleration:openblasport (used on Windows/macOS/manylinux, which builds with-DBUILD_WITHOUT_LAPACK=ON -DNOFORTRAN=ONand therefore can't satisfy OpenCV's LAPACK detection, which requires bothcblas.handlapacke.h), Ubuntu's apt package ships a full Fortran/LAPACK-enabled build.ubuntu:resolute:calib,ml,ptcloud, ...).Test plan
dotnet testonOpenCvSharp.Analyzers.Tests— all 23 tests pass with the bumped packageubuntu:resolutefor both Dockerfileslibopenblas-dev/liblapacke-devheaders resolve via default include paths and realcblas_dgemm/LAPACKE_dgetrfcalls link and execute correctly on bothubuntu:nobleandubuntu:resolutecmakeconfigure (not full build) against the real build options picks up OpenBLAS asLAPACK: YESwith no extra cmake flagsDocker Testworkflow (full end-to-end image builds + native/.NET smoke tests) — relying on CI for the full build since compiling OpenCV from source locally is too slow to verify end-to-end in this sessionCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by CodeRabbit