-
-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
python310Packages.onnx: switch to protobuf 4.x #255077
Conversation
The problem seems to be that although Onnx's We could try to use
(I think C++ 14 isn't sufficient for our version of abseil btw, so using C++ 17 here) |
9c00560
to
ddee4ee
Compare
@acairncross thanks a lot for your idea, it worked flawlessly! |
Result of 6 packages marked as broken and skipped:
65 packages failed to build:
|
alright, the failure I get on linux it seems is
|
I don't know what the cause of the linker error is, but I have found by experimenting that setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems related to abseil/abseil-cpp#634
I've checked a bit and protobuf itself seems to be compiled with c++14 |
ddee4ee
to
ea847b4
Compare
@andersk sorry to bring you into this, (feel free to ignore if you don't have time). |
ea847b4
to
422e73b
Compare
Result of 6 packages marked as broken and skipped:
65 packages failed to build:
|
I've also tried not setting the cxx_standard in abseil to see, but it fails even earlier. |
422e73b
to
a28fafd
Compare
Result of 6 packages marked as broken and skipped:
65 packages failed to build:
|
in this latest try I've tried setting c++ to 17 in onnx and removing the cxx standard in abseil, but that didn't work on linux. |
That's strange, I've just successfully built |
a28fafd
to
c6cb6fe
Compare
Result of 6 packages marked as broken and skipped:
16 packages failed to build:
49 packages built:
|
@acairncross by adding cxxstandard to 17 to abseil, compilation succeeds. @jcwchen let me know if you are against it, but I vote for proceeding with c++17. (btw in the build failures, the mmcv failure is expected, it's missing a pybind11 dependency). |
Hm, it's not ideal that we don't really know why it works, but good that it does work. I've got a bit more of an idea of what was going wrong with
|
Hey thanks for doing a lot of the digging ! If i read the previous PRs correctly, patching abseil was rejected by the maintainers. So we cant go the suggested route right ? What do you think i add a comment describing what we went through and we keep it on the 17 version ? |
Yeah, going with the C++ 17 version is fine. I think part of the reason we need to compile with C++ 17 is because abseil is compiled with C++ 17 by default, which causes it to use C++ 17 features. I don't really understand why we aren't needing to specify that protobuf also be compiled with C++ 17, or at least 14 despite the protobuf docs saying it's necessary. Incidentally, I guess what went wrong with the attempt to compile both onnx and abseil (using |
The protobuf idea is an interesting one, let me go ask the protobuf maintainers, I'll open a PR and will tag you. Are you okay to merge the current PR then ? (I just want to make sure). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've gotten to the bottom of this and this approach looks good to me 😃
c6cb6fe
to
8e89290
Compare
@acairncross I've taken care of your comments, let me know if you have more. |
Result of 22 packages marked as broken and skipped:
51 packages failed to build:
|
1 similar comment
Result of 22 packages marked as broken and skipped:
51 packages failed to build:
|
my reviews failed twice because of disk space. |
Description of changes
I'm trying to switch the protobuf version used in onnx to 4.x
a new dependency is required (abseil-cpp), and there are several warning messages about needing c++14.
I'm stuck though, during the compilation of onnx, c++11 is used, I don't know how to switch the version to c++14, I've tried adding the c++14 flags, and I can see in the build the CXX flags, however I still get warnings that c++11 is used.
@benxiao this should fix the mmcv build, however, I'm not sure how to get this unstuck.
@acairncross maybe you know more about this ?
if anyone is interested, I think it's just a matter of using c++14 here.
I had a small discussion upstream to talk what it would take for an upgrade.
onnx/onnx#5579 (comment)
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)