Skip adding -lstatic to libtest and libstd on Darwin #1620
Conversation
gferon
force-pushed
the
fix-libtest-libstd-runtime-error-macos
branch
from
f8251fa to
149c67d
Compare
|
Looks like the CI failure is unrelated to this change 🤔 |
There was a problem hiding this comment.
It seems a little weird that we'd express "don't dynamically link libstd" by saying "don't statically link libstd", but, if it seems to work (which CI shows it does), that seems better than the status quo...
Would it be possible to get a couple of tests test showing the desired end-effect here?
- Our existing binaries all successfully run on CI at the moment - do you have an example that fails without this patch?
- Could we add a
sh_testwhich runsotool -Lon a producedrust_binaryand shows that it doesn't dynamically link the libraries we don't expect to be dynamically linked?
|
You're right, this would be much better if the issue was documented and a test would prevent any regression. I'll remove the patch on our codebase and try to come up with a minimal non-working example ASAP! |
|
@illicitonion since I'm currently working on upstreaming all of our custom patches, I unburried this one from the past. I added a test to demonstrate the issue, and I can confirm the test fails without the patch and passes otherwise.
I added a |
gferon
changed the title
| #!/bin/sh | ||
| set -e | ||
|
|
||
| $1 |
There was a problem hiding this comment.
Could you instead run otool -L "$1" and verify (by grep or whatever) that it doesn't contain "libstd" or "libtest"?
I want us to be sure that we're actually statically linking stuff, rather than setting up rpaths or whatever so that in the bazel-bin tree the binary happens to work but if you moved it somewhere else or copied it to another machine it would fail to.
You can use target_compatible_with to make sure this test only runs on darwin where otool will be present.
There was a problem hiding this comment.
Good idea! Just pushed a new commit.
|
Thanks! |
This is one way to fix bazelbuild#1573, which I have a feeling can be wrong. The binaries produced with this patch contain all required symbols and are working fine on macOS.
This is one way to fix #1573, which I have a feeling can be wrong. The binaries produced with this patch contain all required symbols and are working fine on macOS.