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

[cmake] Install libraries in standard directories #446

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

Steelskin
Copy link

Previously, libs were installed under lib/swift/${os}. They should be installed in the default library directory for the relevant target system.
In addition, swiftmodules were installed in the older layout format. This changes to use the standard modern layout format for swiftmodules.

Checklist

  • [*] I've read the Contribution Guidelines
  • [*] My contributions are licensed under the Swift license.
  • [*] I've followed the coding style of the rest of the project.
  • [*] I've added tests covering all new code paths my change adds to the project (if appropriate).
  • [*] I've added benchmarks covering new functionality (if appropriate).
  • [*] I've verified that my change does not break any existing tests or introduce unexplained benchmark regressions.
  • [*] I've updated the documentation if necessary.

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.

The changes look good to me! Will this change affect builds of swift-collections that are part of Swift toolchains?

Oh, could you please target this change on the release/1.2 branch rather main? I expect we'll want to quickly ship this.

@Steelskin
Copy link
Author

The changes look good to me! Will this change affect builds of swift-collections that are part of Swift toolchains?

Oh, could you please target this change on the release/1.2 branch rather main? I expect we'll want to quickly ship this.

Ah sorry, I opened it as a draft PR because I am waiting for a review on apple/swift-argument-parser#685, which does the same thing but in swift-argument-parser. I intend to update the rest of the libraries used by the toolchain in the same manner and I would like to stay consistent.

Previously, libs were installed under `lib/swift/${os}`. They should be
installed in the default library directory for the relevant target
system.
In addition, swiftmodules were installed in the older layout format.
This changes to use the standard modern layout format for swiftmodules.
@Steelskin Steelskin force-pushed the fabrice/cmake-install-paths branch from 7c716d0 to e0021fb Compare February 3, 2025 21:16
@Steelskin Steelskin changed the base branch from main to release/1.2 February 3, 2025 21:17
@Steelskin Steelskin marked this pull request as ready for review February 3, 2025 21:17
@Steelskin Steelskin requested a review from lorentey February 3, 2025 21:17
@Steelskin
Copy link
Author

apple/swift-argument-parser#685 has landed. This is bringing the same changes to this repository. I changed the base branch to release/1.2 as requested. Will this make it back to main automatically or do I need to do something else?

This also reverts 40adeb7, which seems to have been done in error.
@Steelskin
Copy link
Author

I pushed an update that reverts commit 40adeb7, it seems to have been done in error. Other Swift libraries put the .swiftdoc in the .swiftmodule folder.

Steelskin added a commit to Steelskin/swift-installer-scripts that referenced this pull request Feb 3, 2025
Steelskin added a commit to Steelskin/swift-installer-scripts that referenced this pull request Feb 4, 2025
@lorentey
Copy link
Member

lorentey commented Feb 7, 2025

Will this make it back to main automatically or do I need to do something else?

You don't need to do anything else -- the changes will get picked up the next time we merge things forward. (This is a manual process, so there can be a delay. But this repo's main should not be used in production.)

@lorentey lorentey merged commit b5e880d into apple:release/1.2 Feb 7, 2025
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.

2 participants