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 forwarding symlinks to reduce toolchain installation size #3276

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

owenv
Copy link
Contributor

@owenv owenv commented Feb 14, 2021

swift-build, swift-package, swift-test, swift-package-collection, and swift-run are now installed as symlinks to _swift-package-manager, which selects the correct tool based on the path used to invoke it.

Motivation:

Reduce download & installation size of the Swift toolchain

Modifications:

  • Add a new executable target, _swift-package-manager which runs the appropriate Swift*Tool.main() based on the path used to invoke it.
  • When installing SwiftPM, only copy _swift-package-manager. Create symlinks named swift-build, swift-package, etc. to _swift-package-manager
  • In the future, we can probably get rid of the symlinks by teaching the driver how to invoke the subcommands directly.

Result:

@owenv
Copy link
Contributor Author

owenv commented Feb 14, 2021

@swift-ci please smoke test

@owenv
Copy link
Contributor Author

owenv commented Feb 14, 2021

@swift-ci please build toolchain

@owenv
Copy link
Contributor Author

owenv commented Feb 15, 2021

@swift-ci please smoke test

@owenv
Copy link
Contributor Author

owenv commented Feb 15, 2021

The macOS toolchain build is failing due to an unrelated issue, but I was able to get some concrete numbers by comparing the Ubuntu 16.04 toolchain to the 2/9 nightly snapshot:

Compressed:
2/9 nightly - 598 MB
With this change - 523 MB (12.5% reduction)

Uncompressed
2/9 nightly - 1.98 GB
With this change - 1.71 GB (13.6% reduction)

With this change, we go from 5 to 1 68 MB executables, which lines up with the numbers here - the impact ended up being much larger than I expected

@shahmishal
Copy link
Member

This is awesome!

@tomerd
Copy link
Contributor

tomerd commented Feb 15, 2021

this is great, thank you @owenv

@abertelrud
Copy link
Contributor

This looks great In the long run it might make sense to have swift-package be the "native" one, with swift build, swift run, etc being shorthands swift package run, swift package test, etc. I think this is what you were referring to in the notes for the PR, and this is something that was discussed a lot in the early days. But this PR seems like a good and safe step.

It looks as if this doesn't change what gets built at debug time, which I don't think will be a problem but which we should maybe consider in a follow-on PR.

abertelrud
abertelrud previously approved these changes Feb 15, 2021
@abertelrud abertelrud self-requested a review February 15, 2021 20:57
@abertelrud abertelrud dismissed their stale review February 16, 2021 00:15

Temporarily marking as un-reviewed so it doesn't get merged before hearing from other reviewers (want more eyes on this).

@tomerd
Copy link
Contributor

tomerd commented Feb 16, 2021

@owenv thanks again for putting together. we would need a bit of time before we can merge this, but will do ASAP

@owenv
Copy link
Contributor Author

owenv commented Feb 16, 2021

@tomerd no problem! I'm in no rush to land this

@owenv
Copy link
Contributor Author

owenv commented Feb 16, 2021

This looks great In the long run it might make sense to have swift-package be the "native" one, with swift build, swift run, etc being shorthands swift package run, swift package test, etc. I think this is what you were referring to in the notes for the PR, and this is something that was discussed a lot in the early days. But this PR seems like a good and safe step.

Yeah, I think there's a few different approaches we could take to clean this up. My one concern is that there might be external tools that are looking specifically for swift-package, swift-build, etc. in usr/bin/ instead of invoking SwiftPM through usr/bin/swift.

It looks as if this doesn't change what gets built at debug time, which I don't think will be a problem but which we should maybe consider in a follow-on PR.

I think the other executable targets are worth keeping around for now because they make manual testing much easier and they don't add much time to the build. _swift-package-manager is pretty annoying to use if you don't setup symlinks first.

@benrimmington
Copy link
Contributor

Could the _swift-package-manager executable be a hidden file? Would it make sense to use the .swiftpm name?

@owenv
Copy link
Contributor Author

owenv commented Feb 16, 2021

@benrimmington I'm not entirely sure if that would work, but I think it's unnecessary. We already ship other executables like repl_swift in the usr/bin/ directory that aren't useable as standalone tools

@tomerd tomerd added the next waiting for next merge window label Feb 16, 2021
@owenv owenv closed this Mar 20, 2021
@owenv owenv reopened this Mar 20, 2021
@owenv
Copy link
Contributor Author

owenv commented Mar 20, 2021

I just wanted to follow up on this - is there any particular reason it can't be shipped with Swift 5.5/anything I can do to unblock it?

@tomerd
Copy link
Contributor

tomerd commented Mar 22, 2021

apologies for the delay @owenv. we are waiting for the 5.5 branching to take changes that should land post 5.5

@owenv
Copy link
Contributor Author

owenv commented Apr 17, 2021

@swift-ci please smoke test

@tomerd
Copy link
Contributor

tomerd commented Apr 19, 2021

thanks for your patience @owenv. 5.5 was just branched this weekend so we are finally ready to take this, @abertelrud can work with you to land

@abertelrud
Copy link
Contributor

This looks great, @owenv — thank you for your work on this and for your patience!

It looks good to me, but I have a suggestion to simplify things even further: since swift-package is the main tool of SwiftPM, I think the same thing could be achieved without a separate _swift-package-manager helper.

I suggest the following simplification, which seems to work well in preliminary testing:

  • use swift-package as the target of the symlinks
  • replace the main.swift source file in swift-package with the one that you wrote for _swift-package-manager
  • add the CMake TSCBasic dependency to swift-package instead of _swift-package-manager, etc

Then we still have just the one tool, and one symlink fewer (and SwiftPM target) fewer.

When swift-package was added, it was intended to become the "home" subcommand for package management, and so I think this would fit in well with that.

swift-build, swift-package, swift-test, swift-package-collection, and swift-run are now
installed as symlinks to _swift-package-manager, which selects the correct tool
based on the path used to invoke it.
@owenv
Copy link
Contributor Author

owenv commented Apr 21, 2021

@abertelrud sure, that makes sense to me. I dropped the underscored target, but kept the ones for swift-build, swift-test, etc. since they're still useful when not using the install action.

@owenv
Copy link
Contributor Author

owenv commented Apr 21, 2021

@swift-ci please smoke test

@tomerd tomerd removed the next waiting for next merge window label Apr 21, 2021
Copy link
Contributor

@abertelrud abertelrud left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks @owenv!

@abertelrud
Copy link
Contributor

@abertelrud sure, that makes sense to me. I dropped the underscored target, but kept the ones for swift-build, swift-test, etc. since they're still useful when not using the install action.

Absolutely, I did not mean to suggest removing the existing executables, only not adding the new underscored one. The existing ones are very useful during debug builds (in fact I use them all the time when working on SwiftPM in Xcode, using the scheme that's autocreated for each of them — and I suspect others use them in this was as well).

Perhaps in the future once we can create arbitrary symlinks during a debug build (perhaps with a postbuild action) we'll be able to remove them, but that's a ways off.

@abertelrud
Copy link
Contributor

@owenv This looks ready to merge at this point. Thanks again!

@owenv
Copy link
Contributor Author

owenv commented Apr 22, 2021

@abertelrud thanks for reviewing! There's no rush, but do you mind merging on my behalf, I don't have commit access to this repo

@abertelrud
Copy link
Contributor

@abertelrud thanks for reviewing! There's no rush, but do you mind merging on my behalf, I don't have commit access to this repo

Oh, right. Sure thing. Thanks again for your work here!

@abertelrud abertelrud merged commit 4f8a377 into swiftlang:main Apr 22, 2021
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