Skip to content

[5.9][build] Make sure SPM is built with the forked clang, not the host clang #67249

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
Jul 28, 2023

Conversation

finagolfin
Copy link
Member

Cherrypick of #64629

Explanation: SPM was built with the freshly-built Swift toolchain, including the Swift-forked clang, for years, but a change in swiftlang/swift-package-manager#5894 late last year inadvertently switched it to be built with the system clang used to natively build the Swift compiler, ie self.toolchain.cc in build-script. That passed the CI but caused problems when cross-compiling SPM on my daily Android CI, as the system clang is only meant for native compilation- for example, we use the freshly-built Swift-forked clang for the native host by default when cross-compiling the Swift compiler itself- so I've been using this patch for the last seven months on my Android CI.

Scope: Only affects the C/C++/asm files built as part of SwiftPM, particularly when cross-compiled

Issue: None

Risk: low, as this only affects compilation of non-Swift portions of SPM itself

Testing: Passed all CI on trunk and I've been using this on my Android CI since last year

Reviewer: @etcwilde

@finagolfin finagolfin requested a review from a team as a code owner July 12, 2023 12:40
@finagolfin
Copy link
Member Author

@kateinoigakukun, would you run the CI on this?

@kateinoigakukun
Copy link
Member

@swift-ci please test

Copy link
Contributor

@al45tair al45tair left a comment

Choose a reason for hiding this comment

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

We’re going to revert the original as it’s broken the SwiftPM option on build-script.

@finagolfin
Copy link
Member Author

This has been reapplied to trunk.

@bnbarham, please run the CI on this again.

@bnbarham
Copy link
Contributor

It's already passed, but I can run again since swiftlang/swift-package-manager#6729 may have caused changes.

@bnbarham
Copy link
Contributor

@swift-ci please test

@finagolfin
Copy link
Member Author

@al45tair, please remove the request whenever you think this is safe to go in.

If there are no further problems on trunk, do you think we can get this into 5.8 also? We'd probably need to get your SPM change in there too.

@al45tair al45tair self-requested a review July 26, 2023 11:07
@al45tair
Copy link
Contributor

5.9 should be good to go in now.

@finagolfin
Copy link
Member Author

@etcwilde, ready for merge.

@finagolfin
Copy link
Member Author

@tomerd, ready to go.

@al45tair al45tair merged commit 549ad3f into swiftlang:release/5.9 Jul 28, 2023
@finagolfin finagolfin deleted the release/5.9 branch July 28, 2023 16:25
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.

5 participants