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

[build-windows-toolchain] Build dependencies with vcpkg #60679

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

stevapple
Copy link
Contributor

This PR adds vcpkg to repositories checkout on Windows, and uses
the tool to build all the dependencies except ICU. In this way we largely simplify the workflow, avoid cloning and building manually, and resulting in a more standardized build process.

This PR also contains some improvements to make the script faster and more robust.

Depends on swiftlang/swift-installer-scripts#138, which adds the manifests.

Resolves SR-NNNN.

`vcpkg` can be used to build all the dependencies except for `ICU`.
This can both simplify and standardize the build process.
`vcpkg` is used to build dependencies on Windows.
- Add Swift vendor field.
- Clean up unused dependencies.
- Fix typo.
`cl` will assume source and execution to be in local charset, so
we should pass in `/utf-8` flags or prefer `clang-cl` instead.
Other dependencies are now managed by `vcpkg`, so we don't need
to handle them manually.
... to prevent multiple SDKs detected by freshly built Swift compiler.
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

No, the non-vcpkg build should be retained.

CC: @tomerd

@stevapple
Copy link
Contributor Author

stevapple commented Aug 21, 2022

No, the non-vcpkg build should be retained.

This is intended to replace manual builds. I agree with @tomerd and @millenomi that we should keep local development workflow the same as CI, so we’re not introducing a second practice.

I built the compiler locally and both binary size and performance is identical. We end up checking out 1 more but 4 less repositories, and the additional one is checked out by update-checkout. We also shrink the build script by ~15% and eliminate most Swift-irrelevant build steps, makes the build workflow on Windows identical to other platforms (where only ICU and CMark are manually built).

@compnerd
Copy link
Member

This does not match the official releases, please do NOT introduce the vcpkg based changes, they are not supported.

@stevapple
Copy link
Contributor Author

I can almost ensure that this won’t break the CI, and I don’t think Swift building process is something that can’t be changed.
Windows platform is long suffering from lack of build-script support, and the current build-windows-toolchain script is long and awkward for local development workflow.

Switching to vcpkg does not break anything. Current 3rd party CI won’t be broken either. We will eventually get fewer build steps and a more identical build result and everything — including sources and patches — is still under control. I don’t believe introducing a new build tool is something not acceptable.

Therefore, may I ask you to provide further context on why you’re against this change and whether there’s anyone else who knows about this proposal and their response. @compnerd

@compnerd
Copy link
Member

The build setup here is already shorter than the build-script + build-script-impl. If you want automation for local development, we already have that. If you want to help get #59276 merged, that is perfectly reasonable. The current "length" of the scripts is extended only because it actually aids in quick modifications, so it is desirable. The scripting in the aforementioned PR actually reduces the build of an entire toolchain into .\build.cmd that is more than reasonable and does not require any additional tooling outside of the VS tools. Diverging the CI from the official builds only complicates things further.

@etcwilde
Copy link
Contributor

@stevapple, I think the issue here is that @compnerd simply doesn't have time to provide the necessary maintenance for yet another tool impacting how things build on Windows, ensuring that everything behaves the same between CI and a local build. I think everyone agrees that it's important to be able to reproduce the results built on the Windows CI, easily.
The binaries might be the same now, but that will need to be checked with every update to ensure that no one has injected anything.

If you're doing development on Windows, would it be possible to copy these scripts and modify them to your needs in your fork?

@stevapple
Copy link
Contributor Author

stevapple commented Aug 22, 2022

The build setup here is already shorter than the build-script + build-script-impl.

build-script-impl is kind of legacy, and I don’t think we should compare to it regarding code lines. There’s not a single build script on Windows that has the identical functionalities to build-script. We cannot skip build products, cannot set build variants easily, and cannot adapt to different build environments. The last point is somehow the key point why build-swift-toolchain.bat and build.cmd cannot provide out-of-the-box support for local development, because they’re still assuming the exact environment as the CI (most notably VS 2019, x64 and UTF-8).

If you want automation for local development, we already have that.

I don’t think we can rely on scripts for local automation. If you really used build-windows-toolchain locally before, you will notice that it keeps reconfiguring everything, and, worst of all, resulting in the Swift runtime being rebuilt on every run. This never happens to build-script, so I won’t rely on the script any more until the bug is fixed. Instead I extracted every build step and rebuild anything I changed manually.

If you want to help get #59276 merged, that is perfectly reasonable.

Not against that, but I hope we can eliminate the number of Windows build scripts in respect of single source of truth rule. This will also help in providing more identical build results and making it easier to reproduce bugs either locally or with CI.

The current "length" of the scripts is extended only because it actually aids in quick modifications, so it is desirable.

This part is almost useless if you’re developing Foundation or developer tools😅

The scripting in the aforementioned PR actually reduces the build of an entire toolchain into .\build.cmd that is more than reasonable and does not require any additional tooling outside of the VS tools.

I don’t think we can easily judge the tradeoff between more build steps and fewer tools, but since we can eliminate several build steps with a single tool, I would prefer the latter.

Diverging the CI from the official builds only complicates things further.

No, I’ve never intended to diverge the CI from the “official builds”. This will be used on Swift CI to produce new toolchains. I know that official Windows toolchains are still produced by the Azure pipeline now, and if it is still the case for Swift 5.8, I am willing to help with the adaption.

@stevapple
Copy link
Contributor Author

stevapple commented Aug 22, 2022

@etcwilde Your points makes sense, so I’ll explain more on that.

@stevapple, I think the issue here is that @compnerd simply doesn't have time to provide the necessary maintenance for yet another tool impacting how things build on Windows, ensuring that everything behaves the same between CI and a local build. I think everyone agrees that it's important to be able to reproduce the results built on the Windows CI, easily.

I think this needs little maintenance effort. I’d like to talk more on what I do to ensure this won’t break in the future:

  • I added vcpkg to update-checkout and it checks out a tag instead of a branch, like what we do with CMake. vcpkg is released monthly and take release date as version tag, which is unlikely to be modified or changed.
  • By switching to vcpkg we don’t need to pass build flags to CMake manually, and instead we rely on manifest files from swift-installer-scripts. This makes local and CI builds far less likely to diverge, and makes updating a dependency super easy.
  • In a word, vcpkg makes it a lot easier to build identically to CI, which I believe is beneficial.

The binaries might be the same now, but that will need to be checked with every update to ensure that no one has injected anything.

We checked out a tag instead of a branch, so with basic trust on OSS development, I don’t believe that the source tree can be easily “injected”.

Even if we use master, vcpkg will respect manually overridden version, so we’re not likely to have diverged builds. vcpkg doesn’t rely on a remote registry like other package managers usually do. This brings me more confidence on its trustfulness:

  • Every package maintains a version history file which ties a version to a specific commit hash, and it is not allowed to be modified. (Not even possible because version number is bump-only!) If patches or dependencies are updated, the “port number” will add up instead of the version number, and port number can also be fixed.
  • vcpkg will check out the designated commit for build configuration, so it’s not possible to be changed “in the future”.

If you're doing development on Windows, would it be possible to copy these scripts and modify them to your needs in your fork?

I don’t think I should do that. As you mentioned, making CI and local builds identical will make life easier, so I proposed this change on the CI script. I’m not intended to maintain a custom “Windows port” of Swift. This makes no sense to me and I’m always contributing my efforts back to the mainline.

@compnerd
Copy link
Member

There’s not a single build script on Windows that has the identical functionalities to build-script

There actually is: #59276. There isn't much holding it up, it just needs a small change to the header. I can take care of that today.

We cannot skip build products, cannot set build variants easily, and cannot adapt to different build environments. The last point is somehow the key point why build-swift-toolchain.bat and build.cmd cannot provide out-of-the-box support for local development, because they’re still assuming the exact environment as the CI (most notably VS 2019, x64 and UTF-8).

This is intentional. It simplifies the build configuration - there is exactly one supported configuration. For local development this doesn't matter - incremental builds take ~1m to run through, so it does not particularly impact the developer productivity.

I don’t think we can rely on scripts for local automation. If you really used build-windows-toolchain locally before, you will notice that it keeps reconfiguring everything, and, worst of all, resulting in the Swift runtime being rebuilt on every run. This never happens to build-script, so I won’t rely on the script any more until the bug is fixed. Instead I extracted every build step and rebuild anything I changed manually.

I actually do use build.cmd for local development and do not see it taking longer than a few seconds to run through the incremental builds.

Not against that, but I hope we can eliminate the number of Windows build scripts in respect of single source of truth rule. This will also help in providing more identical build results and making it easier to reproduce bugs either locally or with CI.

This is unlikely to happen as there are differences - there are going to differences due to signing and the like that we do not wish to have in the local development flow. This is not necessarily a bad thing - having a single linear flow is far more valuable for ease of alternation. I've had to add features over time for supporting different configurations for testing and having the very cookie cutter approach makes things easier to work with.

This part is almost useless if you’re developing Foundation or developer tools😅

I work on both of those EXTENSIVELY. Foundation is the only piece that you should be using this script for. For the developer tools, at this point, the CMake based build systems are only supported for building distributions and bootstrapping, NOT for development. The only supported way to develop anything past Foundation is SPM. There should be documentation in each repository on how to build with SPM. In fact, any change to swift-driver, swift-package-manager, sourcekit-lsp should ALWAYS be built with SPM because that is the only way to run the test suite.

I don’t think we can easily judge the tradeoff between more build steps and fewer tools, but since we can eliminate several build steps with a single tool, I would prefer the latter.

The steps are cookie cutter and do not take very long, I disagree fundamentally here, and think that the additional steps are better than additional tools.

@stevapple
Copy link
Contributor Author

stevapple commented Aug 22, 2022

For local development this doesn't matter - incremental builds take ~1m to run through, so it does not particularly impact the developer productivity.

Wait… You said this doesn’t matter? It’s clear that these three scripts will all fail on my machine simply because I’m not using UTF-8. So do you think it’s reasonable to ask all Windows toolchain developers switch OS language to English and enable native UTF-8? This will break most of the applications I regularly use, including IM and many more. Not to say if anyone wants to build it on an ARM machine.

And I see you’re using a far more advanced machine than I😅 Reconfiguring Swift and rebuilding the runtime takes me about 10 minutes and it even doubles because we have another dedicated runtime build step. Is this something I and other developers must suffer in order to contribute to Swift?

@compnerd
Copy link
Member

Wait… You said this doesn’t matter? It’s clear that these three scripts will all fail on my machine simply because I’m not using UTF-8. So do you think it’s reasonable to ask all Windows toolchain developers switch OS language to English and enable native UTF-8? This will break most of the applications I regularly use, including IM and many more. Not to say if anyone wants to build it on an ARM machine.

Actually, yes, that is reasonable - Swift requires UTF8. However, if that is the cause of the issue, I don't see a problem with explicitly doing a chcp 65001 during the build.

And I see you’re using a far more advanced machine than I😅 Reconfiguring Swift and rebuilding the runtime takes me about 10 minutes and it even doubles because we have another dedicated runtime build step. Is this something I and other developers must suffer in order to contribute to Swift?

This sounds like a separate bug - we should not be rebuilding the runtime if there is no change. The reconfigure should also do nothing (it will run cmake twice and print a few lines).

@stevapple
Copy link
Contributor Author

Actually, yes, that is reasonable - Swift requires UTF8. However, if that is the cause of the issue, I don't see a problem with explicitly doing a chcp 65001 during the build.

chcp 65001 makes no sense and /utf-8 must be added. Of course this is easy to fix, but I urge you don’t assume others having the same environment as yours.

@stevapple
Copy link
Contributor Author

And I believe the main problem solved by vcpkg is — we’re not going to have diverged dependencies any more. It standardized our dependency build workflow, in a way similar to what we did on Linux — to consume a product of a package manager. vcpkg gives us even more control than we required on Linux, and it builds everything from source instead of getting a binary from a registry.

It’s unlikely that we can have a better solution than introducing vcpkg. winget, as a possible alternative, is still not capable of handling these dependencies for us.

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