Move NuGet API usage to its own extension #824
Conversation
twsouthwick
force-pushed
the
tasou/nuget-hosted
branch
from
22609f8 to
3fd31c8
Compare
NuGet API may not be stable and loading an arbitrary version with MSBuild may cause problems. This change isolates those calls so we can pin it to our own version. This change does a couple of things: - Moves all NuGet usages to extension. This will now load in its own AssemblyLoadContext so we won't have any conflicts - Updates to the latest NuGet client API libraries - Adds a 'required' extension concept so that they will load in all cases even when loading is disabled. Fixes #817
twsouthwick
force-pushed
the
tasou/nuget-hosted
branch
from
3fd31c8 to
45a5328
Compare
| @@ -15,6 +15,10 @@ | |||
| "Extensions": { | |||
| "Source": "https://upgradeassistant.blob.core.windows.net/extensions/index.json", | |||
|
|
|||
| "required": [ | |||
There was a problem hiding this comment.
this naming feels a little confusing. almost feels like something that is default is required? or maybe we move the default extension under required?
There was a problem hiding this comment.
This is "required" because everything will need it. Default extensions are used to augment certain services but not needed everywhere. Long term, it may be good to define extensions on a "per command" usage; ie, identify commands that would only be used in analyze/upgrade/etc
| } | ||
|
|
||
| var list = new List<ExtensionInstance>(); | ||
| // Required extensions must load, otherwise they may be turned off | ||
| if (!opts.LoadExtensions) |
There was a problem hiding this comment.
Do we need a LoadExtensions option? I'm not sure it's necessary and if we removed it, this code would simplify a bit as default and required extensions wouldn't have to be distinguished anymore. The only downside I can think of to removing it is that users won't be able to disable any default extensions, but I haven't seen a use case where that would be necessary.
There was a problem hiding this comment.
When managing extensions, it adds extra time that this prevents. The extension management doesn't need all the random extensions people may be adding, but it will need the NuGet one
src/components/Microsoft.DotNet.UpgradeAssistant.Extensions/ExtensionAssemblyLoadContext.cs
Show resolved
Hide resolved
| private readonly Func<IUpgradeContext, IProject, INuGetReferences> _nugetReferenceFactory; | ||
| private readonly Func<IProjectFile, ITargetFrameworkCollection> _tfmCollectionFactory; | ||
|
|
||
| public Factories( |
There was a problem hiding this comment.
I think I'm overlooking something here. I'm trying to figure out how this type is instantiated. I see that it's registered as a transient DI service, but I don't see where the Funcs it needs as arguments are registered. Can you help me figure out where the nugetReferenceFactory and tfmCollectionFactory implementations come from?
There was a problem hiding this comment.
One of the beautiful features of Autofac: https://autofac.readthedocs.io/en/develop/resolve/relationships.html
src/components/Microsoft.DotNet.UpgradeAssistant.MSBuild/MSBuildProject.NuGetPackages.cs
Outdated
Show resolved
Hide resolved
| { | ||
| if (upgradeOptions.Project?.FullName is string fullname) | ||
| { | ||
| optionss.PackageSourcePath = Path.GetDirectoryName(fullname); |
There was a problem hiding this comment.
I believe the setting of PackageSourcePath got lost in this PR. It would explain why changing package sources in nuget.config doesn't work anymore.
There was a problem hiding this comment.
I got the same issue, 0.4.421302 will not follow my nuget.config but 0.2.241603 (a version that is before the above PR) will follow my nuget.config.
NuGet API may not be stable and loading an arbitrary version with MSBuild may cause problems. This change isolates those calls so we can pin it to our own version.
This change does a couple of things:
Fixes #817