Build: initial pass to support static archives on Windows #5720
Conversation
compnerd
requested review from
abertelrud,
elsh,
neonichu and
tomerd
as code owners
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
| if triple.isWindows(), librarian.hasSuffix("link") || librarian.hasSuffix("link.exe") { | ||
| return [librarian, "/LIB", "/OUT:\(binary.pathString)", "@\(linkFileListPath.pathString)"] | ||
| } | ||
| if triple.isDarwin(), librarian.hasSuffix("libtool") { |
There was a problem hiding this comment.
Are these checks for the filename suffix temporary, or intended to be part of the final PR? If the latter, I wonder how robust they are in case people substitute their own version that is intended to work the same as the official one. Would it make sense to expect that whatever the librarian executable is called, it's expected to take a certain set of flags? I think this is how the other tools are expected to work even if customized via an env var.
There was a problem hiding this comment.
They are intended to be the final version. The reason is that the tools have differing command line invocations. lld-link and link (which may be invoked with or without the .exe suffix) both use the MSVC style command line. libtool and llvm-libtool would use the libtool like command line. The other platforms use ar or llvm-ar and would use the Unix style command line.
There is a related issue on Windows. The default tool should be link, and then if the user specifies to use lld (-use-ld=lld), we should prefer lld-link. Finally, if the user sets AR=llvm-ar we should actually fallback to ar as the librarian. On Darwin, libtool is the preferred librarian, though it is possible to use ar (as was previously the case due to the use of the test rules in llbuild).
There was a problem hiding this comment.
I think the question is what happens if you supply an entirely custom tool.
I wonder if we should just change the default? I think the libtool-style argument list makes more sense for a generic tool vs. the highly specific ar argument list.
There was a problem hiding this comment.
I think that ar style arguments make more sense - that is the default archiver on everything but Windows and Darwin.
|
Approach looks good in general, but had a question about checking the name of the executable. Also, should we mark as Draft while it's being iterated on? |
|
Thanks for this work BTW! Having SwiftPM take more control over the forming of the archiving command makes sense to me. |
|
One more thought, for the future: I know it's beyond the limited scope of this PR, but these changes remind me that at some point the generation of the actual command lines should be factored so that BuildPlan can be focused on the semantics without all the details. One approach might be to have a protocol for each of the conceptual tools based on what they do ("archiver", etc), and then concrete types for the various implementations of that protocol to construct the actual command lines (separately from the BuildPlan logic). Again, just thoughts for the future since I'm seeing this code again, and not something to do for this PR. Would also make testing of the command line generation a bit simpler. |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test macOS platform |
Introduce a SPM controlled build rule for building static libraries. This is the intended way to use llbuild to drive the generation of static libraries. We would previously rely on the static default rule intended for testing to generate the static libraries. Not only did this tool not properly support Windows, it would actually cause problems on macOS due to the use of `ar` for the creation of the library over the preferred tool - `libtool`. We now locally determine the correct rule and generate the command. This is incomplete support for Windows and in fact regresses functionality. We no longer honour `AR` as an environment variable on Windows and thus cannot switch the implementation of the librarian. We now drive the archiving through `lld-link` unconditionally while we should prefer `link` unless otherwise requested. This is covered as an issue in swiftlang#5719.
|
@swift-ci please smoke test |
FYI, I filed #5729 to track refactoring BuildPlan.swift someday. Feel free to add ideas — it is certainly beyond the scope of this PR, I'm just reflecting that this PR would have been a bit cleaner if there were more separation of concerns in the code. |
|
@compnerd, do you think we can get this into 5.7 before the release? I applied it to the last 5.7 source snapshot before building it for Android AArch64: it works well and allowed me to remove the llbuild hack I'm using for 5.6 to do the same. |
4 participants
Introduce a SPM controlled build rule for building static libraries.
This is the intended way to use llbuild to drive the generation of
static libraries. We would previously rely on the static default
rule intended for testing to generate the static libraries. Not only
did this tool not properly support Windows, it would actually cause
problems on macOS due to the use of
arfor the creation of the libraryover the preferred tool -
libtool. We now locally determine thecorrect rule and generate the command.
This is incomplete support for Windows and in fact regresses
functionality. We no longer honour
ARas an environment variable onWindows and thus cannot switch the implementation of the librarian. We
now drive the archiving through
lld-linkunconditionally while weshould prefer
linkunless otherwise requested. This is covered asan issue in #5719.