-
Notifications
You must be signed in to change notification settings - Fork 772
also build static libraries of Abseil #22805
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
also build static libraries of Abseil #22805
Conversation
|
Test report by @Flamefire |
|
Test report by @Flamefire |
|
@boegelbot please test @ jsc-zen3 |
|
@lexming: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... Details- notification for comment with ID 2830488500 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
lexming
left a comment
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.
LGTM
|
Merging, thanks @Flamefire ! |
|
@Flamefire Did you by any chance test the impact on PyTorch-bundle? What I see is that |
|
Can you add more information how to reproduce? I guess you rebuild Abseil, protobuf and PyTorch-bundle and the last one fails to build?
That is weird: How can the symbols only be there when using dynamic libraries? Then they should be in Abseil not protobuf shouldn't they? Any hint what exactly uses that symbol? Could be an issue with torchtext specifically or a missing definition making the symbol private/hidden |
|
At the moment I can't reproduce it anymore. I still see less symbols in |
Can you post those / a comparison so we can verify that the diff is OK and/or come back to this via search if it becomes an issue later. |
I'll attach the output of |
|
Those are listed as e.g. IIRC the "U" means that this is a reference to an external symbol to be provided by a shared library at runtime. So this is expected as far as I can tell: Linking the static Abseil libraries resolves the symbols during linking. What I can imagine:
when protobuf linked dynamic abseil the dependencies of X were resolved because libprotobuf pulled in libabseil at runtime. With protobuf not having a dependency on libabseil due to static linking the dependencies of X are not resolved anymore. In any case this is a bug in the build process of X, so we need to find the exact component that breaks this. |
|
Turns out it was how we patch torchtext to use our prebuild RE2. PR to fix it: And it was indeed the case that torchtext ended up linking a library that depends on Abseil without also linking the (static) Abseil libs. |
(created using
eb --new-pr)Fixes #22764
I tested that the CMake config files indeed reference the static libs instead of the shared libs so they'll be used with CMakes
find_package(Abseil)We cannot remove the shared libraries as that would break existing modules linked against the shared libraries.
Changing all EasyConfigs at once so that if anyone updates any of them to a new version the information will be present for him and on CI. The intention is that new Abseil Easyconfigs should only use the static variant. See the issue for motivation
After rebuilding protobuf I don't see any runtime dependencies on Abseil for neither protobuf nor PyTorch