opencv4: protobuf_21 -> protobuf#419335
Merged
philiptaron merged 2 commits intoNixOS:stagingfrom Jul 17, 2025
Merged
Conversation
Signed-off-by: Connor Baker <ConnorBaker01@gmail.com>
github-actions
bot
added
10.rebuild-linux: 501+
Contributor
Author
|
Old Details## `nixpkgs-review` resultGenerated using Command:
|
Contributor
Author
|
Old Details## `nixpkgs-review` resultGenerated using Command:
|
Contributor
Author
|
Old Details## `nixpkgs-review` resultGenerated using Command:
|
Contributor
Author
|
CC @autra @SuperSandro2000 @phanirithvij since you've all previously discussed OpenCV/protobuf in one way or another, what needs to be done to validate whether a protobuf bump is acceptable? |
Member
|
As far as I remember we need to check the configure log and/or the cmake file to see what is happening there. At least the cmake file should contain some version constraint or condition and will then log that the feature was disabled or skipped or similar. |
Contributor
Author
|
Watching the build more closely, I did see That shouldn't be the default language version for these compilers and I don't see anything in the derivation setting it. @SuperSandro2000 would you happen to know if the language standard for C/C++ is set somewhere by default by Nix or its toolchain wrappers? |
Member
|
see also #386339 |
Member
|
It seems like opencv sets a default in many places opencv/opencv#23752 I think the compilers have an inbuilt default, I don't think we set that by default but it could also be burried somewhere deep. Maybe see #218671 and #255979 |
fixes NixOS#386327 (cherry picked from commit fbced4e)
ConnorBaker
force-pushed
the
feat/opencv-protobuf-bump
branch
from
def0c7c to
3eb8b87
Compare
Contributor
Author
|
Contributor
Author
|
Contributor
Author
|
I ran |
Contributor
Imo, apart from a successful build, a check that upstream supports it and some testing. Relevant thread (where we decided not to bump protobuf), which in summary was "bumping CXX is risky, let's not do it now": #375774 (comment) |
Contributor
Author
I'm currently running the passthru tests to check for breakage. EDIT: Added the results of running the tests above.
I saw that -- I'm not sure what's risky beyond the possibility of link-time failures (which would cause build failures, not runtime failures). @SuperSandro2000 any ideas on what to look for specifically in terms of breakages typically caused by bumping the language standard with which a package builds? The only thing I could think of as having caused issues previously was the C++ ABI. |
Member
|
I think most issues should be catched at compile time, so if most things continue to build afterwards we should be fine. |
Contributor
Author
|
The CUDA tests weren't working, so I did a general cleanup of the tests here:#422208. |
philiptaron
approved these changes
Jul 17, 2025
Contributor
philiptaron
left a comment
philiptaron
left a comment
There was a problem hiding this comment.
Builds for me and a quick survey of dependent packages indicate they build and test, too. Let's give it a go.
Contributor
|
Successfully created backport PR for |
1 task
github-actions
bot
added
the
8.has: port to stable
13 tasks
3 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.
Updates the OpenCV derivation expression to use the default version of
protobufand updates the language standard used by OpenCV to C++17.protobuf_29was briefly made the default (#370155), but reverted in c312f6f due to language standard requirements (C++17) and concerns about possible linking breakage as a result (#375774 (comment)).#386339 raises the language standard to C++17 -- this
appears unnecessary given the default language standard for GCC and Clang has been C++17 since versions 11 and 16, respectively (https://gcc.gnu.org/projects/cxx-status.html#cxx17 and https://clang.llvm.org/cxx_status.html)is necessary given OpenCV sets the default language standard in several places (#419335 (comment)).Things done
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.