-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Implement a NuGet-based SDK resolver #2850
Conversation
{ | ||
public override string Name => nameof(NuGetSdkResolver); | ||
|
||
public override int Priority => 2500; |
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.
To be explicit here for everyone reviewing, the DefaultSdkResolver
is 10,000 and DotNetMSBuildSdkResolver
is 5,000. So at 2,500 NuGetSdkResolver
will run before any others (that we ship).
else | ||
{ | ||
// This should never happen | ||
errors.Add("Package was supposed to be installed but wasn't"); |
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.
Move the 2 error messages to resources please.
Oh and the message above.
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.
I was having trouble with the wording, I can't see how the conditions would be hit in the first place. NuGet says it successfully restored the package but you have to do a secondary lookup of where it was put. Any suggestions for the messages?
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.
Not concerned much with the message if it shouldn't ever happen. Maybe just Error resolving package '{0}'. NuGet restored successfully but the package was not found.
Or something like that? Just think it should have the package name and be moved to resource.
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.
Done
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.
Many nits and questions, nothing very blocking.
I do want to see some internal tests set up to validate that this continues working in VS scenarios, with changing NuGet/Newtonsoft.Json versions as deployed. Don't want to find out about breakage too late.
src/Build/Microsoft.Build.csproj
Outdated
@@ -810,6 +819,7 @@ | |||
<Compile Include="Evaluation\LazyItemEvaluator.EvaluatorData.cs" /> | |||
<None Include="project.json" /> | |||
</ItemGroup> | |||
<ItemGroup /> |
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.
nit: remove
src/Build/project.json
Outdated
@@ -3,6 +3,8 @@ | |||
"net46": { | |||
"dependencies": { | |||
"Microsoft.VisualStudio.Setup.Configuration.Interop": "1.14.114", | |||
"NuGet.Commands": "4.5.0", | |||
"NuGet.Protocol": "4.5.0", |
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.
Do we need to get the NuGet team to add us to their automatically-drop-new-NuGet-assemblies PRs?
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.
I'm not sure what this is?
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.
They have a bot that creates PRs like dotnet/sdk#1851. I was wondering if we needed to get them to target us too. Hopefully not, since we don't need the very latest . . . until we adopt their version of the resolverEx thing.
/// <summary> | ||
/// The sub-folder under the Visual Studio installation where the NuGet assemblies are located. | ||
/// </summary> | ||
public const string PathToNuGetUnderVisualStudioRoot = @"Common7\IDE\CommonExtensions\Microsoft\NuGet"; |
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.
Is this documented? How will we know if it changes?
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.
I have no idea, maybe @emgarten knows?
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 is considered a well known location and it is defined in the VSIX manifest, we have it set here also: https://github.com/NuGet/NuGet.BuildTasks/blob/1d8af3499f94a32c1d128a42faceade39c1f4337/src/Microsoft.NuGet.Build.Tasks/ImportBeforeAfter/Microsoft.NuGet.ImportAfter.targets#L14
I would not expect this to change, if it did it would be very hard to find where it moved to.
#elif !CLR2COMPATIBILITY | ||
return Assembly.UnsafeLoadFrom(assemblyPath); | ||
#else | ||
return Assembly.LoadFrom(assemblyPath); |
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.
How do these loads work in the face of deployed NuGet DLLs that are a different version from what we built against? Is it different between Core and full framework?
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.
They are effectively assembly binding redirects. If we compile against 4.3 we can safely load 4.5 instead. As long as there are no breaking API changes, it should work.
src/Build/project.json
Outdated
@@ -3,6 +3,8 @@ | |||
"net46": { | |||
"dependencies": { | |||
"Microsoft.VisualStudio.Setup.Configuration.Interop": "1.14.114", | |||
"NuGet.Commands": "4.5.0", | |||
"NuGet.Protocol": "4.5.0", |
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.
Is the Newtonsoft.Json reference coming to us transitively through this? Since we use it explicitly I'd like to see it mentioned explicitly.
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.
Yes, it comes through NuGet.ProjectModel
Technically the version doesn't matter since we only load it from where NuGet is at. We only have one call which I highly doubt would ever be broken. But if you feel strongly about it...
public static Task<IReadOnlyList<RestoreResultPair>> RunWithoutCommit(string projectPath, string id, string version, ISettings settings, ILogger logger) | ||
{ | ||
// NuGet requires at least one framework, we use .NET Standard here just to get the API to do work. The framework is not actually used. | ||
List<NuGetFramework> targetFrameworks = new List<NuGetFramework> |
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.
make this a static readonly
field?
if (potentialResolvers.Count == 0) return resolvers; | ||
if (potentialResolvers.Count == 0) | ||
{ | ||
return resolvers.Count == 1 ? resolvers : resolvers.OrderBy(t => t.Priority).ToList(); |
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.
Was this optimization noticeably faster?
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.
I left it in to keep existing functionality. @AndyGerlicher had the optimization in there first, possibly by accident
{ | ||
result = (SdkResult) sdkResolver.Resolve(sdk, context, resultFactory); | ||
} | ||
catch (Exception e) when (sdkResolver is NuGetSdkResolver && e is FileNotFoundException) |
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.
Oof, this is specific. But I can't think offhand of a better way.
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.
Yeah we went back and forth on this. We're explicitly adding the resolver so we felt comfortable with the special case here. I'll add a comment to explain the reasoning
catch(Exception e) | ||
{ | ||
// MSB4242: The SDK resolver "{0}" failed to run. {1} | ||
loggingContext.LogWarning(null, new BuildEventFileInfo(sdkReferenceLocation), "CouldNotRunSdkResolver", sdkResolver.Name, e.Message); |
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.
What were we doing for this before? Is it ok that this is no longer fatal?
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.
Yes before it was a fatal error. When testing things out, a fatal build error when something was wrong with a resolver seemed like overkill. What do you think?
src/Build/project.json
Outdated
@@ -14,19 +16,26 @@ | |||
"netstandard1.5": { | |||
"dependencies": { | |||
"NETStandard.Library": "1.6.0", | |||
"NuGet.Commands": "4.5.0", | |||
"NuGet.Protocol": "4.5.0", |
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.
nit: since these are the same across frameworks it'd be nice to have them in a common node.
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.
Okay
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.
Looks good once Rainer's comments are addressed!
catch(Exception e) | ||
{ | ||
// MSB4242: The SDK resolver "{0}" failed to run. {1} | ||
loggingContext.LogWarning(null, new BuildEventFileInfo(sdkReferenceLocation), "CouldNotRunSdkResolver", sdkResolver.Name, e.Message); |
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.
Yes before it was a fatal error. When testing things out, a fatal build error when something was wrong with a resolver seemed like overkill. What do you think?
src/Build/project.json
Outdated
@@ -3,6 +3,8 @@ | |||
"net46": { | |||
"dependencies": { | |||
"Microsoft.VisualStudio.Setup.Configuration.Interop": "1.14.114", | |||
"NuGet.Commands": "4.5.0", | |||
"NuGet.Protocol": "4.5.0", |
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.
I'm not sure what this is?
src/Build/project.json
Outdated
@@ -3,6 +3,8 @@ | |||
"net46": { | |||
"dependencies": { | |||
"Microsoft.VisualStudio.Setup.Configuration.Interop": "1.14.114", | |||
"NuGet.Commands": "4.5.0", | |||
"NuGet.Protocol": "4.5.0", |
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.
Yes, it comes through NuGet.ProjectModel
Technically the version doesn't matter since we only load it from where NuGet is at. We only have one call which I highly doubt would ever be broken. But if you feel strongly about it...
src/Build/project.json
Outdated
@@ -14,19 +16,26 @@ | |||
"netstandard1.5": { | |||
"dependencies": { | |||
"NETStandard.Library": "1.6.0", | |||
"NuGet.Commands": "4.5.0", | |||
"NuGet.Protocol": "4.5.0", |
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.
Okay
src/Build/project.json
Outdated
"System.Collections.Immutable": "1.2.0", | ||
"System.Collections.NonGeneric": "4.0.1", | ||
"System.Diagnostics.Contracts": "4.0.1", | ||
"System.Diagnostics.FileVersionInfo": "4.0.0", | ||
"System.Diagnostics.Process": "4.1.0", | ||
"System.Diagnostics.TraceSource": "4.0.0", | ||
"System.IO": "4.1.0", |
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 had to be added explicitly because the reference to NuGet caused some unwanted upgrades.
|
||
public override SdkResultBase Resolve(SdkReference sdk, SdkResolverContextBase context, SdkResultFactoryBase factory) | ||
{ | ||
object parsedSdkVersion; |
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.
Why object?
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.
I've been waiting to hear when I can write a build script that works without requiring any specific machine state (think pristine Windows installation) by pulling MSBuild and all SDKs from NuGet. I'm specifically interested in the legacy .csproj, the new .csproj, and .vcxproj SDKs. @rainersigwald told me to wait for #1493 which has been inactive for eight months. Does this PR bring us any closer to that? Is progress being made and I'm just still out of the loop? |
@jnm2 Thanks for pointing out that that issue was super stale; I updated it a bit. This improves the situation for custom SDKs, which can now be acquired from NuGet. I believe there is no planned work to make the .NET Core SDK available via NuGet, so you must still deploy it separately (or get MSBuild through the Core SDK, which is a single unzippable entity). There's no planned work to make the installed C# or C++ targets available on NuGet, which doesn't change based on this--they do not use MSBuild Sdks so Sdk acquisition doesn't affect those scenarios. |
Thank you!
Who do I talk to? |
An SDK resolver that activates only when a user has specified a version. This resolver is ordered to come before the DotNetSdkResolver but if a user does not specify a version (the default), then the NuGet SDK resolver is skipped.
The NuGet SDK resolver can be disabled with an environment variable just in case its causing issues or a user wants to override its functionality.
NuGet assemblies are loaded at runtime from their installed location. They either come come [VisualStudioInstallRoot\Common7\IDE\CommonExtensions\Microsoft\NuGet or next to MSBuild.exe. There is also an environment variable that can be set to specify a folder to use instead.
I've also placed references to NuGet into private subclasses so that assemblies are not loaded unless they are needed. This means that if you have no version specified and no
global.json
with versions,Newtonsoft.Json
andNuGet.*.dll
are not loaded. You only pay JIT penalty if you're using the functionality and hopefully we'll make up that time later in the build through caching.Addresses item 6 in #2803
Related to #2278