Use real SdkResolver APIs to set .NET environment variables for .NET TaskHost purposes#50091
Conversation
…ations, report all VS-env validation errors in one batch
There was a problem hiding this comment.
Pull Request Overview
This PR updates the MSBuild SDK resolver to use proper SDK resolver APIs for setting .NET environment variables, specifically moving from using the deprecated DOTNET_EXPERIMENTAL_HOST_PATH property to the newer DOTNET_HOST environment variable through the SDK resolver's EnvironmentVariablesToAdd property.
Key changes:
- Migrates from properties-based to environment variables-based approach for passing .NET host information
- Updates MSBuild dependencies to newer versions to support the enhanced SDK resolver APIs
- Updates tests to verify the new environment variable approach instead of the old property-based approach
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| MSBuildSdkResolver.cs | Core implementation change from properties to environment variables for DOTNET_HOST |
| GivenAnMSBuildSdkResolver.cs | Test updates to verify environment variables instead of properties |
| Microsoft.DotNet.MSBuildSdkResolver.csproj | Updated dependency versions and error handling for dependency validation |
| Versions.props | Updated MSBuild and related package versions to support new APIs |
| build.sh/build.cmd | Added build summary logging flags |
src/Resolvers/Microsoft.DotNet.MSBuildSdkResolver/Microsoft.DotNet.MSBuildSdkResolver.csproj
Show resolved
Hide resolved
eng/Versions.props
Outdated
| <MicrosoftBuildVersion>17.15.0-preview-25377-103</MicrosoftBuildVersion> | ||
| <MicrosoftBuildLocalizationVersion>17.15.0-preview-25377-103</MicrosoftBuildLocalizationVersion> | ||
| <MicrosoftBuildMinimumVersion Condition="'$(DotNetBuildSourceOnly)' != 'true'">17.11.4</MicrosoftBuildMinimumVersion> | ||
| <!-- TODO: ensure the right version actually gets inserted --> |
There was a problem hiding this comment.
The TODO comment indicates uncertainty about version handling. This should be resolved before merging to ensure the correct MSBuild version is being used.
| <!-- TODO: ensure the right version actually gets inserted --> | |
| <!-- The correct MSBuild version is set via MicrosoftBuildVersion; ensure this matches the intended minimum version for non-source-only builds. --> |
| <ExpectedDependencies Include="Microsoft.Bcl.AsyncInterfaces, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="Microsoft.Build.Framework, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <ExpectedDependencies Include="System.Collections.Immutable, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <ExpectedDependencies Include="System.Memory, Version=4.0.1.2, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="Microsoft.Bcl.AsyncInterfaces, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Runtime.CompilerServices.Unsafe, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <ExpectedDependencies Include="System.Numerics.Vectors, Version=4.1.4.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <ExpectedDependencies Include="System.Buffers, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="Microsoft.Deployment.DotNet.Releases, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35" /> | ||
| <ExpectedDependencies Include="System.Buffers, Version=4.0.5.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Collections.Immutable, Version=9.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <!-- <ExpectedDependencies Include="System.Diagnostics.DiagnosticSource, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> --> | ||
| <ExpectedDependencies Include="System.IO.Pipelines, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Memory, Version=4.0.5.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Numerics.Vectors, Version=4.1.6.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <ExpectedDependencies Include="System.Runtime.CompilerServices.Unsafe, Version=6.0.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <ExpectedDependencies Include="System.Text.Encodings.Web, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Text.Json, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> |
There was a problem hiding this comment.
These expected versions need checking
There was a problem hiding this comment.
per @marcpopMSFT's comment here, use the versions from https://github.com/dotnet/msbuild/blob/vs17.15/src/MSBuild/app.amd64.config for checking these.
eng/Versions.props
Outdated
| <MicrosoftBclAsyncInterfacesToolsetPackageVersion>9.0.0</MicrosoftBclAsyncInterfacesToolsetPackageVersion> | ||
| <MicrosoftDeploymentDotNetReleasesToolsetPackageVersion>2.0.0-preview.1.24427.4</MicrosoftDeploymentDotNetReleasesToolsetPackageVersion> | ||
| <SystemBuffersToolsetPackageVersion>4.5.1</SystemBuffersToolsetPackageVersion> | ||
| <SystemCollectionsImmutableToolsetPackageVersion>8.0.0</SystemCollectionsImmutableToolsetPackageVersion> | ||
| <SystemCollectionsImmutableToolsetPackageVersion>9.0.0</SystemCollectionsImmutableToolsetPackageVersion> | ||
| <SystemMemoryToolsetPackageVersion>4.5.5</SystemMemoryToolsetPackageVersion> | ||
| <SystemReflectionMetadataLoadContextToolsetPackageVersion>8.0.0</SystemReflectionMetadataLoadContextToolsetPackageVersion> | ||
| <SystemReflectionMetadataToolsetPackageVersion>8.0.0</SystemReflectionMetadataToolsetPackageVersion> | ||
| <SystemTextJsonToolsetPackageVersion>8.0.5</SystemTextJsonToolsetPackageVersion> | ||
| <SystemReflectionMetadataLoadContextToolsetPackageVersion>9.0.0</SystemReflectionMetadataLoadContextToolsetPackageVersion> | ||
| <SystemReflectionMetadataToolsetPackageVersion>9.0.0</SystemReflectionMetadataToolsetPackageVersion> | ||
| <SystemTextJsonToolsetPackageVersion>9.0.0</SystemTextJsonToolsetPackageVersion> | ||
| <SystemThreadingTasksExtensionsToolsetPackageVersion>4.5.4</SystemThreadingTasksExtensionsToolsetPackageVersion> | ||
| <SystemResourcesExtensionsToolsetPackageVersion>8.0.0</SystemResourcesExtensionsToolsetPackageVersion> | ||
| <SystemResourcesExtensionsToolsetPackageVersion>9.0.0</SystemResourcesExtensionsToolsetPackageVersion> |
There was a problem hiding this comment.
These bumps were necessary IIRC because the higher MSBuild version below dragged them upwards.
baronfel
left a comment
There was a problem hiding this comment.
Quick pass through the versions in the vs17.15 branch of msbuild and noting what's out of sync
| <ExpectedDependencies Include="System.Numerics.Vectors, Version=4.1.4.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <ExpectedDependencies Include="System.Buffers, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="Microsoft.Deployment.DotNet.Releases, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35" /> | ||
| <ExpectedDependencies Include="System.Buffers, Version=4.0.5.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> |
| <ExpectedDependencies Include="System.Collections.Immutable, Version=9.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <!-- <ExpectedDependencies Include="System.Diagnostics.DiagnosticSource, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> --> | ||
| <ExpectedDependencies Include="System.IO.Pipelines, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Memory, Version=4.0.5.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> |
| <!-- <ExpectedDependencies Include="System.Diagnostics.DiagnosticSource, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> --> | ||
| <ExpectedDependencies Include="System.IO.Pipelines, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Memory, Version=4.0.5.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Numerics.Vectors, Version=4.1.6.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> |
| <ExpectedDependencies Include="System.IO.Pipelines, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Memory, Version=4.0.5.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Numerics.Vectors, Version=4.1.6.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <ExpectedDependencies Include="System.Runtime.CompilerServices.Unsafe, Version=6.0.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> |
| <ExpectedDependencies Include="Microsoft.Deployment.DotNet.Releases, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35" /> | ||
| <ExpectedDependencies Include="System.Buffers, Version=4.0.5.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> | ||
| <ExpectedDependencies Include="System.Collections.Immutable, Version=9.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a" /> | ||
| <!-- <ExpectedDependencies Include="System.Diagnostics.DiagnosticSource, Version=9.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" /> --> |
There was a problem hiding this comment.
This one is present, can uncomment it
|
for now we are going to roll back most of the actual version updates and direct usage of the MSBuild API in this PR, because bumping the MSBuild version here to 17.15 will make all of the SdkResolver tests in the Full-Framework CI leg fail. This is because those legs test the SdkResolver in situ - using To get the functionality in while not blocking the entire Full-Framework CI leg, we're instead going to use a cached reflection lookup to see if we can find the new factory function overload with the environment variable parameter. If we can, we will call that. If we cannot, we will call use the previous logic. We will also skip the tests that I added for this functionality. When there is an updated VS version available in the Full-Framework leg we will
In the meantime, the MSBuild team dogfooding this via the SDK will let us know if there are any regressions in this area. |
| private const string SdkResolverGlobalJsonPath = "SdkResolverGlobalJsonPath"; | ||
| private static CachingWorkloadResolver _staticWorkloadResolver = new(); | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
@dsplaisted I've made that change we talked about now - want to check me? I did load the updated MSBuild dll in an FSI session and made sure that the reflection-based lookup below does in fact resolve.
This uses the proper APIs for reporting Environment Variables to be set by the SdkResolvers, but because it requires a newer version of the MSBuild APIs and those APIs have brought increased dependency bumps we probably need to take a deeper look at the coordination involved here.