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

Install llvm-ar in the toolchain #62510

Merged
merged 1 commit into from
Dec 18, 2022
Merged

Install llvm-ar in the toolchain #62510

merged 1 commit into from
Dec 18, 2022

Conversation

finagolfin
Copy link
Contributor

A recent SPM change made it mandatory for there to be an archiver in the toolchain/PATH, swiftlang/swift-package-manager#5761, so make sure llvm-ar is bundled in the toolchain.

@compnerd and @tomerd, let me know what you think.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

I think that the build and packaging is good. Changing the default I'm less certain about. Arguably, we already have to do that with LTO, so this does simplify things, but honouring AR from the environment seems reasonable as that mirrors gcc and clang IIRC.

@finagolfin
Copy link
Contributor Author

finagolfin commented Dec 12, 2022

but honouring AR from the environment seems reasonable as that mirrors gcc and clang IIRC.

This simply changes the name of the default archiver looked for by the Swift compiler on Unix, it changes nothing about where it's looked for, ie you can still configure that by passing in a -tools-directory to the compiler or a wholly different archiver to SPM. The only real change is that we would be switching the default on Linux from ar in binutils to this llvm-ar we're now bundling in the Swift toolchain.

@finagolfin
Copy link
Contributor Author

@MaxDesiatov, I see you're up, would you run the CI on this?

@MaxDesiatov
Copy link
Contributor

I'd like to remind that as a frequent contributor you can get CI access, as described in the corresponding docs.

@MaxDesiatov
Copy link
Contributor

@swift-ci please smoke test

@finagolfin
Copy link
Contributor Author

@MaxDesiatov, as I mentioned to you the last time you reminded me, I'm winding down my port of Swift to Android and plan to submit only maybe 5-10 maintenance pulls in the coming year. With such a low frequency, I think it's better for security reasons that I don't have commit access, so I prefer to ask others to run the CI. If I'm bothering you in any way, please just say so and I will ask other committers from now on.

@finagolfin
Copy link
Contributor Author

Building CMark on Linux and macOS failed because I changed the host archiver to llvm-ar too. I wasn't sure about that one as it depends on what is pre-installed on the CI host. Since it appears llvm-ar is not installed- Windows passed as it doesn't build CMark for some reason- should be fine to use the installed ar only to build the host compiler itself, removed that last change of the host archiver.

@finagolfin
Copy link
Contributor Author

@egorzhdan, please run the CI again.

@egorzhdan
Copy link
Contributor

@swift-ci please smoke test

A recent SPM change made it mandatory for there to be an archiver in the
toolchain/PATH, swiftlang/swift-package-manager#5761, so make sure llvm-ar is
bundled in the toolchain.
@finagolfin
Copy link
Contributor Author

Linux CI failed because I changed the default archiver name on Unix to llvm-ar and the static libdispatch build couldn't find it because the compiler uses what's installed in the host environment, as the freshly-built llvm-ar isn't installed in the same directory as the compiler yet. We could remedy this by installing llvm-ar on the linux CI, but I just changed back the default archiver used instead. We can always do that later, simply bundling an archiver in the toolchain should do for cross-compiling to Android for now.

@egorzhdan, one final CI run and this can go in.

@finagolfin finagolfin changed the title Install llvm-ar in the toolchain and use it by default on Unix Install llvm-ar in the toolchain Dec 15, 2022
@keith
Copy link
Member

keith commented Dec 15, 2022

@swift-ci please smoke test

@finagolfin
Copy link
Contributor Author

Alright, @compnerd, this is ready for review.

@finagolfin
Copy link
Contributor Author

@keith, mind getting this in? I submitted this so we can get this mitigation in before the 5.8 branch tomorrow.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Building and packaging the LLVM archiver is a good idea in general.

@compnerd
Copy link
Member

Yeah, merging this before the 5.8 branch sounds reasonable. Thanks @buttaface!

@compnerd compnerd merged commit 53e4d03 into swiftlang:main Dec 18, 2022
finagolfin added a commit to finagolfin/swift that referenced this pull request Jan 1, 2023
…ld directory

Now that llvm-ar is installed by default in the toolchain, swiftlang#62510, and a recent
SPM change requires there to be an archiver in the toolchain/PATH,
swiftlang/swift-package-manager#5761, use that bundled llvm-ar for all Unix
platforms, which requires copying it over into the build directory too before
building the corelibs.
finagolfin added a commit to finagolfin/swift that referenced this pull request Jan 3, 2023
…ld directory

Now that llvm-ar is installed by default in the toolchain, swiftlang#62510, and a recent
SPM change requires there to be an archiver in the toolchain/PATH,
swiftlang/swift-package-manager#5761, use that bundled llvm-ar for all Unix
platforms, which requires copying it over into the build directory too before
building the corelibs.
finagolfin added a commit to finagolfin/swift that referenced this pull request Jan 9, 2024
…ld directory

Now that llvm-ar is installed by default in the toolchain, swiftlang#62510, and a recent
SPM change requires there to be an archiver in the toolchain/PATH,
swiftlang/swift-package-manager#5761, use that bundled llvm-ar for all Unix
platforms, which requires copying it over into the build directory too before
building the corelibs.
finagolfin added a commit to finagolfin/swift-driver that referenced this pull request Jan 9, 2024
Now that llvm-ar is installed by default in the toolchain, swiftlang/swift#62510,
and a recent SPM change requires there to be an archiver in the toolchain/PATH,
swiftlang/swift-package-manager#5761, use that bundled llvm-ar for all Unix
platforms.
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