msbuild: run manifest install only once #51348
Conversation
VS 2015 is not supported anymore and even have problems to compare versions.
|
15% reduction is really impressive! What's that in minutes? It's way more than I would have expected - is it that MSBuild is able to make forward progress without being blocked on the 'vcpkg install'? Or is it that the 'vcpkg install' backlog just clears more quickly? |
|
The latest run is with this change. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the MSBuild manifest-mode install flow so that dependency installation is executed via a single shared MSBuild invocation, aiming to avoid repeated vcpkg install work across multi-project builds and reduce CI build time.
Changes:
- Replaces the inline
ExecinVcpkgInstallManifestDependencieswith anMSBuildtask that runs a shared implementation target. - Adds inline MSBuild tasks to capture global property names (for
RemoveProperties) and to snapshot/restore selectedVCPKG_/X_VCPKG_environment variables. - Introduces a new
InstallImplementation.targetsproject containing the actual install implementation and stamp touch.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/buildsystems/msbuild/vcpkg.targets | Switches manifest install target to call a shared MSBuild implementation and adds imports for new inline tasks. |
| scripts/buildsystems/msbuild/support/RestoreEnvVars.task | Adds inline task to restore captured environment variables before running vcpkg. |
| scripts/buildsystems/msbuild/support/InstallImplementation.targets | New shared implementation target that runs vcpkg install and touches the stamp file. |
| scripts/buildsystems/msbuild/support/GetVcpkgEnvVars.task | Adds inline task to capture VCPKG-related environment variables into a property. |
| scripts/buildsystems/msbuild/support/GetGlobalProperties.task | Adds inline task to retrieve current MSBuild global property names for RemoveProperties. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BillyONeal
left a comment
There was a problem hiding this comment.
I still need to demonstrate that this doesn't break export (the same blocker as #41605 )
| Inputs="@(_ZVcpkgInstallManifestDependenciesInputs)" | ||
| Outputs="$(_ZVcpkgMSBuildStampFile)"> |
There was a problem hiding this comment.
I'm pretty sure that all the stuff about trying to make incremental be correct also needs to be moved to that other project. For example, this is declaring _ZVcpkgMSBuildStampFile as an output but is no longer creating it in the target.
There was a problem hiding this comment.
But it is created while the target is executed. I can only say that it did the right thing when testing it.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Can you explain why this could break export? According to the code you linked only a reference to the vcpkg.target file is exported and the behavior of this file does not change with this PR. |
I'm like 80% sure it will be fine but I need to verify that because we don't have tests for export, and export does interact with all this MSBuild bindings stuff. (For example, a potential bug would be that the resulting exported package doesn't have this new support directory inside which would likely explode) |
|
I have tested the export, the |
In the nuget export, the target file exists, but the support folder is not copied. But since there is no vcpkg.json file manifest mode is not enabled and the files are not loaded.
But we don't need to, we don't need the support files. We never want to run |
Greatly inspired by #41605 and works the same way.
This reduces our ci build times by 15%