Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[build] Adjust install name for Differentiation and Concurrency #34216

Merged
merged 2 commits into from Oct 12, 2020
Merged

[build] Adjust install name for Differentiation and Concurrency #34216

merged 2 commits into from Oct 12, 2020

Conversation

edymtt
Copy link
Contributor

@edymtt edymtt commented Oct 7, 2020 •
edited

This effectively reverts #31183 -- we need to match the install name convention of the other stdlib libraries.

From the review feedback:

The right way to load the stdlib & runtime libraries from a custom toolchain is to set DYLD_LIBRARY_PATH when executing the generated binary. This is how we run tests against the just-built libraries and this is how downloadable toolchain snapshots are currently configured in Xcode -- see #33178

@edymtt
[build] Adjust install name for Differentiation and Concurrency
8d65427 
This way we match the way we build other Swift libraries, to allow the
dylib to have an absolute install name and at the same time be found
when used as part of a local toolchain (e.g. apple#31183)
@edymtt
Copy link
Contributor Author

edymtt commented Oct 7, 2020

@swift-ci please smoke test

@edymtt
Copy link
Contributor Author

edymtt commented Oct 7, 2020

@swift-ci please build toolchain

@edymtt
Copy link
Contributor Author

edymtt commented Oct 7, 2020 •
edited

For the sake of completeness -- I'm not touching Differentiation_NoTgMath on purpose for now, since I need to think more about that (namely #34193)

@edymtt edymtt mentioned this pull request Oct 7, 2020
Merged
@swift-ci
Copy link
Collaborator

swift-ci commented Oct 7, 2020

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 8d65427

Install command
tar zxf swift-PR-34216-456-ubuntu16.04.tar.gz
More info

lorentey
lorentey requested changes Oct 7, 2020
View reviewed changes
Copy link
Member

@lorentey lorentey left a comment •
edited

Choose a reason for hiding this comment

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

The right way to load the stdlib & runtime libraries from the toolchain is to set DYLD_LIBRARY_PATH when executing the generated binary. This is how we run tests against the just-built libraries and this is how downloadable toolchain snapshots are currently configured in Xcode -- see #33178)

stdlib/public/Concurrency/CMakeLists.txt Outdated Show resolved Hide resolved
lorentey
lorentey reviewed Oct 7, 2020
View reviewed changes
stdlib/public/Concurrency/CMakeLists.txt
@@ -29,5 +30,4 @@ add_swift_target_library(swift_Concurrency ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES} I
${SWIFT_STANDARD_LIBRARY_SWIFT_FLAGS}
-parse-stdlib
LINK_FLAGS "${SWIFT_RUNTIME_SWIFT_LINK_FLAGS}"
DARWIN_INSTALL_NAME_DIR "@rpath"
Copy link
Member

Choose a reason for hiding this comment

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

Note: this change is right -- the install_name should be left with the default /usr/lib/swift/ prefix, matching the other stdlib libraries.

@edymtt
Copy link
Contributor Author

edymtt commented Oct 8, 2020

@swift-ci please smoke test

@edymtt
Copy link
Contributor Author

edymtt commented Oct 8, 2020

@swift-ci please build toolchain

@edymtt edymtt requested a review from lorentey October 8, 2020 18:57
@swift-ci
Copy link
Collaborator

swift-ci commented Oct 8, 2020

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 48a298f

Install command
tar zxf swift-PR-34216-464-ubuntu16.04.tar.gz
More info

@edymtt
Copy link
Contributor Author

edymtt commented Oct 8, 2020

@swift-ci please smoke test

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 9, 2020

macOS Toolchain
Download Toolchain
Git Sha - 48a298f

Install command
tar -zxf swift-PR-34216-723-osx.tar.gz --directory ~/

lorentey
lorentey approved these changes Oct 9, 2020
View reviewed changes
Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@edymtt
Copy link
Contributor Author

edymtt commented Oct 9, 2020

@swift-ci please smoke test Linux

1 similar comment
@edymtt
Copy link
Contributor Author

edymtt commented Oct 9, 2020

@swift-ci please smoke test Linux

@edymtt
Copy link
Contributor Author

edymtt commented Oct 9, 2020

Tested that the generated toolchain restores the intended behaviour

> DYLD_LIBRARY_PATH=$(dirname $(xcrun --toolchain PR34216 --find swift))/../lib/swift/macosx xcrun --toolchain PR34216 swift run
Hello, world!
12.0

>  xcrun --toolchain PR34216 swift run                                                                                          
dyld: Library not loaded: /usr/lib/swift/libswift_Differentiation.dylib
  Referenced from: /Users/emiotto/Downloads/swiftpm_test/.build/x86_64-apple-macosx/debug/swiftpm_test
  Reason: image not found
[1]    24454 abort      xcrun --toolchain PR34216 swift run

@edymtt edymtt merged commit be38df8 into apple:main Oct 12, 2020
@dan-zheng dan-zheng mentioned this pull request Dec 30, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd approved these changes

@lorentey lorentey lorentey approved these changes

@rxwei rxwei rxwei approved these changes

@DougGregor DougGregor DougGregor approved these changes

@shahmishal shahmishal shahmishal approved these changes

@dan-zheng dan-zheng Awaiting requested review from dan-zheng

@marcrasi marcrasi Awaiting requested review from marcrasi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@edymtt @swift-ci @compnerd @lorentey @rxwei @DougGregor @shahmishal