-
Notifications
You must be signed in to change notification settings - Fork 42
Add SwiftBuild dlls to cli installer component alongside SwiftPM #417
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
base: main
Are you sure you want to change the base?
Conversation
e4d3a59
to
ab10d2d
Compare
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 change is technically correct and nothing to be done here.
However, because no good deed goes unpunished - looking at this entry makes me wonder if we are building Swift Build properly. Should all these dynamic libraries be dynamic? Would we do better in terms of size and performance to convert some of them to static? I do not have the answer to that, and that is something that requires experimentation. That is something that we should really consider and evaluate (and make the appropriate changes here).
That work however IMO should be a follow up change. Packaging and distributing SwiftBuild as a starting point is the right thing to do.
One final question: should this also go into 6.2?
Right now building all the libraries as dynamic simplifies the build by making it easy to ensure we're picking the right linker driver to pull in the c++ stdlib + blocks runtime in the right targets. With some tweaks to swift-driver and general build config cleanup, I think we can gain a lot of flexibility here and experiment with alternate layouts. That said, we've been shipping the fully dynamically linked version on macOS for quite awhile without issues, so I'm not too concerned about perf impact.
I intend to cherrypick to 6.2 but I'd like to make sure this + the SwiftPM changes are working end-to-end on main first |
Please do a cross-repo test with building the toolchain for Windows and Windows ARM64 before merging. |
hmm seeing failures in the cross-repo toolchain build
Which is true, TOOLCHAIN_ROOT isn't on the command line:
|
ab10d2d
to
2ddf127
Compare
Think I just needed a rebase, my local checkout was old |
2ddf127
to
dca65b9
Compare
TOOLCHAIN_ROOT -> ToolchainRoot to align with the changes I rebased on |
Verified that the toolchains built successfully and can be installed: I can launch SwiftPM without any missing DLL errors, which is a good sign that nothing is broken at runtime. I couldn't test much more than that because swiftc.exe is crashing in mimalloc when compiling manifests:
I expect this is probably unrelated to the changes here since the compiler/driver should be unaffected, but I'll see if there's anything I can double-check - any concerns on your end @compnerd? |
This is my first time touching the windows installer, so there may be some silly mistakes here. Opening the PR now so I can run some tests