-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[Driver] Use llvm-ar by default on Unix and copy it over into the build directory #62800
base: main
Are you sure you want to change the base?
Conversation
@egorzhdan, would you run the CI on this? |
@swift-ci please smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not convinced that this is really the right thing to do. ar
is the system archiver. I think that we should continue to defer to that if/when possible. It is also possible that the ar
installed on the system is new enough to support LTO via plugins. I think that defaulting AR
to llvm-ar
is reasonable if you are building on Termux as a host, but even when targeting android, I think that the system archiver should be preferred.
While this pull is not strictly required because of your SPM change, I think it's a good idea to remove that external dependency.
I mean, we already ship our own clang.
I'm unsure what this refers to as I don't use LTO, but since
On the contrary, I think that if we can remove this external dependency on the system archiver for one we supply, we should. The long-standing escape hatch of supplying your own archiver with There is no "right" answer here: it's a question of priorities. @gottesmm, would you like to chime in? |
I can't make head nor tails of this Windows CI error: your LLVM directory on the Windows CI doesn't have llvm-ar?
|
The correct spelling is |
Thanks, I will just remove that copy for non-Unix platforms. Since the linux CI passed, it appears this approach does work, unlike last time I tried but didn't copy over |
utils/CMakeLists.txt
Outdated
file(COPY ${LLVM_RUNTIME_OUTPUT_INTDIR}/bin/llvm-ar | ||
DESTINATION "${SWIFT_RUNTIME_OUTPUT_INTDIR}" | ||
FILE_PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ | ||
GROUP_EXECUTE WORLD_READ WORLD_EXECUTE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really should be done outside of the CMake in the build-script python logic. The way that we handle this on Windows is by using the LLVM distribution functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason you think this should be in Python instead? I could see someone porting the toolchain to a new platform only by using CMake without build-script
, as I did natively on Android initially, and this being in CMake would keep that working for new Unix platforms.
As for Windows, I simply disabled this copy on OS X and Windows in the latest version of this pull.
Rebased and added a fix for the Windows CI. |
@compnerd, this should pass the Windows CI now. |
@swift-ci please smoke test |
@CodaFi, any input on this change of the default archiver name on Unix platforms? |
@troughton, you added this Driver option in #25202, any feedback on this consolidation? |
@0xTim, maybe you'd like to chime in since this affects building for linux on the server. |
@buttaface What's the TL;DR for end users writing and compiling code? From my understanding of this is doesn't really affect anything? |
@0xTim, it shouldn't, as all it's doing is changing the default archiver used for static libraries on Unix platforms like linux from the Ever since SPM made it mandatory that such a librarian archiver be installed before it builds anything last year (with 5.7, it's not mandatory and will simply fail when building static libraries if it doesn't find |
Ping @ktoso, what do you think about getting this change in? |
@edymtt, would you give your opinion on this archiver shift? |
I am currently busy with other priorities, and as a result I don't plan to be able to look into this any time soon. (And in case I'm able to return to this, I'm not sure if I would be able to do a proper review, since I don't have much experience with the Linux/Unix aspects of the build) |
Ping @tomerd, please let me know what you think. |
@al45tair, just need someone to decide on this: you have linux experience, wdyt? |
Interestingly I'm actually deliberately using One thing I'm not entirely clear on here is what happens if the triple says to use a specific object format. It isn't obvious to me what There's also the somewhat orthogonal issue of the exact format of archive files. I'd assume that |
I don't know the details, but my understanding is that the LLVM tools are fairly cross-platform. If Note that what this pull proposes is to change the default archiver that the Swift compiler uses for native compilation on Unix platforms: cross-compilation is a hairier situation that sometimes requires that
I'm unaware of differences in the archive format output produced by |
@al45tair, what do you think about finally getting this in now? We have four months of solid experience with swiftlang/swift-package-manager#6829 switching the default for Unix on trunk and 5.10 to |
@bnbarham, would you run the CI and review? |
Ping @al45tair, wdyt? |
@swift-ci Please smoke test |
@finagolfin I'll need to discuss this with some other folks, but I'm broadly in favour of using |
@al45tair, any update? Would be good to get these two driver pulls in before the 6.0 branch. |
Nothing yet, sorry. There are some things going on behind the scenes that have a bearing on decisions like this one — I don't know that we'll be able to get this in before the Swift 6 branch, and I still have some reservations (as does @compnerd, who I have spoken to briefly about this). |
@al45tair, anything new? I think we should get these two Driver pulls in before the branch, as SwiftPM switching most builds over to |
@al45tair, any result from those discussions? |
@al45tair, this keeps getting delayed. We have evidence that SwiftPM works well with this I realize this is a small consistency issue and most Swift devs simply use SwiftPM and thus |
Originally the hold-up was that we were setting up the Platform Steering Group but hadn't yet announced it, and this is definitely the kind of thing PSG is supposed to look into. Subsequently the hold-up is that I've been busy working on something other than this, and it's me that is supposed to be looking at whether it's really OK to use Note also that SwiftPM often doesn't bother making static archives anyway — unless it's a This is very much on my list of things to do, and towards the top of it at that. Next week, as I'm sure you're aware, is WWDC, which has been keeping me busy but I expect to have more time once that's over with. |
By that, I and the Swift code refer to non-Darwin, non-Windows platforms, for which my SwiftPM pull made
Great, I welcome further testing. 😄
I know it's an uncommon build product config, with a few packages simply commenting it out.
OK, thanks for the update. I just didn't understand the repeated delays for this small pull, now I do. |
@al45tair, rebased this pull, would be good to get this in before the upcoming 6.1 branch. |
@swift-ci please smoke test |
@swift-ci please smoke test linux |
1 similar comment
@swift-ci please smoke test linux |
@al45tair, we're coming on the two-year anniversary of my submitting this pull, 🥲 would be good to finally get it merged in the coming months. I've rebased it and made sure it passes the CI. |
…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.
@swift-ci test |
@swift-ci smoke test macos |
swiftlang/swift-driver#1822 |
Explained why it's in CMake for non-Darwin Unix platforms and no longer applies to Windows for more than a year
@al45tair, rebased and passed all CI again, can we go ahead and get this and the equivalent swift-driver pull in now? The main SwiftPM switch to |
swiftlang/swift-tools-support-core#500 |
swiftlang/swift-tools-support-core#500 |
Now that llvm-ar is installed by default in the toolchain, #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.
I noticed that we now install the LLVM and compiler tools to
toolchain-linux-x86_64/
before the corelibs are built, but we don't use that yet to build the corelibs inbuild-script-impl
, only for SPM and onwards. I considered switching over to using that installed toolchain to build the corelibs, but since that's a larger change that might fail elsewhere and because it would require simple builds to always--install-swift
too, I chose this simpler route of copying over the freshly-builtllvm-ar
.Alternately, we could install
llvm-ar
in the linux CI by making sure that Ubuntu package is always installed, but I figured this copying is easier for now.Note that the system
ar
is still passed intoCMAKE_AR
, so the corelibs will keep using that for the non-Swift static libs on the linux CI, but at least this will allow changing the Swift compiler's default archiver name for Unix.