Skip to content

Add -no-toolchain-stdlib-rpath flag. #27207

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

Merged
merged 3 commits into from
Sep 21, 2019

Conversation

dan-zheng
Copy link
Contributor

Add -no-toolchain-stdlib-rpath flag: the negative version of
-toolchain-stdlib-rpath.

Make -no-toolchain-stdlib-rpath be the default: use /usr/lib/swift as
default RPATH on Darwin platforms instead of toolchain standard library.


Adapted from #27206.

tensorflow branch requires the opposite default (use toolchain standard
library as RPATH) because some stdlib modules like TensorFlow do not exist in
/usr/lib/swift.

Add `-no-toolchain-stdlib-rpath` flag: the negative version of
`-toolchain-stdlib-rpath`.

Make `-no-toolchain-stdlib-rpath` be the default: use `/usr/lib/swift` as
default RPATH on Darwin platforms instead of toolchain standard library.

Adapted from swiftlang#27206.

tensorflow branch requires the opposite default (use toolchain standard
library as RPATH) because some stdlib modules like TensorFlow do not exist in
`/usr/lib/swift`.
@dan-zheng dan-zheng requested a review from beccadax September 17, 2019 00:06
@dan-zheng
Copy link
Contributor Author

Patch suggested by @brentdax, to minimize the diff between master and tensorflow branches.
I'm not sure how to add tests, suggestions welcome.

@beccadax
Copy link
Contributor

beccadax commented Sep 17, 2019

To test this, I would add test cases to test/Driver/linker-rpath.swift which duplicate the existing x86_64-apple-macosx10.14 and x86_64-apple-macosx10.15 test cases but pass -no-toolchain-stdlib-rpath. Your change in the TensorFlow branch should probably require you to modify the originals of these test cases; just leave the copies testing the original results.

@dan-zheng
Copy link
Contributor Author

To test this, I would add test cases to test/Driver/linker-rpath.swift which duplicate the existing x86_64-apple-macosx10.14 and x86_64-apple-macosx10.15 test cases but pass -no-toolchain-stdlib-rpath. Your change in the TensorFlow branch should probably require you to modify the originals of these test cases; just leave the copies testing the original results.

Done in 9a1b3f2.

I added -no-toolchain-stdlib-rpath test cases for x86_64-apple-macosx10.9 and x86_64-apple-macosx10.15, following existing tests in the same file for -no-stdlib-rpath and -toolchain-stdlib-rpath.

I added mirror tests in the tensorflow branch patch: 1ad2aa4.

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems all right to me. I'm not quite sure what the future of -toolchain-stdlib-rpath mode is on Apple platforms at all, but it seems like the same things we need for testing the master stdlib also apply to using the TensorFlow stdlib.

(A bunch of this might get reorganized too as part of the "generalize cross-compilation and SDK layouts" work described on the forums. But for now this is good.)

Copy link
Contributor

@beccadax beccadax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should settle the help text question, but otherwise this LGTM.

@dan-zheng dan-zheng force-pushed the no-toolchain-stdlib-rpath-flag branch from e68c24d to adce3fb Compare September 20, 2019 01:00
@dan-zheng
Copy link
Contributor Author

This seems all right to me. I'm not quite sure what the future of -toolchain-stdlib-rpath mode is on Apple platforms at all, but it seems like the same things we need for testing the master stdlib also apply to using the TensorFlow stdlib.

(A bunch of this might get reorganized too as part of the "generalize cross-compilation and SDK layouts" work described on the forums. But for now this is good.)

Thank you for the helpful context.


Thanks to reviewers!

@dan-zheng
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0596b20

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 0596b20

@dan-zheng dan-zheng requested a review from beccadax September 20, 2019 22:15
@dan-zheng dan-zheng merged commit 3bbc504 into swiftlang:master Sep 21, 2019
@dan-zheng dan-zheng deleted the no-toolchain-stdlib-rpath-flag branch September 21, 2019 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants