opencv: switch to openexr_3#375774
Merged
SuperSandro2000 merged 2 commits intoNixOS:masterfrom Apr 25, 2025
Merged
Conversation
github-actions
bot
added
10.rebuild-darwin: 101-500
13 tasks
Contributor
Author
|
Contributor
Author
|
This is ready for review, although I need to check the issue with all these python 3.13 packages. |
Contributor
Author
Contributor
Author
|
I do have failed tests though: |
Contributor
Author
|
Ok those tests also fails on master. |
Contributor
Author
|
@basvandijk this is ready now. Please review :-) |
autra
commented
Jan 26, 2025
SuperSandro2000
added
the
1.severity: security
SuperSandro2000
approved these changes
Feb 4, 2025
autra
force-pushed
the
opencv_switch_to_openexr_3
branch
from
b116fce to
fe7bf1b
Compare
Contributor
Author
|
Rebased onto latest master. |
Contributor
Author
|
@SuperSandro2000 thanks for the approve! Do you think this could be merged? |
Contributor
Author
|
@basvandijk @SuperSandro2000 can we merge? (it'll unblock part of #367406 ) |
Member
|
Do you have any idea why the following tests fail according to ofborg in opencv4? |
Contributor
Author
|
It's opencv/opencv#25674, fixed in 4.10. I've opened #382967 to update opencv to the latest version (4.11). I'll rebase this one once it is merged. |
autra
force-pushed
the
opencv_switch_to_openexr_3
branch
from
fe7bf1b to
056bbd4
Compare
Contributor
Author
|
All right, the tests should be fixed on staging. Also, I've rebased this one on staging, since the number of packages is above 500 apparently. |
github-actions
bot
added
10.rebuild-linux: 501+
Contributor
Author
|
|
What's the implication of I was under the impression this causes opencv to use a vendorded library and we didn't want to do that (it was delibrately removed from opencv awhile ago). I was also under the impression that we don't need protobuf as a dependency if we enable Do we know when we want to use vendored libraries or not on nix? I am unaware of the sort of policy on that. |
Contributor
Author
It enable or disable protobuf support entirely. I had build or test problem without setting it. By reading the derivation, I thought that the original intention was to have the protobuf support by default so I switched this boolean to true. From my understanding, what causes opencv to use a vendored protobuf is the BUILD_PROTOBUF variable (set to false in our derivation). I do also think we shouldn't use vendored libs whenever possible indeed. |
Contributor
Author
For protobuf. |
Contributor
Author
|
@ZeroEcks btw I'm quite suprised that homebrew didn't seem to have this boolean set. I'm really quite sure it is needed, if my understanding is correct. |
|
You are right, I confused |
Contributor
Author
|
I still have test failure. Master also have test failure, but not the same 😮 @SuperSandro2000 WDYT? Should we care? |
47 tasks
Contributor
Author
|
Contributor
Author
|
Note: all of the failed packages above are also failing on hydra. |
Contributor
Author
|
So after digging, only the Gui tests are failing, and only with nix-build. If I do the This is ready to merge. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5346 |
wegank
added
the
2.status: merge conflict
Contributor
There was a problem hiding this comment.
It seems to need a rebase on master, and you might also be able to cherry-pick the commit from #375774 instead of downgrading protobuf as used by opencv?
See https://github.com/opencv/opencv/blob/4.11.0/cmake/OpenCVFindProtobuf.cmake#L81 Only protobuf <= 22 will work with the current CXX standard version.
autra
force-pushed
the
opencv_switch_to_openexr_3
branch
from
623bb41 to
c312f6f
Compare
ofborg
bot
removed
the
2.status: merge conflict
SuperSandro2000
approved these changes
Apr 21, 2025
Member
|
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
@eclairevoyant #375774 is this MR, what MR did you want to link? |
Contributor
Member
|
I do not know what the impacts of that are to linking programs. If we cause many regressions now, I would rather use an older protobuf. |
Contributor
Author
|
wegank
added
the
12.approvals: 1
Member
|
I randomly tested 5 packages and they where all pre-existing failures or unfree. |
Contributor
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-24.11
git worktree add -d .worktree/backport-375774-to-release-24.11 origin/release-24.11
cd .worktree/backport-375774-to-release-24.11
git switch --create backport-375774-to-release-24.11
git cherry-pick -x 2b2a13782b6842091e943a62f6d7f7116d89cc12 c312f6f8187eefb9c860bc54d1e508490f20c94a |
13 tasks
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
6 participants
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.
Things done
Follow up of #367406 : opencv v2 is EOL and has security vulnerabilities.
This switches the build of opencv to openexr v3.
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.