Ensure that MSBuild integration bootstraps Vcpkg#41605
Conversation
|
We discussed this and confirm that we would be happy to make this work in MSBuild if a solution to the concurrency problem can be found and the perf impacts aren't too significant.
We still support back to VS2015 so this ends up being kind of a problem. On older MSBuild does this 'explode' or just fail to bootstrap? I am somewhat concerned with the dependency on |
|
Thanks for taking a look. The reason I structured the change the way I did was to 'firewall' the new stuff. Everything in the edited files (
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
OK, this means no blocker to merging it. I still have reservations about using CodeTaskFactory or RoslynCodeTaskFactory; this pattern has exploded on me on a number of occasions. But I think if that ends up being a serious issue we could just revert this. I asked for one more confirmation from the other maintainers before landing this, probably Tuesday of next week |
BillyONeal
left a comment
There was a problem hiding this comment.
I need to check: We really only want this to run in the "git clone" vcpkg deployment paradigm
Ah, interesting. I just looked at the VS redistribution of 'vcpkg', and it doesn't have |
|
I'll convert to a draft to prevent this from being completed (is there a better way? I'm more familiar with ADO than GitHub), until I've handled the |
684f35a to
3186b0a
Compare
|
Accommodated the 'bootstrap' file not being present and a few copy edits in the second commit. I rebased onto the latest origin/master, and buddy tested with and without the bootstrap file. |
There was a problem hiding this comment.
Pull request overview
This PR updates the vcpkg MSBuild integration so that, in manifest mode on Windows, vcpkg can be automatically bootstrapped in a way that avoids multi-project parallel build concurrency issues (by leveraging MSBuild’s “same project + same global properties runs once” guarantee).
Changes:
- Adds a
VcpkgAutoBootstrapopt-out property (defaulting totrue) for manifest-mode builds. - Conditionally imports a new bootstrap orchestration target that triggers before
VcpkgInstallManifestDependencies. - Introduces a custom MSBuild task to enumerate global property names and then invokes a nested MSBuild call with a controlled global-property set to ensure the bootstrap runs only once.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/buildsystems/msbuild/vcpkg.targets | Conditionally imports bootstrap support targets for eligible builds. |
| scripts/buildsystems/msbuild/vcpkg.props | Adds VcpkgAutoBootstrap property default. |
| scripts/buildsystems/msbuild/support/GetGlobalProperties.task | Adds a RoslynCodeTaskFactory task to read global properties from the build engine. |
| scripts/buildsystems/msbuild/support/Bootstrap.targets | Adds the VcpkgBootstrap target that orchestrates the “run once” bootstrap via nested MSBuild. |
| scripts/buildsystems/msbuild/support/BootstrapImplementation.targets | Adds the actual bootstrap implementation target that runs bootstrap-vcpkg.bat. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Oh, hey. Some nice due diligence fixes from Copilot. Nice. Moderate sidebar/discussion: re: microsoft/vcpkg-tool#1913. I don't think this fix would help there. This fix is ensuring that bootstrapping runs, by using MSBuild threading guarantees to avoid contentions. "bootstrapping" is tricky because there's nothing present to help with contentions (because you're bootstrapping), so the only contention prevention we can use is MSBuild itself and that's what this fix uses. From the looks of things (and I'm a little rusty/out-of-the-loop), 1913 is talking about 'installation' contention, that's always been the job of vcpkg-tool. A similar approach to this fix could move the contention prevention 'out' from vcpkg-tool to MSBuild, but I'm not sure that would help a great deal†, and it looks like a really tricky fix. † - I suppose MSBuild might have the build-graph fidelity to know that the 'targets' would block, so might be able to do other work. The current contention blocking in vcpkg-tool will simply manifest as lots of long-running tasks in MSBuild, which might prevent MSBuild from making other progress..? |
|
Yes, this PR currently does not help with microsoft/vcpkg-tool#1913. |
Yes this is what I meant. Instead of having all the MSBuilds launch different vcpkgs that all block/poll, this tech would let MSBuild do the deduplication itself. (But this PR itself doesn't do that yet) |
|
@BillyONeal What is the status of this PR from your side? |
|
One problem I currently have with this approach and |
Other stuff keeps getting in the way but it's on the todo list somewhere :( |
|
@BillyONeal Can we simply drop VS2015 support? That would make some things simpler. VS 2015 is out of extended support (https://learn.microsoft.com/en-us/lifecycle/products/visual-studio-2015) |
|
Related: MicrosoftDocs/vcpkg-docs#573 |
BillyONeal
left a comment
There was a problem hiding this comment.
Thank you and sorry it took so long to land this!
Fixes #23366
The current MSBuild integration doesn't bootstrap VCPkg when running in 'manifest' mode - meaning that consumers have to manually run, say,
bootstrap-vcpkg.batbefore building, or the build fails with errors. #23366 tracks this and calls out the problems in runningbootstrap-vcpkg.bat- just adding a target that runsbootstrap-vcpkg.batcan cause concurrency problems on parallel, multi-project builds. When building in parallel, multiple MSBuild 'engines' are invoked for the multiple projects, and multiple could attempt to runbootstrap-vcpkg.batconcurrently, andbootstrap-vcpkg.batdoesn't protect against concurrent execution, resulting in errors writing files. Under CMake this isn't a problem since the CMake integration runs during the CMake 'configuration' phase, which is executed sequentially. But MSBuild doesn't have a similar, single threaded equivalent.MSBuild does provide a threading guarantee that we can use - it guarantees that an MSBuild task for a given project, and a given set of global properties will run once (called out in the documentation for the MSBuild task). So if we invoke an MSBuild task with a global property of the Vcpkg root to bootstrap then MSBuild will ensure that it is only run once, blocking all other MSBuild engines until the bootstrap is complete. And that's what this PR does.
There's a few pieces to this change:
VcpkgAutoBootstrapproperty and default it totrue. This allows consumers to opt-out of the functionality. I'm fine with defaulting tofalse, and making it opt-in if a more cautious roll-out is needed.scripts/buildsystems/msbuild/vcpkg.targetsI conditionally importscripts/buildsystems/msbuild/support/Bootstrap.targetsthat contains the bulk of the implementation. By putting the logic in a separate 'targets' file, then it can be 'firewalled' a little. The conditionality requires:IBuildEngine6.GetGlobalProperties, which was introduced in MSBuild 16.5. The MSBuild 16.5 release notes don't call this out, I binary searched the "Microsoft.Build.Framework" NuGet packages, and 16.5 is the first version to containIBuildEngine6.scripts/buildsystems/msbuild/support/Bootstrap.targetsprovides aVcpkgBootstraptarget that marks itself asBeforeTargets="VcpkgInstallManifestDependencies"to ensure that Vcpkg is bootstrapped before it is used. The target also configuresInputsandOutputsappropriately, so that when bootstrapped the target won't be run, and the added cost is minimal. The implementation uses a custom task to get the current global property names, and then invokesMSBuildto run theVcpkgBootstrapImplementationremoving all global properties and setting the_ZVcpkgRootproperty. MSBuild will only run a singleVcpkgBootstrapImplementationinstance, and so the work thatVcpkgBootstrapImplementationdoes will only run once.scripts\buildsystems\msbuild\support\GetGlobalProperties.taskprovides the implementation of the custom task to get the global properties. The task has a single output parameter -GlobalPropertyNames- which is a semi-colon delimited list of the global property names, that theVcpkgBootstraptarget specifies as theRemovePropertiesparameter toMSBuild.I've been using an implementation of this fix in my consuming repo for a week or so, and I've buddy tested this fix by forcing a long delay in
bootstrap-vcpkg.bat. On a build where vcpkg.exe isn't present, theVcpkgBootstraptarget is 'built' by multiple projects (showing that there's contention by default), but theVcpkgBootstrapImplementationtask is only run once (so MSBuild is protecting it). On an incremental build, theVcpkgBootstraptask is skipped entirely, thanks to the MSBuild up-to-date checks.