Support sourceUri passed on nuget restore/install and dotnet restore or RestoreSources set for msbuild /t:restore command, nuget restore #4214

erdembayar · 2021-08-22T16:15:33Z

Bug

Partially Fixes: NuGet/Home#10948
Fixes: NuGet/Home#10961

Regression? Last working version: n/a

Description

Users can pass sources with -source option to the restore/install command. It happens to be -source support already works for package namespace without any code change due to previous code change, so I added unit tests to assert them, only product change made here is made for nuget.exe install and nuget.exe install -source scenario.
Following scenarios are included here:

nuget.exe restore -source
RestoreSources in csproj for PackageReference (msbuild/t:restore, nuget restore)
nuget.exe install -source
dotnet.exe restore -source
PMC install-package -source
PMC update-package -source

Following is not implemented, because NuGet/Home#11035 is not complete yet, so I created follow up NuGet/Home#11140.
dotnet add package -source

Also this PR addresses nuget install scenarios not covered by previous PRs(nuget restore).

nuget install packages.config
nuget install Contoso --Version 1.0.0

Currently this PR contains commits for #4212.

PR Checklist

src/NuGet.Clients/NuGet.CommandLine/Commands/InstallCommand.cs

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreNETCoreTest.cs

nkolev92

Looks good to me.

A few questions about the coverage.

I hope that we can simplify/shorten the tests though.

nkolev92 · 2021-08-24T01:52:37Z

test/EndToEnd/tests/PackageNameSpaceTests.ps1

@@ -326,6 +326,296 @@ function Test-VsPackageInstallerServices-PackageNamespaceInstall-WithMultipleFee
    }
 }

+function Test-PackageNamespaceInstall_Succeed


naming: Please add the project style in the name.

Why do we need this test here? Wasn't it covered in some previous work items?

Ok. I added project type "PC" into all of them. I chosen PC over PackagesConfig otherwise some test name would be very long.
It looks I didn't add 'PMC install' tests into End2end test previously, but I did add Apex tests with #4150

test/EndToEnd/tests/PackageNameSpaceTests.ps1

nkolev92 · 2021-08-24T02:02:01Z

test/EndToEnd/tests/PackageNameSpaceTests.ps1

+        Remove-Item $nugetConfigPath
+    }
+}
+
 # Create a custom test package 


There's no package installation tests for PR here, is that on purpose?

Now that I've gone through it all in detail, I see that you had a baseline for installation and -Source for installation.

Note that -Source is not supported in PackageReference

I believe PMC install for PR is synonymous with dotnet add package -source, but it's not implemented yet.

I believe PMC install for PR is synonymous with dotnet add package -source, but it's not implemented yet.

They are not equivalent.

Sources are ignored in PMC commands, there's a warning that gets logged when you try it.

Note that I'm not really suggesting we should add tests here.
I'm trying to provide as much context as possible to you, so that you can make sure you've covered all scenarios.

I created follow up issue for this. NuGet/Home#11187

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/RestoreCommandTests.cs

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreNETCoreTest.cs

erdembayar · 2021-08-25T15:00:30Z

@nkolev92 @kartheekp-ms
Please review.

nkolev92

Some test feedback, but looks great overall.

test/EndToEnd/tests/PackageNameSpaceTests.ps1

nkolev92 · 2021-08-25T18:28:33Z

test/EndToEnd/tests/PackageNameSpaceTests.ps1

+        Remove-Item $nugetConfigPath
+    }
+}
+
 # Create a custom test package 


I believe PMC install for PR is synonymous with dotnet add package -source, but it's not implemented yet.

They are not equivalent.

Sources are ignored in PMC commands, there's a warning that gets logged when you try it.

nkolev92 · 2021-08-25T18:34:04Z

test/EndToEnd/tests/PackageNameSpaceTests.ps1

+        Remove-Item $nugetConfigPath
+    }
+}
+
 # Create a custom test package 


Note that I'm not really suggesting we should add tests here.
I'm trying to provide as much context as possible to you, so that you can make sure you've covered all scenarios.

nkolev92 · 2021-08-25T18:45:27Z

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/RestoreNETCoreTest.cs

@@ -10854,6 +10854,191 @@ public async Task RestoreNetCore_WhenPackageNamespacesConfiguredAndNoMatchingSou
            Assert.Contains($"Installed {packageY} {version} from {pathContext.PackageSource}", result.AllOutput);
        }

+        [Fact]
+        public async Task RestoreCommand_NameSpaceFilter_PR_WithAllRestoreSources_Properies_Succeed()


Given that I understand well enough what SimpleTestPathContext does, I feel like we could have benefited from an addition to modify that particular nuget.config, instead of adding a new one.

Quite often I think that the global packages folder of the user is getting polluted, forgetting that we are relying on having multiple sources.

Just something to consider. We can extend SimpleTestPathContext when appropriate.

erdembayar marked this pull request as ready for review August 22, 2021 16:17

erdembayar requested a review from a team as a code owner August 22, 2021 16:17

erdembayar requested review from kartheekp-ms and nkolev92 August 23, 2021 16:32

nkolev92 reviewed Aug 23, 2021

View reviewed changes

erdembayar requested a review from nkolev92 August 24, 2021 00:31

nkolev92 reviewed Aug 24, 2021

View reviewed changes

erdembayar and others added 21 commits August 24, 2021 14:36

1

428e6fd

Adjust tests.

78dbeeb

Test

95b1f3c

Add install package scenario.

463246c

Fix install tests.

77d9dc7

Move namespace logic

e451729

Move package namespace logic more ahead than actual restore action.

73f820b

Add tests.

0baa1d9

Handle case insensetive case.

45525fc

Fix tests.

6069347

Remove --debug

6a8d8f2

Cleanup

a5e73c9

1

56d20cc

2

cc662a6

3

a7846dc

Address PR comment

cb5cc3f

Add project type in End2end tests.

cd27e32

Remove unrelated test

cd3bc75

Revert unneeded tests.

b2648b7

Address PR comments

6857024

Clean up.

2f48afd

erdembayar force-pushed the dev-eryondon-SourceUri5 branch from 15d635c to 2f48afd Compare August 24, 2021 21:36

erdembayar requested a review from nkolev92 August 25, 2021 02:27

Change comparison for international id.

c9a605b

nkolev92 approved these changes Aug 25, 2021

View reviewed changes

erdembayar added 2 commits August 25, 2021 17:18

International source id tests

2510b5c

Rename End2end test names

3e3a837

erdembayar mentioned this pull request Aug 26, 2021

Change from permissive mode to fullySpecified mode for PC project package namespace #4212

Merged

8 tasks

erdembayar added 4 commits August 26, 2021 08:33

Merge branch 'dev' into dev-eryondon-SourceUri5

fd91ecf

Add back test

9133e01

Rename tests

beb0169

Fix test

870dfd5

erdembayar merged commit cb411a3 into dev Aug 27, 2021

erdembayar deleted the dev-eryondon-SourceUri5 branch August 27, 2021 03:35

kartheekp-ms mentioned this pull request Oct 18, 2021

Add additional tests for package namespaces (PackageReference) restore NuGet/Home#11038

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sourceUri passed on nuget restore/install and dotnet restore or RestoreSources set for msbuild /t:restore command, nuget restore #4214

Support sourceUri passed on nuget restore/install and dotnet restore or RestoreSources set for msbuild /t:restore command, nuget restore #4214

erdembayar commented Aug 22, 2021 •

edited

Loading

nkolev92 left a comment

nkolev92 Aug 24, 2021

erdembayar Aug 24, 2021

nkolev92 Aug 24, 2021

erdembayar Aug 24, 2021 •

edited

Loading

nkolev92 Aug 25, 2021

nkolev92 Aug 25, 2021

erdembayar Aug 27, 2021

erdembayar commented Aug 25, 2021

nkolev92 left a comment

nkolev92 Aug 25, 2021

nkolev92 Aug 25, 2021

nkolev92 Aug 25, 2021

Support sourceUri passed on nuget restore/install and dotnet restore or RestoreSources set for msbuild /t:restore command, nuget restore #4214

Support sourceUri passed on nuget restore/install and dotnet restore or RestoreSources set for msbuild /t:restore command, nuget restore #4214

Conversation

erdembayar commented Aug 22, 2021 • edited Loading

Bug

Description

PR Checklist

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar commented Aug 25, 2021

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar commented Aug 22, 2021 •

edited

Loading

erdembayar Aug 24, 2021 •

edited

Loading