Skip to content

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

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
Aug 4, 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 17, 2023 09:23
@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.

@etcwilde
Copy link
Contributor

@swift-ci please test macOS

@al45tair al45tair self-requested a review July 25, 2023 18:06
@finagolfin
Copy link
Member Author

@al45tair, what about this one? I can submit your SPM change for 5.8 too, then this should be fine.

@al45tair
Copy link
Contributor

Yeah, the SwiftPM change needs cherry picking first. I was going to do that later on today (I'm build wrangler at the moment so I'm busy doing that right now).

@finagolfin
Copy link
Member Author

@al45tair, I can submit that 5.8 SPM pull with your commit for you if you're busy, just let me know.

@al45tair
Copy link
Contributor

The cherry pick of that change is here: swiftlang/swift-package-manager#6754

@finagolfin
Copy link
Member Author

@egorzhdan, one last CI run and we can get this in.

@al45tair
Copy link
Contributor

I don't think we need a CI run so much as a branch manager to approve it. @tomerd you can do that I believe.

@finagolfin
Copy link
Member Author

@al45tair, this pull has not been run through the CI since your SPM pull was merged into 5.8 before the weekend, which is why I suggested it.

@al45tair
Copy link
Contributor

al45tair commented Aug 1, 2023

@swift-ci Please test

@finagolfin
Copy link
Member Author

Linux CI failed when checking out the source, it seems super flaky this week.

@al45tair
Copy link
Contributor

al45tair commented Aug 1, 2023

@swift-ci Please test Linux platform

@finagolfin
Copy link
Member Author

@bnbarham, one last linux CI run?

@bnbarham
Copy link
Contributor

bnbarham commented Aug 2, 2023

@swift-ci Please test Linux platform

@finagolfin
Copy link
Member Author

@egorzhdan, one more linux CI run, please.

@egorzhdan
Copy link
Contributor

@swift-ci please test Linux

@finagolfin
Copy link
Member Author

Whoo, it didn't crash at source checkout this time. 😄

@finagolfin
Copy link
Member Author

@tomerd, ready for final approval and merge.

@al45tair al45tair merged commit f741bc2 into swiftlang:release/5.8 Aug 4, 2023
@finagolfin finagolfin deleted the release/5.8 branch August 4, 2023 15:23
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.

7 participants