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

Freeze Microsoft.DotNet.MSBuildSdkResolver assembly version #36733

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

ladipro
Copy link
Member

@ladipro ladipro commented Nov 8, 2023

MSBuild.exe currently spends a significant amount of time JITting Microsoft.DotNet.MSBuildSdkResolver and its dependencies, see dotnet/msbuild#9303 for details.

The most straightforward way to fix this is to load the assembly into the default load context using Assembly.Load rather than the load-from context where native images are not used. For this to work without creating a secondary AppDomain with a custom app.config, MSBuild needs to know the version of the resolver assembly statically - at its own build time. Since MSBuild and SDK insert into Visual Studio independently, we have basically two options:

  • Be OK with a temporary perf regression every time SDK inserts vNext into VS (or trying to make a combined SDK + MSBuild insertion with MSBuild compensating for the version bump). Note: The assembly load can be made such that it falls back to LoadFrom if the assembly identity doesn't match.
  • Freeze the version of the assembly. The exact version is not important as long as it's fixed. This should be safe because the assembly should have no other consumers outside of MSBuild.

There are other approaches but I consider them non-practical / unattractive:

  • Redesign the code flow so MSBuild always knows the version of the SDK it's going to run with in VS.
  • Update MSBuild.exe.config as a post-install step on the customer machine.

cc @joeloff @marcpopMSFT

Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Nov 8, 2023
@@ -1,6 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<!-- The version is frozen to allow MSBuild.exe loading it into the default load context -->
<AssemblyVersion>8.0.100.0</AssemblyVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be 9.0.100 since main is .NET 9?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just like dotnet/installer#17732, I would like this to appear in 8.0.2xx / VS 17.9. So the version from 8.0.2xx onwards would ideally be 8.0.200.0 or 8.0.100.0. But it's fine to make it 9.0.100 if you prefer that; any version will do as long as it's fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this is going to be in a separate instance of VS right. You could just set the version to $(VersionPrefix) maybe. That would select 8.0.100 or 8.0.200, etc. depending on the branch you're building.

Also, are there any risk with having the stable version? Does anything rely on loading the assembly dynamically?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if $(VersionPrefix) would help. I'm probably misunderstanding but having different branches produce different versions is specifically what I don't want. VS main currently has 8.0.100 of this assembly and what I basically want to avoid is having to bump the reference in MSBuild every time a new version of SDK is inserted. So my goal is for the version of the file in VS to never change, similar to how MSBuild has frozen its assembly versions to 15.1.0.0.

Also, are there any risk with having the stable version? Does anything rely on loading the assembly dynamically?

The file is loaded only by MSBuild, to my best knowledge.

Copy link
Member

Choose a reason for hiding this comment

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

Ok and if we have both 8.0.100 and 8.0.200 SDKs installed and multiple VS instances, they would all have 8.0.100 versioned resolver DLLs. These are Framework assemblies, we don't install them into the GAC - I'm just trying to think about what can go wrong with different assemblies having the same identity, but potentially be functionally different. These are getting loaded up from the VS instance local MSBUILD copy, so they shouldn't intererfere I think.

@ladipro ladipro merged commit 5e82c43 into dotnet:main Nov 20, 2023
16 checks passed
@ladipro
Copy link
Member Author

ladipro commented Nov 20, 2023

/backport to release/8.0.2xx

Copy link
Contributor

Started backporting to release/8.0.2xx: https://github.com/dotnet/sdk/actions/runs/6926793568

@ladipro ladipro deleted the freeze-sdkresolver-version branch November 20, 2023 07:21
ladipro added a commit to dotnet/msbuild that referenced this pull request Dec 13, 2023
…SBuild.exe only) (#9439)

Fixes #9303

### Context

After a new version of `VS.Redist.Common.Net.Core.SDK.MSBuildExtensions` is inserted into VS, a native image for `Microsoft.DotNet.MSBuildSdkResolver` will be generated, both for devenv.exe and MSBuild.exe (see dotnet/installer#17732).

We currently load SDK resolvers using `Assembly.LoadFrom` on .NET Framework, which disqualifies it from using native images even if they existed. This PR makes us use the native image.

### Changes Made

Added a code path to use `Assembly.Load` to load resolver assemblies. The call is made such that if the assembly cannot be found by simple name, it falls back to loading by path into the load-from context, just like today. The new code path is enabled only for `Microsoft.DotNet.MSBuildSdkResolver` under a change-wave check.

### Testing

Experimental insertions.

### Notes

Using `qualifyAssembly` in the app config has the advantage of keeping everything _field-configurable_, i.e. in the unlikely case that a custom build environment will ship with a different version of the resolver, it will be possible to compensate for that by tweaking the config file. The disadvantage is that the same `qualifyAssembly` will need to be added to devenv.exe.config because .pkgdef doesn't support this kind of entry, to my best knowledge. It should be a one-time change, though, because [we have frozen the version of `Microsoft.DotNet.MSBuildSdkResolver` to 8.0.100.0](dotnet/sdk#36733).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants