Fix dotnet add package doesn't work with relative path source or when version is not specified #3783

erdembayar · 2020-12-01T23:17:57Z

Bug

Fixes: Issue#5127
Regression: No

Last working version:
How are we preventing it in future:

Fix

Details: Currently if user add package with dotnet tool from cli with custom source feed(local, remote) as argument then it doesn't work unless user supply exact version. Also this behavior changes between V3 and V2 format sources.

Below is whole steps:
Create a folder named 'Domain'
Create a folder named 'App'

Inside of Domain, run these commands:

dotnet new classlib
dotnet restore
dotnet pack --include-symbols --output ..\Mysource
Inside of App, run these commands:

dotnet new classlib
dotnet restore
dotnet add package Domain -s ..\Mysource

Don't forget to delete Domain package from local cache after successful operation, otherwise always success regardless of input next time.

V3 source:
Absolute path:
dotnet add package Domain3 -s C:\NuGetProj\IssueRepro\5127\Mypackages3 -v 1.0.0 works

dotnet add package Domain3 -s C:\NuGetProj\IssueRepro\5127\Mypackages3 error: There are no versions available for the package 'Domain3'. (Fixed)

Relative path:
dotnet add package Domain3 -s ..\Mypackages3 -v 1.0.0 works
dotnet add package Domain3 -s ..\Mypackages3 error: There are no versions available for the package 'Domain3'.
(Fixed)

V2 source:
Absolute path:
dotnet add package Domain2 -s C:\NuGetProj\IssueRepro\5127\Mysource2 -v 1.0.0 works.

dotnet add package Domain2 -s C:\NuGetProj\IssueRepro\5127\Mysource2 error: There are no versions available for the package 'Domain2'. (Fixed)

Relative path:
dotnet add package Domain2 -s ..\Mysource2 -v 1.0.0 error: NU1101: Unable to find package Domain2. No packages exist with this id in source(s): ..\Mysource2, nuget.org
error: Package 'Domain2' is incompatible with 'all' frameworks in project 'C:\NuGetProj\IssueRepro\5127\App2\App2.csproj'.
(Fixed)

dotnet add package Domain2 -s ..\Mysource2 error: There are no versions available for the package 'Domain2'.
(Fixed)

Please note: To make this code consistent with other dotnet command with user supplied custom source feeds in this fix also replaces dgSpec sources with user supplied custom source feeds.

For example below replaces source feeds in dgSpec file;
dotnet restore /p:restoreSources="C:\NuGetProj\IssueRepro\5127\Mypackages3;C:\NuGetProj\IssueRepro\5127\Mysource2"

Testing/Validation

Tests Added: Yes
Reason for not adding tests:
Validation: Manual from VS and patched sdk5.

zkat

This eyeballs fine to me. I would like to see tests specific to this scenario before approving, though.

erdembayar · 2020-12-02T01:04:24Z

This eyeballs fine to me. I would like to see tests specific to this scenario before approving, though.

@zkat
Sure, I'll add unit test by tomorrow.

nkolev92

You won't know if you are the correct thing if you don't test it.

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

erdembayar · 2020-12-03T01:32:17Z

This eyeballs fine to me. I would like to see tests specific to this scenario before approving, though.

@zkat
Please check I added 2 unit test for successful paths. Can't think of actual fail case specific to this case.

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatTestUtils.cs

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatAddPkgTests.cs

nkolev92

Couple of more suggestions/questions.

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/AddPackageCommandUtility.cs

nkolev92 · 2020-12-04T22:21:40Z

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/AddPackageCommandUtility.cs

@@ -88,9 +88,10 @@ public static async Task<NuGetVersion> GetLatestVersionFromSourceAsync(PackageSo
        /// Returns the PackageSource with its credentials if available
        /// </summary>
        /// <param name="requestedSources">Sources to match</param>
+        /// <param name="additionalSources">Additional user supplied source feeds as argument</param>


I don't think they are supposed to be additional, but rather they are supposed to replace the existing sources.

I believe it's treated as addition into existing sources user already setup on project.
Here below screenshot case it already has nuget.org as source and also Mysource which is I passed as argument on AddPackageReferenceCommandRunner.cs#L347. Maybe user can use this one as alternative sourcefeed for just once during restore.

I don't think this is a technical question.

--source for restore completely overwrites the sources.

dotnet add package should work the same.

There are docs that describe this behavior as well: https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-add-package .

@nkolev92
Can you suggest name for it?
How about Overriding source or Replacing source, New sources?

The sources in the restore request are not used at all.
The sources in the dg spec are used

NuGet.Client/src/NuGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/RestoreArgs.cs

Lines 129 to 144 in 6562ef3

internal List<SourceRepository> GetEffectiveSources(ISettings settings, IList<PackageSource> dgSpecSources)

{

if (settings == null)

{

throw new ArgumentNullException(nameof(settings));

}

var values = settings.GetConfigRoots();

if (dgSpecSources != null)

{

values.AddRange(dgSpecSources.Select(e => e.Source));

}

var cacheKey = string.Join("|", values);

return _sourcesCache.GetOrAdd(cacheKey, (root) => GetEffectiveSourcesCore(settings, dgSpecSources));

}

.

Don't think you'd need to change it there. It'd be difficult to do without consequences, so I'd just change in the package spec that you are passing for restore.

@nkolev92
Yes, can we talk 5-10 min today. Early is better.

@nkolev92
I tested example on https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-add-package:
dotnet add package Microsoft.AspNetCore.StaticFiles -s https://dotnet.myget.org/F/dotnet-core/api/v3/index.json

It doesn't change the source property on dgspec file. It's only used as temporary thing. It's not persisted, I can't say --source completely overwrites the sources.
It's still

"sources": { "https://api.nuget.org/v3/index.json": {} },

Only sources listed in dotnet nuget list source or nuget.config go in there.
So when we use --source it's temporary source lets restore package from that source and persist copy of package in cache, but source details not saved. So better to call it temp source.

It looks one problem with current behavior of --source we can't do repeatable restore/build if local cache is cleared even though we successfully added package from custom relative source unless that one is added into source list.

It looks one problem with current behavior of --source we can't do repeatable restore

That's exactly why source is not supported in the PM UI & why you can't select a subset of sources when doing restore, it always uses them all.

I think it's ok if you overwrite the source in this case.
If the customer gets themselves in a non-repeatable position, it is what it is.
They can already do that with /p:RestoreSources when they pass something different from what's specified in their nuget.config.

It doesn't change the source property on dgspec file. It's only used as temporary thing. It's not persisted, I can't say --source completely overwrites the sources.

I'm confused what this means. It doesn't work at all, it's disregarding it, which is what you are trying to fix.

I think it's ok if you overwrite the source in this case.

@nkolev92

I think overwriting is not good idea, maybe append is better.

First of all let's run this:
dotnet restore /p:restoreSources="C:\NuGetProj\IssueRepro\5127\Mypackages3;C:\NuGetProj\IssueRepro\5127\Mysource2"

Csproj file content: <ItemGroup> <PackageReference Include="Domain3" Version="1.0.0" /> <PackageReference Include="Domain2" Version="1.0.0" /> </ItemGroup>

RestoreSuccessful

and source changes to below:

"sources": { "C:\\NuGetProj\\IssueRepro\\5127\\Mypackages3": {}, "C:\\NuGetProj\\IssueRepro\\5127\\Mysource2": {} },

Another question is do we write relative path or absolute path in case user passed relative path for source?

"sources": { "..\Mysource2": {} },

or

"sources": { "C:\\NuGetProj\\IssueRepro\\5127\\Mysource2": {} },

Overwrite

By default usually we have this source:
"sources": {
"https://api.nuget.org/v3/index.json": {}
},

dotnet add package Domain2 -s ..\Mysources2

After overwrite we going to have

"sources": { "..\Mysources2": {}, },

dotnet add package Domain3 -s ..\Mypackages3

After yet another overwrite we going to have

"sources": { "..\\Mypackages3": {}, },

As you can see it's not great since every action overwrites the previous source.

Append.

By default usually we have this source:
"sources": {
"https://api.nuget.org/v3/index.json": {}
},

dotnet add package Domain2 -s ..\Mysources2

After append we going to have

"sources": { "..\Mysources2": {}, "https://api.nuget.org/v3/index.json": {} },

dotnet add package Domain3 -s ..\Mypackages3

After append we going to have

"sources": { "..\Mysources2": {}, "..\\Mypackages3": {}, "https://api.nuget.org/v3/index.json": {} },

This looks better, because it looks similar to RestoreSuccessful.
Only issue is we have "https://api.nuget.org/v3/index.json": {} which is not in RestoreSuccessful.

Please give your thought?

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/AddPackageCommandUtility.cs

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatAddPkgTests.cs

erdembayar · 2020-12-05T02:41:41Z

Couple of more suggestions/questions.

@nkolev92
It seems I misdiagnosed the problem. I'll change the PR.

nkolev92 · 2020-12-08T02:46:30Z

I imagine the source problem still stands right?
Which issue will this PR target?

erdembayar · 2020-12-08T17:22:46Z

I imagine the source problem still stands right?
Which issue will this PR target?

Out of 8 scenarios described above, now 6 is working. But last 2(V2 relative paths) need bit more work. Only reason V3 works for relative path is if it can't determine file path type then it defaults to V3.
I'll check 1-2 more ideas if I can make V2 relative paths work, then I'll talk with you.

nkolev92 · 2020-12-08T18:54:22Z

My best guess is that the source + weird error message about supporting all are orthogonal.

One might prevent you from hitting the other, but if that's the case you should solve the problems in order.
If you think there are multiple problems, solve them individually and PR them one by one.

erdembayar · 2020-12-08T19:30:25Z

My best guess is that the source + weird error message about supporting all are orthogonal.

One might prevent you from hitting the other, but if that's the case you should solve the problems in order.
If you think there are multiple problems, solve them individually and PR them one by one.

Main problem is when we pass "..\Mysource" it doesn't know it was local source type then it default to false. Maybe we can add step to check if Dir.Exists("..\Mysource") then make it local = true. That will solve relative path problem.

nkolev92 · 2020-12-08T19:38:54Z

@erdembayar

Look into

NuGet.Client/src/NuGet.Core/NuGet.Build.Tasks/BuildTasksUtility.cs

Line 703 in 3ef6cd3

    
           () => sourcesOverride?.Select(MSBuildRestoreUtility.FixSourcePath).Select(e => UriUtility.GetAbsolutePath(projectDirectory, e)).ToArray(),

.

We already do that in restore...if we're doing it different in other parts of the code, restore contains a tried and proven concept.

erdembayar · 2020-12-09T21:45:39Z

@erdembayar

Look into

NuGet.Client/src/NuGet.Core/NuGet.Build.Tasks/BuildTasksUtility.cs

Line 703 in 3ef6cd3

() => sourcesOverride?.Select(MSBuildRestoreUtility.FixSourcePath).Select(e => UriUtility.GetAbsolutePath(projectDirectory, e)).ToArray(),

.
We already do that in restore...if we're doing it different in other parts of the code, restore contains a tried and proven concept.

Ok. Fixed all possible paths and added 16 unit tests for covering them.

erdembayar · 2020-12-11T00:09:10Z

@nkolev92 @zkat
Please review again.

erdembayar · 2020-12-15T17:51:56Z

@NuGet/nuget-client please review.

nkolev92 · 2020-12-16T02:30:30Z

Just wanted to make sure the comment doesn't get missed, see #3783 (comment).

erdembayar · 2020-12-18T02:20:55Z

Just wanted to make sure the comment doesn't get missed, see #3783 (comment).

I replied, can you check?

erdembayar · 2020-12-21T19:39:02Z

Just wanted to make sure the comment doesn't get missed, see #3783 (comment).

@nkolev92
Please check now.
Instead of adding source now replacing them and wrote several unit test to check that behavior.

nkolev92

I'd separate the --source fix from the other fixes.

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/AddPackageCommandUtility.cs

src/NuGet.Core/NuGet.Protocol/LocalRepositories/FindLocalPackagesResourceV3Provider.cs

erdembayar · 2021-01-06T22:13:40Z

@nkolev92
It's ready to review. It's already passing all tests, I just committed small changes. I added source relative path unit tests into Dotnet.Integration.Test.
But I added source absolute path unit tests into Xplat.FuncTest, because previous one is slow than this one and I don't want to make it more slow by adding tests that can run faster elsewhere.

nkolev92

Minor changes/comments.

A few suggestions to clean-up the code and make it easier to follow.

nkolev92 · 2021-01-07T02:56:05Z

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

@@ -102,6 +102,22 @@ public async Task<int> ExecuteCommand(PackageReferenceArgs packageReferenceArgs,

            var originalPackageSpec = matchingPackageSpecs.FirstOrDefault();

+            // Convert relative path to absolute path if there is any
+            List<string> sourcePaths = new List<string>();


naming: sources, not sourcepaths. These can be http sources, not necessarily paths.

Ok. Renamed to sources.

nkolev92 · 2021-01-07T02:56:57Z

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

@@ -334,7 +350,7 @@ public static async Task<NuGetVersion> GetLatestVersionAsync(PackageSpec origina
                    MachineWideSettings = new XPlatMachineWideSetting(),
                    GlobalPackagesFolder = packageReferenceArgs.PackageDirectory,
                    PreLoadedRequestProviders = providers,
-                    Sources = packageReferenceArgs.Sources?.ToList()
+                    Sources = sourcePaths


tbh, you can cut this. It's not used anyways. We should probably mark it as not used.

Added comment here for why we're not passing value down here.

nkolev92 · 2021-01-07T02:57:31Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

@@ -453,6 +461,372 @@ public void TestCommandInvalidArguments(string command, int badCommandIndex)
            }
        }

+        [Theory]
+        [InlineData("net5.0")]


nit: Doesn't matter for now, but you don't need a theory if you only have 1 test case. Use a fact instead.

Initially I was thinking about testing many frameworks as much possible then considering heavy taxing on testing system I just changed to one framework.
Ok. I changed to Fact.

As a rule, you should only pivot on things that can affect the result of your test.
None of the changes you are making affect compat, so it's ok to not pivot on a framework.

Note that the focus should be correctness and completeness.
The testing time is a secondary consideration and imo irrelevant unless the above 2 satisified.

nkolev92 · 2021-01-07T02:58:05Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

@@ -453,6 +461,372 @@ public void TestCommandInvalidArguments(string command, int badCommandIndex)
            }
        }

+        [Theory]
+        [InlineData("net5.0")]
+        public async Task AddPkg_WithV2_RelativePath_NoVersionSpecified_Success(string targetFrameworks)


This test is not in the right file. The file suggests sources tests not add package command tests.

name: withV2 what? :)

Is the feed type relevant to the whole scenario?
I think local vs http feed is the scenario you are trying to solve.

I moved in to new DotnetAddPackageTests.cs
Also I renamed each tests to reflect they're local sourcefeed.

nkolev92 · 2021-01-07T02:59:20Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

+                var packageX_V1 = new PackageIdentity(packageX, new NuGetVersion("1.0.0"));
+                var packageX_V2 = new PackageIdentity(packageX, new NuGetVersion("2.0.0"));
+                var packageXPath = Path.Combine(pathContext.WorkingDirectory, "Custompackages");
+                var sourceRelativePath = RuntimeEnvironmentHelper.IsWindows ? "..\\..\\Custompackages" : "../../Custompackages";


nit: You can use Path.Combine("..","..", custompackages)

That way you don't need the env check, the runtime does it for you.

Ok. Addressed it. I'm not sure what I was thinking when I add this.

I'm not sure what I was thinking when I add this.

I think that about most of the code I've not written today :D

nkolev92 · 2021-01-07T03:04:35Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

+                var packageX = "packageX";
+                var packageY = "packageY";
+                var packageY_V1 = new PackageIdentity(packageY, new NuGetVersion("1.0.0"));
+                var packageXPath = Path.Combine(pathContext.WorkingDirectory, "Custompackages");


naming: I think should name this, custom source. The name suggest a path to the package, but it's really a path to the source.

Note that sourceRelativePath already follows that logic, so this is about being consistent.

Ok. I renamed to customSourcePath. Please check.

nkolev92 · 2021-01-07T03:05:33Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

+
+                // Generate Package, but packageX is not in nuget feed.
+                await SimpleTestPackageUtility.CreateFolderFeedV2Async(
+                    packageXPath, //not using solution source folder


packageXPath, but Y is in the source? I'm not following.

This code is for unhappy path(fail case). By mistake user placed packageY package in packageX's custom location.
Then try to restore packageX then it should fail.

nkolev92 · 2021-01-07T03:07:22Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

+                PackageSpec packageSpec = projectA.AssetsFile.PackageSpec;
+                string[] sources = packageSpec.RestoreMetadata.Sources.Select(s => s.Name).ToArray();
+                Assert.Equal(sources.Count(), 1);
+                Assert.True(sources[0].Contains("Custompackages"));


The value here should be the full path, so you can check it against the full path I think.

Sure, I changed to full address.

nkolev92 · 2021-01-07T03:11:43Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

+            }
+        }
+
+        [Theory]


If you are going to write a theory, you can probably combine the test cases.

For example, the theory data can be the version.

[InlineDate("")] [InlineDate("1.0.0")]

I changed code to 'Fact'. Since dotnet integration test are slow I don't want overburden with many input cases.

I changed code to 'Fact'. Since dotnet integration test are slow I don't want overburden with many input cases.

I was suggesting you merge test cases into 1. I think having a test for scenarios with and without version makes sense.

erdembayar

@nkolev92
Ok. I addressed PR review.
Moved dotnet integration tests into new DotnetAddPackageTests.cs file.
Also in order to make sure I'm not breaking existing default behavior before my code changes I added new 2 tests which doesn't use custom source, instead use default solution source.
AddPkg_V2LocalSourceFeed_WithDefaultSolutiuonSource_NoVersionSpecified_Success
AddPkg_V3LocalSourceFeed_WithDefaultSolutiuonSource_NoVersionSpecified_Success

erdembayar · 2021-01-07T18:38:56Z

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

@@ -102,6 +102,22 @@ public async Task<int> ExecuteCommand(PackageReferenceArgs packageReferenceArgs,

            var originalPackageSpec = matchingPackageSpecs.FirstOrDefault();

+            // Convert relative path to absolute path if there is any
+            List<string> sourcePaths = new List<string>();


Ok. Renamed to sources.

erdembayar · 2021-01-07T18:39:50Z

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs

@@ -334,7 +350,7 @@ public static async Task<NuGetVersion> GetLatestVersionAsync(PackageSpec origina
                    MachineWideSettings = new XPlatMachineWideSetting(),
                    GlobalPackagesFolder = packageReferenceArgs.PackageDirectory,
                    PreLoadedRequestProviders = providers,
-                    Sources = packageReferenceArgs.Sources?.ToList()
+                    Sources = sourcePaths


Added comment here for why we're not passing value down here.

erdembayar · 2021-01-07T18:41:47Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

@@ -453,6 +461,372 @@ public void TestCommandInvalidArguments(string command, int badCommandIndex)
            }
        }

+        [Theory]
+        [InlineData("net5.0")]


Initially I was thinking about testing many frameworks as much possible then considering heavy taxing on testing system I just changed to one framework.
Ok. I changed to Fact.

erdembayar · 2021-01-07T18:42:35Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

+                var packageX_V1 = new PackageIdentity(packageX, new NuGetVersion("1.0.0"));
+                var packageX_V2 = new PackageIdentity(packageX, new NuGetVersion("2.0.0"));
+                var packageXPath = Path.Combine(pathContext.WorkingDirectory, "Custompackages");
+                var sourceRelativePath = RuntimeEnvironmentHelper.IsWindows ? "..\\..\\Custompackages" : "../../Custompackages";


Ok. Addressed it. I'm not sure what I was thinking when I add this.

erdembayar · 2021-01-07T18:42:58Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

+                PackageSpec packageSpec = projectA.AssetsFile.PackageSpec;
+                string[] sources = packageSpec.RestoreMetadata.Sources.Select(s => s.Name).ToArray();
+                Assert.Equal(sources.Count(), 1);
+                Assert.True(sources[0].Contains("Custompackages"));


Sure, I changed to full address.

erdembayar · 2021-01-07T19:09:42Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

+                var packageX = "packageX";
+                var packageY = "packageY";
+                var packageY_V1 = new PackageIdentity(packageY, new NuGetVersion("1.0.0"));
+                var packageXPath = Path.Combine(pathContext.WorkingDirectory, "Custompackages");


Ok. I renamed to customSourcePath. Please check.

erdembayar · 2021-01-07T19:10:33Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

+
+                // Generate Package, but packageX is not in nuget feed.
+                await SimpleTestPackageUtility.CreateFolderFeedV2Async(
+                    packageXPath, //not using solution source folder


This code is for unhappy path(fail case). By mistake user placed packageY package in packageX's custom location.
Then try to restore packageX then it should fail.

erdembayar · 2021-01-07T19:11:53Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

+            }
+        }
+
+        [Theory]


I changed code to 'Fact'. Since dotnet integration test are slow I don't want overburden with many input cases.

erdembayar · 2021-01-07T19:14:47Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetSourcesTests.cs

@@ -453,6 +461,372 @@ public void TestCommandInvalidArguments(string command, int badCommandIndex)
            }
        }

+        [Theory]
+        [InlineData("net5.0")]
+        public async Task AddPkg_WithV2_RelativePath_NoVersionSpecified_Success(string targetFrameworks)


I moved in to new DotnetAddPackageTests.cs
Also I renamed each tests to reflect they're local sourcefeed.

erdembayar · 2021-01-08T17:52:13Z

@NuGet/nuget-client
It's ready to review.

nkolev92 · 2021-01-08T21:02:16Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetAddPackageTests.cs

+        }
+
+        [Fact]
+        public async Task AddPkg_V2LocalSourceFeed_WithRelativePath_NoVersionSpecified_Success()


Do you need both a V2 and a V3 test case?

I thought V2 and V3 sources have different directory structure. So I thought testing both of them relative path would be cover all possible scenarios.
I can remove one of them. So we keep V3 one.

@nkolev92
I removed V2 source tests. Please check now.

I thought V2 and V3 sources have different directory structure. So I thought testing both of them relative path would be cover all possible scenarios.

The reason why I kept probing around this topic is the fact that I wanted to make you are focusing on the right scenarios for the test.

You needed to test 2 things:

--source completely overwrites the source.

--source when used with a relative local source, the full path is calculated correctly.

The full path calculation is not affected by the feed type (v2 or v3 local feed), that only matters afterwards.

I think the tests are there right now.
Taking another look :)

nkolev92

nit: watch the Pr title :) source and feed are the same thing.

nkolev92 · 2021-01-08T23:04:48Z

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatTestUtils.cs

@@ -219,14 +219,15 @@ internal static PackageReferenceArgs GetPackageReferenceArgs(string packageId, S
        }

        internal static PackageReferenceArgs GetPackageReferenceArgs(string packageId, string packageVersion, SimpleTestProjectContext project,
-            string frameworks = "", string packageDirectory = "", string sources = "", bool noRestore = false, bool noVersion = false, bool prerelease = false)
+            string frameworks = "", string packageDirectory = "", string sources = "", bool noRestore = false, bool noVersion = false, bool prerelease = false, string dgFilePath = "")


Do you still need this file change?

No need, I had some learning curve around Xplat env. I'll do better next time here. Reverted it.

erdembayar · 2021-01-08T23:20:16Z

This eyeballs fine to me. I would like to see tests specific to this scenario before approving, though.

@zkat
Can you review now? I added unit tests.

When setting up NuGet sources, if NuGet is unable to parse the path, this tries to resolve relative paths to absolute, and then retries setting up the NuGet source. If the path is unable to be resolved, then this ignores the error and lets NuGet handle the resulting invalid source. This path resolution requires the filesystem to be passed in. This means that various method signatures change to allow access to the filesystem. When using NuGet.Core, then relative paths were able to be resolved. But now with NuGet.Client, relative paths are resolved by clients, not by libraries, which is why this change has to be made. NuGet/NuGet.Client#3783

erdembayar requested a review from a team as a code owner December 1, 2020 23:17

erdembayar force-pushed the dev-eryondon-package-incompatible-allframworks branch from c200f9b to acd0e7d Compare December 2, 2020 00:38

zkat reviewed Dec 2, 2020

View reviewed changes

nkolev92 reviewed Dec 2, 2020

View reviewed changes

...uGet.CommandLine.XPlat/Commands/PackageReferenceCommands/AddPackageReferenceCommandRunner.cs Outdated Show resolved Hide resolved

nkolev92 reviewed Dec 3, 2020

View reviewed changes

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatTestUtils.cs Outdated Show resolved Hide resolved

test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/XPlatAddPkgTests.cs Outdated Show resolved Hide resolved

nkolev92 reviewed Dec 4, 2020

View reviewed changes

erdembayar marked this pull request as draft December 6, 2020 05:00

erdembayar force-pushed the dev-eryondon-package-incompatible-allframworks branch from f108c01 to 1273913 Compare December 9, 2020 16:40

erdembayar marked this pull request as ready for review December 9, 2020 21:44

erdembayar requested review from nkolev92 and zkat December 11, 2020 00:08

erdembayar changed the title ~~Fix dotnet add package with custom source feed doesn't work when version is not specified~~ Fix dotnet add package doesn't work when with relative path source feed or version is not specified Dec 14, 2020

erdembayar changed the title ~~Fix dotnet add package doesn't work when with relative path source feed or version is not specified~~ Fix dotnet add package doesn't work with relative path source feed or when version is not specified Dec 14, 2020

erdembayar force-pushed the dev-eryondon-package-incompatible-allframworks branch 2 times, most recently from 67675df to 7e386f0 Compare December 19, 2020 00:25

nkolev92 reviewed Dec 21, 2020

View reviewed changes

erdembayar requested a review from nkolev92 January 6, 2021 22:14

nkolev92 reviewed Jan 7, 2021

View reviewed changes

erdembayar commented Jan 7, 2021

View reviewed changes

Address PR review by Nkolche.

4943245

erdembayar force-pushed the dev-eryondon-package-incompatible-allframworks branch from f9e5368 to 4943245 Compare January 7, 2021 19:19

erdembayar mentioned this pull request Jan 7, 2021

Package 'NameOfPackage' is incompatible with 'all' frameworks in project NuGet/Home#5127

Closed

Remove unused sources parameter passed down.

ec6c73f

erdembayar requested a review from nkolev92 January 8, 2021 17:52

nkolev92 reviewed Jan 8, 2021

View reviewed changes

Remove V2 sourcefeed unit tests

8f797e1

nkolev92 approved these changes Jan 8, 2021

View reviewed changes

Revert unused file change.

c9e4a0f

erdembayar changed the title ~~Fix dotnet add package doesn't work with relative path source feed or when version is not specified~~ Fix dotnet add package doesn't work with relative path source or when version is not specified Jan 8, 2021

zkat approved these changes Jan 11, 2021

View reviewed changes

erdembayar modified the milestones: Sprint 181 - 2020.12.21, Sprint 182 - 2021.01.11 Jan 11, 2021

Merge branch 'dev' into dev-eryondon-package-incompatible-allframworks

514e29b

erdembayar merged commit 190784b into dev Jan 12, 2021

erdembayar deleted the dev-eryondon-package-incompatible-allframworks branch January 12, 2021 22:17

P9os added a commit to P9os/tdlib.native that referenced this pull request Apr 18, 2021

fix NuGet/NuGet.Client#3783

ae416c5

P9os added a commit to P9os/tdlib.native that referenced this pull request Apr 19, 2021

fix NuGet/NuGet.Client#3783

26da2cf

TheCakeIsNaOH mentioned this pull request Dec 20, 2022

(#508) Attempt to resolve relative paths for sources chocolatey/choco#2943

Merged

10 tasks

	internal List<SourceRepository> GetEffectiveSources(ISettings settings, IList<PackageSource> dgSpecSources)
	{
	if (settings == null)
	{
	throw new ArgumentNullException(nameof(settings));
	}
	var values = settings.GetConfigRoots();
	if (dgSpecSources != null)
	{
	values.AddRange(dgSpecSources.Select(e => e.Source));
	}

	var cacheKey = string.Join("\|", values);

	return _sourcesCache.GetOrAdd(cacheKey, (root) => GetEffectiveSourcesCore(settings, dgSpecSources));
	}

Fix dotnet add package doesn't work with relative path source or when version is not specified #3783

Fix dotnet add package doesn't work with relative path source or when version is not specified #3783

Conversation

erdembayar commented Dec 1, 2020 • edited Loading

Bug

Fix

Testing/Validation

zkat left a comment

Choose a reason for hiding this comment

erdembayar commented Dec 2, 2020 • edited Loading

nkolev92 left a comment

Choose a reason for hiding this comment

erdembayar commented Dec 3, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Dec 18, 2020 • 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

RestoreSuccessful

Another question is do we write relative path or absolute path in case user passed relative path for source?

Overwrite

Append.

erdembayar commented Dec 5, 2020 • edited Loading

nkolev92 commented Dec 8, 2020

erdembayar commented Dec 8, 2020

nkolev92 commented Dec 8, 2020

erdembayar commented Dec 8, 2020

nkolev92 commented Dec 8, 2020

erdembayar commented Dec 9, 2020

erdembayar commented Dec 11, 2020

erdembayar commented Dec 15, 2020

nkolev92 commented Dec 16, 2020

erdembayar commented Dec 18, 2020

erdembayar commented Dec 21, 2020 • edited Loading

nkolev92 left a comment

Choose a reason for hiding this comment

erdembayar commented Jan 6, 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

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

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

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

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

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 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

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

Choose a reason for hiding this comment

erdembayar commented Dec 1, 2020 •

edited

Loading

erdembayar commented Dec 2, 2020 •

edited

Loading

erdembayar Dec 18, 2020 •

edited

Loading

erdembayar commented Dec 5, 2020 •

edited

Loading

erdembayar commented Dec 21, 2020 •

edited

Loading

nkolev92 Jan 8, 2021 •

edited

Loading