-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[build] Remove runpath from build host from shared ICU libraries on linux #64365
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
Conversation
@swift-ci please test |
Strangely, the flag got chomped down to
I hate these bash escaping rules, let's try the three-slash variant. |
@mhjacobson, if you're still up, please run the CI on this. |
@swift-ci please smoke test |
Sigh, that didn't work either. |
Found the magic incantation that appears to work, after testing it locally. @bnbarham, please run the CI again and I will add an integration test next, to make sure this doesn't regress again. |
@swift-ci please test |
OK, CI passed now that the rpath was set properly, and I checked the build log to make sure it's set to the right value. This is ready for review and merge, @bnbarham. |
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.
If this should stay a shared library this seems reasonable enough to me. Though it seems like the only thing that uses this now is foundation? In which case... maybe we should just statically link it?
I am not sure about the considerations that go into making such a change. For one, ICU uses different licenses than Foundation, which might then require some legal vetting. I'm just trying to clean up all these CI runpaths leaking into the final toolchain, not going to attempt a bigger change like that, though it may make sense one day. |
I suggested the
--enable-rpath
flag in #40340 to add$ORIGIN
more than a year ago, but I didn't realize it also adds thelibdir
to the runpath:See if this fixes it by using the more traditional
LDFLAGS
instead.@bnbarham, please run the CI on this and I'll check that it works.