-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[abseil_cpp_internal] Fix static linking patch commands #21961
Conversation
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.
Manual test technique:
$ ls bazel-bin/_solib_k8/*abseil*
That command will find some shared library files in a built master workspace. After this patch (and possibly a "bazel clean"), it should not find any shared library files.
Reviewable status: needs platform reviewer assigned, needs at least one assigned reviewer (waiting on @rpoyner-tri)
0eb6ec0
to
49b99d1
Compare
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.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least one assigned reviewer (waiting on @rpoyner-tri)
a discussion (no related file):
After this patch, many mold
versions should successfully link drake. I've tried 1.03 (apt install mold
on jammy), and 2.34 (latest built from source).
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.
+@jwnimmer-tri for review.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)
tools/workspace/abseil_cpp_internal/repository.bzl
line 17 at r2 (raw file):
], patch_cmds = [ # Back-fill a linkopts key missing in the upstream build system.
nit I think edit this would be substantially clearer as a patch file instead of sed. Also, everywhere else in abseil sets copts = ABSL_DEFAULT_COPTS
and linkopts = ABSL_DEFAULT_LINKOPTS
, so I imagine that's how our patch file should spell it also?
It also seems like a the kind of possible oversight that upstream would appreciate knowing about, i.e., we should propose that patch to upstream -- although given abseil/abseil-cpp#1003 (comment) maybe they would decline.
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform)
tools/workspace/abseil_cpp_internal/repository.bzl
line 17 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit I think edit this would be substantially clearer as a patch file instead of sed. Also, everywhere else in abseil sets
copts = ABSL_DEFAULT_COPTS
andlinkopts = ABSL_DEFAULT_LINKOPTS
, so I imagine that's how our patch file should spell it also?It also seems like a the kind of possible oversight that upstream would appreciate knowing about, i.e., we should propose that patch to upstream -- although given abseil/abseil-cpp#1003 (comment) maybe they would decline.
Am I guaranteed that commands are applied after patch files?
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.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)
tools/workspace/abseil_cpp_internal/repository.bzl
line 17 at r2 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
Am I guaranteed that commands are applied after patch files?
When I skimmed the patching implementation, it seemed like "yes" to me. If it doesn't work, we can revisit.
49b99d1
to
bcae7d8
Compare
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform)
tools/workspace/abseil_cpp_internal/repository.bzl
line 17 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
When I skimmed the patching implementation, it seemed like "yes" to me. If it doesn't work, we can revisit.
That file doesn't import ABSL_DEFAULT_LINKOPTS
, and I don't need it, so I left that out of the patch.
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.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions
tools/workspace/abseil_cpp_internal/patches/civil_time_linkopts.patch
line 3 at r3 (raw file):
This patch could be upstreamed.
nit This comment leaves the point murky. This specific patch should not be upstreamed. What could be upstreamed is a patch that imported both of the ..._DEFAULT_...
constants and using them.
Amend the patch commands to convert all of the cc_library() rules in the upstream's bazel files to use "linkstatic = 1". This prevents generation of "accidental" shared libraries, which in turn were causing link failures with more modern linkers (.i.e., ld.mold).
bcae7d8
to
9fe33aa
Compare
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.
Reviewable status: 1 unresolved discussion
tools/workspace/abseil_cpp_internal/patches/civil_time_linkopts.patch
line 3 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This patch could be upstreamed.
nit This comment leaves the point murky. This specific patch should not be upstreamed. What could be upstreamed is a patch that imported both of the
..._DEFAULT_...
constants and using them.
Sure. I'm still not certain I know why this particular build file is different from all the others.
What I really think is that keying on arguments is the wrong seam. We should probably be reimplementing cc_library(). I had hopes for the documented cc_static_library()
, but alas, it seems to have been abandoned. https://bazel.build/reference/be/c-cpp#cc_static_library
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.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion
tools/workspace/abseil_cpp_internal/patches/civil_time_linkopts.patch
line 3 at r3 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
Sure. I'm still not certain I know why this particular build file is different from all the others.
What I really think is that keying on arguments is the wrong seam. We should probably be reimplementing cc_library(). I had hopes for the documented
cc_static_library()
, but alas, it seems to have been abandoned. https://bazel.build/reference/be/c-cpp#cc_static_library
This BUILD file is different because the code is vendored (as noted here: abseil/abseil-cpp#1003 (comment)).
Longer term, we could submit an upstream ABSL_DEFAULT_LINKSTATIC
to match how they handle default copts and linkopts. Their architecture is to use named constants to configure cc_library defaults. Using a wrapper cc_library reimplementation would go against how they've chosen to manage their project.
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.
Reviewable status: 1 unresolved discussion
tools/workspace/abseil_cpp_internal/patches/civil_time_linkopts.patch
line 3 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This BUILD file is different because the code is vendored (as noted here: abseil/abseil-cpp#1003 (comment)).
Longer term, we could submit an upstream
ABSL_DEFAULT_LINKSTATIC
to match how they handle default copts and linkopts. Their architecture is to use named constants to configure cc_library defaults. Using a wrapper cc_library reimplementation would go against how they've chosen to manage their project.
Oh, I'm not talking about upstreaming a cc_library hook...
Amend the patch commands to convert all of the cc_library() rules in the upstream's bazel files to use "linkstatic = 1". This prevents generation of "accidental" shared libraries, which in turn were causing link failures with more modern linkers (.i.e., ld.mold).
This change is