Skip to content

Explicitly add rpath for lib_InternalSwiftSyntaxParser.dylib #371

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 1 commit into from
Mar 15, 2022

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Mar 15, 2022

Previously, we were relying on SwiftPM adding the toolchain’s stdlib to the rpaths and we were finding lib_InternalSwiftSyntaxParser.dylib inside the stdlib directory. This behavior was removed in swiftlang/swift-package-manager#4208.

Explicitly add the directory that contains lib_InternalSwiftSyntaxParser.dylib as an rpath to SwiftSyntaxParser when compiling SwiftSyntax using build-script.py.

The major downsides of this approach are

  • You can no longer run SwiftSyntaxParser tests for SwiftSyntax in Xcode with an open source toolchain
    • Maybe this isn’t too bad because it only affects SwiftSyntaxParser tests and those can be run from the command line
  • All adopters of SwiftSyntax also need to explicitly specify the lib_InternalSwiftSyntaxParser.dylib rpath
    • Maybe this isn’t too bad because on Linux lib_InternalSwiftSyntaxParser.dylib is in the standard search paths and on macOS we start shipping lib_InternalSwiftSyntaxParser.dylib as a binary dependency.

Previously, we were relying on SwiftPM adding the toolchain’s stdlib to the rpaths and we were finding `lib_InternalSwiftSyntaxParser.dylib` inside the stdlib directory. This behavior was removed in swiftlang/swift-package-manager#4208.

Explicitly add the directory that contains `lib_InternalSwiftSyntaxParser.dylib` as an rpath to SwiftSyntaxParser when compiling SwiftSyntax using build-script.py.

The major downsides of this approach are
- You can no longer run SwiftSyntaxParser tests for SwiftSyntax in Xcode with an open source toolchain
  - Maybe this isn’t too bad because it only affects SwiftSyntaxParser tests and those can be run from the command line
- All adopters of SwiftSyntax also need to explicitly specify the `lib_InternalSwiftSyntaxParser.dylib` rpath
  - Maybe this isn’t too bad because on Linux `lib_InternalSwiftSyntaxParser.dylib` is in the standard search paths and on macOS we start shipping `lib_InternalSwiftSyntaxParser.dylib` as a binary dependency.
@ahoppen ahoppen requested review from akyrtzi and neonichu March 15, 2022 08:37
@ahoppen
Copy link
Member Author

ahoppen commented Mar 15, 2022

@swift-ci Please test

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround here 👍

@ahoppen
Copy link
Member Author

ahoppen commented Mar 15, 2022

swiftlang/swift-stress-tester#175

@swift-ci Please test

@ahoppen ahoppen merged commit aa38592 into swiftlang:main Mar 15, 2022
@ahoppen ahoppen deleted the pr/add-parser-lib-rpath branch March 15, 2022 11:57
@dduan
Copy link
Contributor

dduan commented Mar 15, 2022

This change made the package un-buildable for users with the following errors

the target 'SwiftSyntax' in product 'SwiftSyntax' contains unsafe build flags

Test manifest

// swift-tools-version:5.5

import PackageDescription

let package = Package(
    name: "SwiftSyntaxProblem",
    products: [
        .executable(
            name: "SwiftSyntaxProblem",
            targets: ["SwiftSyntaxProblem"]
        ),
    ],
    dependencies: [
        .package(
            name: "SwiftSyntax",
            url: "https://github.com/apple/swift-syntax.git",
            .exact("0.50600.0")
        ),
    ],
    targets: [
        .executableTarget(
            name: "SwiftSyntaxProblem",
            dependencies: ["SwiftSyntax"]),
    ]
)
swift-driver version: 1.26.21 Apple Swift version 5.5.2 (swiftlang-1300.0.47.5 clang-1300.0.29.30)
Target: arm64-apple-macosx12.0

@dduan
Copy link
Contributor

dduan commented Mar 15, 2022

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