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

Use llvm-ar as default librarian on Unix platforms #6829

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

finagolfin
Copy link
Contributor

Resolves #5761

@compnerd, I implemented your TODO comment exactly.

@compnerd
Copy link
Member

@swift-ci please smoke test

@compnerd
Copy link
Member

Seems like a step in the right direction - @MaxDesiatov thoughts?

@finagolfin
Copy link
Contributor Author

Sigh, didn't compile this first and had a syntax error. Fixed it now, please rerun the CI.

@MaxDesiatov
Copy link
Contributor

@swift-ci smoke test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@compnerd
Copy link
Member

Hmm, this doesn't fallback any longer?

@finagolfin
Copy link
Contributor Author

this doesn't fallback any longer?

Yes, I removed it based on Boris's suggestion, see the review comments above.

@finagolfin
Copy link
Contributor Author

@egorzhdan, please run the CI on this.

@finagolfin
Copy link
Contributor Author

@neonichu, another CI run and we can get this in?

@tomerd
Copy link
Contributor

tomerd commented Aug 25, 2023

@swift-ci smoke test

@tomerd
Copy link
Contributor

tomerd commented Aug 25, 2023

@MaxDesiatov @compnerd is this good to merge?

@compnerd
Copy link
Member

I think that keeping the fallback is better. The librarian is expected to be ar. llvm-ar is a drop-in replacement, but the official archiver is still ar and we should resort to that if necessary.

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor

IIRC @al45tair had a relevant change in this area, would be great to get his opinion

@finagolfin
Copy link
Contributor Author

I don't think the fallback is necessary since the 5.8 toolchains onwards come with llvm-ar, so it just depends how safe you want to play it. @neonichu, wdyt?

The single self-hosted macOS test failure looks like a flake.

@neonichu
Copy link
Contributor

I don't think the fallback is necessary since the 5.8 toolchains onwards come with llvm-ar, so it just depends how safe you want to play it. @neonichu, wdyt?

I agree with you but @compnerd also makes a valid point. It might make new platform bringup easier to keep the fallback in place?

@finagolfin
Copy link
Contributor Author

OK, I will put the fallback in again.

In the meantime, the macOS smoke test passed, meaning the self-hosted failure was a flake.

@finagolfin
Copy link
Contributor Author

Rebased and the fallback is back.

@finagolfin
Copy link
Contributor Author

One last CI run and we can get this in.

@finagolfin
Copy link
Contributor Author

@egorzhdan, would you run the CI on this?

@egorzhdan
Copy link
Contributor

@swift-ci please test

@finagolfin
Copy link
Contributor Author

Hmm, only one CI ran?

@neonichu
Copy link
Contributor

@swift-ci please smoke test

@finagolfin
Copy link
Contributor Author

@compnerd, ready to go in, just needs approval.

@finagolfin
Copy link
Contributor Author

@neonichu, can we get this in now?

@finagolfin
Copy link
Contributor Author

@tomerd, can we get this in before the branch tomorrow?

@finagolfin
Copy link
Contributor Author

@neonichu, would be good to get this merged before the branch today.

@neonichu neonichu merged commit 92e0e8f into swiftlang:main Sep 6, 2023
5 checks passed
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.

SwiftPM requires librarian to be installed before it can build anything
6 participants