Load Microsoft.DotNet.MSBuildSdkResolver into default load context (MSBuild.exe only) #9439
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ladipro
force-pushed
the
9303-sdk-resolver
branch
from
7098a6f to
a31fbff
Compare
|
Should this go to 17.9? |
ladipro
commented
Dec 7, 2023
There was a problem hiding this comment.
Should this go to 17.9?
I believe so. Because the SDK is now attempting to NGEN the resolver against MSBuild.exe but the app.config is missing the requisite redirects, PerfDDRITs have flagged a regression in "invalid assembly count". Merging this into 17.9 would prevent that regression (however insignificant) to be shipped publicly.
ladipro
added a commit
to dotnet/sdk
that referenced
this pull request
Dec 8, 2023
…eline (#37381) As pointed out in dotnet/msbuild#9439 (comment), we have to be careful when changing the set of dependencies of the SDK resolver assembly as it may impact binding redirects in MSBuild's app.config. This PR adds a simple post-build step which compares the dependencies of the just-built assembly with a baseline. Co-authored-by: Rainer Sigwald <[email protected]>
AR-May
approved these changes
Dec 8, 2023
ladipro
force-pushed
the
9303-sdk-resolver
branch
2 times, most recently
from
aa68c3a to
e3b2480
Compare
|
I have retargeted the PR to 17.9. |
rainersigwald
approved these changes
Dec 13, 2023
ladipro
force-pushed
the
9303-sdk-resolver
branch
from
e3b2480 to
90997de
Compare
JanKrivanek
added a commit
to JanKrivanek/msbuild
that referenced
this pull request
Dec 22, 2023
…ntext (MSBuild.exe only) (dotnet#9439)" This reverts commit 6257b8e.
ladipro
added a commit
to ladipro/msbuild
that referenced
this pull request
Mar 12, 2024
…ntext (MSBuild.exe only) (dotnet#9439)" This reverts commit 6257b8e.
YuliiaKovalova
pushed a commit
that referenced
this pull request
Mar 14, 2024
…ntext" (#9857) (#9861) * Revert "Load Microsoft.DotNet.MSBuildSdkResolver into default load context (MSBuild.exe only) (#9439)" This reverts commit 6257b8e. * Bump version Co-authored-by: Ladi Prosek <[email protected]>
ladipro
added a commit
that referenced
this pull request
Sep 11, 2024
…ontext" (#10603) ### Context Now that dotnet/sdk#39573 has made it into VS, we can finally enable loading `Microsoft.DotNet.MSBuildSdkResolver` by assembly name, as opposed to file path, which makes it possible to use its NGEN image. ### Changes Made Re-did #9439 which had to be reverted because of the problematic dependency on `Newtonsoft.Json`. The .NET SDK resolver does not depend on `Newtonsoft.Json` anymore. All the other pieces are already in place: - `Microsoft.DotNet.MSBuildSdkResolver` is NGENed, both for MSBuild.exe and for devenv.exe. - `devenv.exe.config` contains binding redirects analogous to what's added in this PR. ### Testing Experimentally inserted into VS and verified that `Microsoft.DotNet.MSBuildSdkResolver` is no longer JITted, saving ~50-100 ms of MSBuild.exe startup time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #9303
Context
After a new version of
VS.Redist.Common.Net.Core.SDK.MSBuildExtensionsis inserted into VS, a native image forMicrosoft.DotNet.MSBuildSdkResolverwill be generated, both for devenv.exe and MSBuild.exe (see dotnet/installer#17732).We currently load SDK resolvers using
Assembly.LoadFromon .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.Loadto 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 forMicrosoft.DotNet.MSBuildSdkResolverunder a change-wave check.Testing
Experimental insertions.
Notes
Using
qualifyAssemblyin 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 samequalifyAssemblywill 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 ofMicrosoft.DotNet.MSBuildSdkResolverto 8.0.100.0.