Making sure explicit source parameter with IVS apis is added for restore #2692
Conversation
jainaashish
requested review from
nkolev92,
dtivel,
dominoFire,
zivkan and
heng-liu
|
@NuGet/nuget-client 🔔 |
| @@ -172,6 +172,16 @@ private List<SourceRepository> GetEffectiveSourcesCore(ISettings settings, IList | |||
| // Create a shared caching provider if one does not exist already | |||
| CachingSourceProvider = new CachingSourceProvider(packageSourceProvider); | |||
| } | |||
| else | |||
| { | |||
| foreach(var source in CachingSourceProvider.GetRepositories()) | |||
There was a problem hiding this comment.
When does this happen?
I think the caching provider is initialized in the regular restore case isn't it?
There was a problem hiding this comment.
this happens while creating restoreContext from DependencyGraphRetoreUtility here
Then we add the additional source being passed through restore here This additional source only matters for IVS/ template wizard APIs as in all other cases, this source is already coming from NuGet.config chain where as we just take the full list of sources because it does a restore (not installing a specific package).
There was a problem hiding this comment.
There really only is 1 correct way to provider sources to restore as implemented currently and that is through the package spec.
You basically allow the GetPackageSpecs to figure out the sources.
There is also another way to do it, through the RestoreArgs - This was kept for backwards compatibility when implemented and it's slowly been removed. Currently it's only productive usage is the Add package dotnet command.
If you look where the sources are set in RestoreArgs it's only tests + AddPackageReferenceCommand.
This here adds a new way to calculate the effective sources that works in an non-obvious way.
I think there are some unforeseen consequences here.
The CachingSourceProvider is shared per restore operation as you said. By adding this you are potentially overriding the configs configured by the user through MSBuild properties. You would always have the dg spec sources(which could be msbuild) and the config sources that they tried to override.
My fear is that this changes makes #1976 obsolete, as now the specified source will be respected in addition to the already defined project sources.
Hope this makes sense.
There was a problem hiding this comment.
Given the current scheme of things, I disagree that only correct way to provide sources is through GetPackageSpec because that doesn't cover command line arguments, explicit PMC arguments, or IVS parameters. These all are extension points which should be respected in similar way and shouldn't be discriminated.
The CachingSourceProvider is shared per restore operation as you said. By adding this you are potentially overriding the configs configured by the user through MSBuild properties. You would always have the dg spec sources(which could be msbuild) and the config sources that they tried to override.
With this PR, we aren't overriding the configs, instead adding the additional source which isn't part of config, there is different between the both.
For #1976, then I'm not sure if adding that warning was the right thing for #5763. But then we haven't really solved that issue, we just added a warning to tell consumers that it doesn't work.
I still don't see a reasoning of not doing this, may be I'm missing a bigger picture here. IMHO customers are making a conscious choice to install a package from a specific source which isn't part of their configs, knowing the consequences and we should allow it like we allow for packages.config. And there are legit customers reasons for doing that.
There was a problem hiding this comment.
Given the current scheme of things, I disagree that only correct way to provide sources is through GetPackageSpec because that doesn't cover command line arguments, explicit PMC arguments, or IVS parameters. These all are extension points which should be respected in similar way and shouldn't be discriminated.
This is exactly how it's implemented though.
We've time and time again wanted people to avoid using specific sources for PR restores because it breaks repeatability.
With this PR, we aren't overriding the configs, instead adding the additional source which isn't part of config, there is different between the both.
There are multiple ways the sources list is populated.
The sources here are coming from
In most cases this is populated from the setting sources.
You could've probably avoided the changes in the RestoreArgs.GetEffectiveSources by populating the Sources of the restore args.
But the same point would still stand in that case.
It would have unintended consequences where you'd have extra sources in the scenarios where someone has used the RestoreSources msbuild property.
For #1976, then I'm not sure if adding that warning was the right thing for #5763. But then we haven't really solved that issue, we just added a warning to tell consumers that it doesn't work.
I still don't see a reasoning of not doing this, may be I'm missing a bigger picture here. IMHO customers are making a conscious choice to install a package from a specific source which isn't part of their configs, knowing the consequences and we should allow it like we allow for packages.config. And there are legit customers reasons for doing that.
The reasoning was that we should not solve it as a "bad practices" thing. (At least for PMC) and the Package Manager UI.
There was a problem hiding this comment.
You could've probably avoided the changes in the RestoreArgs.GetEffectiveSources by populating the Sources of the restore args.
This will not work, since RestoreArgs sources are only consumed when they are also available in dgSpec source (which means there are also part of packageSpec sources) which is why I didn't add them there. Corresponding source - https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/RestoreArgs.cs#L154
There was a problem hiding this comment.
Read the below code;
It will add them, just think log a debug level that something "different" is happening.
There was a problem hiding this comment.
dah... this comment //DGSpecSources should always match the Sources confused me. :)
There was a problem hiding this comment.
Yours truly is to blame for that 😉
| @@ -67,7 +67,10 @@ public SourceRepository CreateRepository(PackageSource source, FeedType type) | |||
|
|
|||
| public void AddSourceRepository(SourceRepository source) | |||
| { | |||
| _cachedSources.TryAdd(source.PackageSource.Source, source); | |||
| if(_cachedSources.TryAdd(source.PackageSource.Source, source)) | |||
There was a problem hiding this comment.
I'm not confident about this change.
CreateRepository just above does not use the _repositories field.
In fact the _repositories field only seems to be used during initialization.
How does this affect later restores?
Would they end up using this extra source configured?
There was a problem hiding this comment.
CreateRepository and AddSourceRepository are different APIs if they were meant to be same then we wouldn't have needed an additional API.
To give you more perspective, CreateRepository is meant to create source repository instance on the fly which have nothing to do with current or later restore operation. But AddSourceRepository is meant to pass additional sources for that restore operation hence should be updated in the _repositories list so that all the consumers of this CachingSourceProvider have the option to consume it.
Also, it DOESN'T impact later restores because each restore always creates a new instance of CachingSourceProvider so it's only applicable for the current operation.
More so, you can look through the code to follow it through, there are additional source providers which should also give your more details about this. I've tested it thoroughly for different scenarios, but feel free to test it at your own capacity as well.
|
@nkolev92 #4693 is asking to use these one-off source (VSIX source, registry source provided through Template wizard) as part of normal restore. Which means they want to add bunch of Where as this PR (for bug# 7104) is to use VSIX source for the first time restore for a PR based project which will be consistent with PC approach. Neither PC supports VSIX sources for next restore, nor PR will. Hope this clarifies the confusion, otherwise we can also talk in person. |
|
@nkolev92 are you satisfied with the explanation or have any other concerns? I wish to merge it today or tomorrow if possible... @NuGet/nuget-client 🔔 |
|
This is the fix that @nkolev92 @jainaashish and I discussed the other day, i think. |
nkolev92
force-pushed
the
dev-asja-fix7104
branch
from
045483c to
e87dd77
Compare
|
Discussed offline. The fallouts are of this fix are the following:
Punting to the next release for a bit more involved fix. |
|
Closing given the length of time since the last message. We can re-open if we revisit this work later. |
Bug
Fixes: NuGet/Home#7104
Regression: No
Fix
Details: Currently we aren't consuming explicit source provided through IVS
InstallPackageor NuGet template wizard apis while restoring withPackageReference. Which these APIs only works with PR when source is already part of NuGet.config file.Testing/Validation
Tests Added: Yes
Reason for not adding tests:
Validation: