Skip to content

Add swiftpm-xctest-helper rpath on macOS. #2694

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

Closed
wants to merge 1 commit into from

Conversation

dan-zheng
Copy link
Contributor

@dan-zheng dan-zheng commented Apr 16, 2020

Add an extra rpath for swiftpm-xctest-helper on macOS:
@executable_path/../../../lib/swift/macosx.

This should fix SR-12599 and SR-12600, not yet verified:

$ swift test --filter foo
error: signalled(6): /Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2020-04-14-a.xctoolchain/usr/libexec/swift/pm/swiftpm-xctest-helper /Users/danielzheng/swift-tf/Package/.build/x86_64-apple-macosx/debug/PackagePackageTests.xctest /var/folders/m_/6f7q8zfs3n9fr0c_4gy8840m00hc_q/T/TemporaryFile.B8gUFG output:
    dyld: Library not loaded: @rpath/XCTest.framework/Versions/A/XCTest
      Referenced from: /Library/Developer/Toolchains/swift-DEVELOPMENT-SNAPSHOT-2020-04-14-a.xctoolchain/usr/libexec/swift/pm/swiftpm-xctest-helper
      Reason: image not found

Building Swift toolchains now to verify the fix.
Edit: this patch doesn't fix the issues in its current form.


This idea was suggested by @abertelrud in #2692 (comment) as a more focused fix for SR-12599 and SR-12600.

Add an extra rpath for swiftpm-xctest-helper on macOS:
`@executable_path/../../../lib/swift/macosx`.

This should fix `swift test --filter ...`.
@dan-zheng dan-zheng changed the title [bootstrap] Add swiftpm-xctest-helper rpath on macOS. Add swiftpm-xctest-helper rpath on macOS. Apr 16, 2020
@dan-zheng
Copy link
Contributor Author

@swift-ci Please smoke test

@dan-zheng
Copy link
Contributor Author

This PR did not fix SR-12599 or SR-12600, so I'm converting it back to a draft.

$ otool -l swiftpm-xctest-helper | grep LC_RPATH -A2
          cmd LC_RPATH
      cmdsize 168
         path swift/swift-nightly-install/Library/Developer/Toolchains/swift-tensorflow-DEVELOPMENT-2020-04-17-a.xctoolchain/usr/lib/swift/macosx (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk/usr/lib/swift (offset 12)
--
          cmd LC_RPATH
      cmdsize 32
         path @loader_path (offset 12)
--
          cmd LC_RPATH
      cmdsize 168
         path swift/swift-nightly-install/Library/Developer/Toolchains/swift-tensorflow-DEVELOPMENT-2020-04-17-a.xctoolchain/usr/lib/swift/macosx (offset 12)
--
          cmd LC_RPATH
      cmdsize 56
         path @executable_path/../lib/swift/macosx (offset 12)

@abertelrud: I wonder if you have any ideas why this PR didn't work?
#2692 remains a functional fix.

@dan-zheng dan-zheng marked this pull request as draft April 18, 2020 02:50
@abertelrud
Copy link
Contributor

Thanks for taking the time to try this fix, and apologies for not replying until now. I don't know offhand why your diff doesn't work, but I'll take a look and see what I can find. Otherwise, we could take the fix that adds the rpath to all of them, but as I'm also looking into a similar issue, I'd like to take a stab at figuring out how to do more targeted rpaths. BTW, I am just now learning CMake myself, so I will need to explore a bit.

@dan-zheng
Copy link
Contributor Author

Hi @abertelrud,

I wonder if you have any updates regarding this PR or #2692, as a fix for the swiftpm-xctest-helper rpath issue? We've been manually cherry-picking it as part of every macOS toolchain build, and our toolchain-building users are confused why they encounter it too.

Thanks!

@abertelrud
Copy link
Contributor

Hi, apologies for the delay. I've taken a look at this now, and am seeing the same as you. In fact, it doesn't look as if CMake is being used for building swiftpm-xctest-heper (at least not when using Utilities/bootstrap): because even if I deliberately introduce a syntax error into the CMakeLists.txt file for swiftpm-xctest-helper, I get no error. @aciidb0mb3r or @neonichu, do you know whether it's expected that the CMakeLists.txt file for swiftpm-xctest-helper is unused when bootstrapping, and if so, how best to add linker options to it?

@abertelrud
Copy link
Contributor

Looking closer at the log, it looks as if the swiftpm-xctest-helper is being built using SwiftPM even in the bootstrap case, so the CMakeLists.txt might be there only to for the Utilities/build-using-cmake script.

If so, the right place for these flags would be in the SwiftPM package manifest itself.

@abertelrud
Copy link
Contributor

Something like this seems to work:

diff --git a/Package.swift b/Package.swift
index 5244b1fb..0f4ce338 100644
--- a/Package.swift
+++ b/Package.swift
@@ -167,7 +167,9 @@ let package = Package(
         .target(
             /** Shim tool to find test names on OS X */
             name: "swiftpm-xctest-helper",
-            dependencies: []),
+            linkerSettings: [
+                .unsafeFlags(["-Xlinker", "-rpath", "-Xlinker", "@executable_path/../../../lib/swift/macosx"], .when(platforms: [.macOS])),
+            ]),
 
         // MARK: Additional Test Dependencies

I have confirmed that this results in the right rpath in the produced binary, but I am not sure whether that is the right way to do it, and whether there would be any unwanted side effects. @aciidb0mb3r or @neonichu, what do you think?

@aciidgh
Copy link
Contributor

aciidgh commented May 13, 2020

This should be ok for now since we don't export swiftpm-xctest-helper as a product so should have no effect on packages that depend on libSwiftPM.

@dan-zheng
Copy link
Contributor Author

dan-zheng commented Jun 13, 2020

I confirmed that #2694 (comment) works as a fix for SR-12600. I made it into a PR: #2785.
This PR doesn't work as a fix and is obsolete.

@dan-zheng dan-zheng closed this Jun 13, 2020
@dan-zheng dan-zheng deleted the fix-macosx-rpath branch June 13, 2020 19:06
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.

3 participants