Skip to content

[stdlib] Inject WinSDK module map using VFS #63042

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

Closed
wants to merge 2 commits into from

Conversation

stevapple
Copy link
Contributor

@stevapple stevapple commented Jan 15, 2023

Inject module map for WinSDK automatically to avoid modifying Windows SDK directly.

Thanks for @egorzhdan who mentioned this approach in #62248 (comment). I believe this is very useful for Windows, where the header files are scattered in many places and we have to inject multiple module maps.

The future direction is to fully eliminate manual file injections on Windows, so that:

  • The installer implementation can be largely simplified;
  • SDK installation will be much faster;
  • Another blocking issue for parallel Swift installations is removed;
  • Users will not need to “repair” Swift after VS upgrades.

I’m also going to move Windows-related files from stdlib/public/Platform to stdlib/public/Windows. Splitting stdlib/public/Platform can prevent its CMakeLists.txt from being messed up by files from distinct platforms, especially when we want to add new platforms and prove native experience on each of them.

@compnerd
Copy link
Member

Inject module map for WinSDK automatically to avoid modifying Windows SDK directly.

Injecting the module via the VFS is definitely something that I am interested in and had hoped to look into soon (after the 5.9 rebranch settles).

Thanks for @egorzhdan who mentioned this approach in #62248 (comment). I believe this is very useful for Windows, where the header files are scattered in many places and we have to inject multiple module maps.

The future direction is to fully eliminate manual file injections on Windows, so that:

  • The installer implementation can be largely simplified;

I'm not sure it can be "largely simplified". It will avoid the need for the current custom action, but it won't be any simpler.

  • SDK installation will be much faster;

Do you have any proof for this? I don't think that the speed will be changed at all.

  • Another blocking issue for parallel Swift installations is removed;

Care to explain how this impacts parallel installations? The modulemap is not tied to the compiler version but rather the SDK version. If anything, "it "largely complicates" the frontend instead.

  • Users will not need to “repair” Swift after VS upgrades.

I’m also going to move Windows-related files from stdlib/public/Platform to stdlib/public/Windows. Splitting stdlib/public/Platform can prevent its CMakeLists.txt from being messed up by files from distinct platforms, especially when we want to add new platforms and prove native experience on each of them.

The shuffling actually makes sense. Do you want to do the file shuffle first? That is fairly involved as is (it impacts ~3 repositories at the very least).

I think that the real improvements given by the VFS approach are:

  • allows us to not modify the VS/WinSDK installation
  • avoids the need for the elevated privileges during installation
  • can (at least temporarily) remove the need for a custom action
  • allows us to support newer SDKs/VS releases without changes
  • avoids the need to repair the Swift installation after VS changes

Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

This is really awesome! Removing the requirement to have admin privileges when installing the Swift toolchain is very helpful. Looks like there are a couple more modulemaps that would need to switch to the VFS injection mechanism to make that happen, but this is certainly a step in the right direction.

Please wait for @compnerd's approval before merging this PR.

@egorzhdan
Copy link
Contributor

@swift-ci please smoke test

@xedin xedin removed their request for review January 16, 2023 23:11
@compnerd
Copy link
Member

On further thought, I think that it might be a good idea to stage this in through the Swift build itself first. That ensures that we can test the approach in a few different environments for a bit before we flip to it globally. Basically, let us do the following order:

  1. move the files around
  2. switch the build system and infrastructure to use the VFS to build the toolchain
  3. migrate the client side to the VFS approach

@compnerd
Copy link
Member

I've taken a first pass at 2 and I think that I prefer a slightly different approach for this (3).

#63880 does step 2 (migrates the Swift compiler build away from the injected content). This is important since the build itself does a few contortions to build. Once the build and test cycle for Swift itself is stable, I think that doing the non-build scenario is much easier.

For step 3, which this patch partially does, I think I favour a different approach. I would rather use the LLVM functions to identify the toolchain and SDK locations. I've taken a first pass at that in #63887. It is also more complete, handling WinSDK, ucrt, and vcruntime modules as well as the APINotes.

Thanks for the inspiration on the approach here @stevapple!

@compnerd
Copy link
Member

compnerd commented Mar 9, 2023

@stevapple I think that this PR can be closed? There are two more sets of changes that I need to finish up to get this work completed (swift-driver, SPM), but those changes are not part of this PR in the first place.

@stevapple stevapple closed this Apr 15, 2023
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.

3 participants