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.

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

Clean up rpaths of installed swift-format #683

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Jan 25, 2024

  • Don’t explicitly add rpaths to /usr/lib/swift, @executable_path/../lib/swift/macosx and @executable_path/../lib/swift-5.5/macosx on Darwin. I don’t know what they were needed for but they don’t seem to be necessary. Also standard Swift command line tools created from Xcode don’t contain these rpaths.
  • Increase the deployment target to macOS 12.0. This removes an rpath in the built binary that is an absolute path to the toolchain on the host, which was used to build swift-format.
  • Set --disable-local-rpath on Linux. Otherwise the swift-format executable has an $ORIGIN rpath on Linux, which we don’t want.
  • Rename SOURCEKIT_LSP_CI_INSTALL -> SWIFTFORMAT_CI_INSTALL. Just cleaning up after copy-pasting.

rdar://121400644

@ahoppen ahoppen requested review from allevato and bnbarham January 25, 2024 02:13
@ahoppen ahoppen force-pushed the ahoppen/clean-rpath branch from c0033f2 to 695eddb Compare January 25, 2024 02:19
Copy link
Member

@allevato allevato left a comment

Choose a reason for hiding this comment

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

I'm not sure if any users will bristle at the higher macOS deployment version requirement. I'm personally ok with it (folks should at least be on Monterey by now, and it allows us to use Regex), but I noticed that sourcekit-lsp is still on macOS 12. Do you plan on updating that one as well? I wonder if they should all be consistently using the same value.

@ahoppen
Copy link
Member Author

ahoppen commented Jan 26, 2024

Usually the policy has been that sourcekit-lsp and swift-format should have the same deployment target as the Xcode that is released alongside it (which usually is the current and last OS). IMO we should always be able to bump the deployment target to that version but if there’s no need to increase the deployment target, I think it’s nice to keep it as low as possible.

- Don’t explicitly add rpaths to `/usr/lib/swift`, `@executable_path/../lib/swift/macosx` and `@executable_path/../lib/swift-5.5/macosx` on Darwin. I don’t know what they were needed for but they don’t seem to be necessary. Also standard Swift command line tools created from Xcode don’t contain these rpaths.
- Increase the deployment target to macOS 12.0. This removes an rpath in the built binary that is an absolute path to the toolchain on the host, which was used to build swift-format.
- Set `--disable-local-rpath` on Linux. Otherwise the swift-format executable has an `$ORIGIN` rpath on Linux, which we don’t want.
- Rename `SOURCEKIT_LSP_CI_INSTALL` -> `SWIFTFORMAT_CI_INSTALL`. Just cleaning up after copy-pasting.

rdar://121400644
KidiIT

This comment was marked as spam.

@KidiIT

This comment was marked as spam.

@ahoppen ahoppen deleted the ahoppen/clean-rpath branch February 4, 2024 06:47
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