-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Use real SdkResolver APIs to set .NET environment variables for .NET TaskHost purposes #50091
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
base: main
Are you sure you want to change the base?
Conversation
…ations, report all VS-env validation errors in one batch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -189,7 +189,8 @@ | |||
Additionally, set the MinimumVSVersion for the installer UI that's required for targeting NetCurrent --> | |||
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. --> |
Copilot uses AI. Check for mistakes.
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These expected versions need checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These bumps were necessary IIRC because the higher MSBuild version below dragged them upwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of sync with msbuild: 4.0.4.0
<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" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of sync with msbuild: 4.0.2.0
<!-- <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" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of sync with msbuild: 4.1.5.0
<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" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of sync with msbuild: 6.0.1.0
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 static CachingWorkloadResolver _staticWorkloadResolver = new(); | ||
|
||
/// <summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.