[.NET 9 Preview 3] no-op restore is no longer a no-op

[jonathanpeppers]

NuGet Product Used

dotnet.exe

Product Version

.NET SDK 9.0.100-preview.3.24126.2

Worked before?

.NET SDK 9.0.100-preview.2.24122.3

Impact

It's more difficult to complete my work

Repro Steps & Context

We discovered here a test failure: dotnet/android#8763

The underlying cause appears to be:

The restore inputs for 'UnnamedProject' have changed. Continuing restore.

This was an incremental build with no changes to the project.

Digging deeper, the offending build seems to have some NuGet sources listed twice:

++https://api.nuget.org/v3/index.json
++https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json
https://api.nuget.org/v3/index.json
https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json

So there must be something that adds extra feeds, making the no-op restore skip? so it runs a full restore?

Verbose Logs

Here are two .binlog files, install2.binlog restores as expected, install3.binlog does not:
nuget-noop.zip

[nkolev92]

Hey @jonathanpeppers,

Where do you see that delta?

The thing that determines whether a restore should happen is a nuget.dgspec.json file in the obj folder, before and after.
If it's easy for you to upload that, it would really speed up our investigation.

If you can't, one of us can try to run the test locally.

[jonathanpeppers]

We have just one nuget.dgspec.json at the end as an artifact, let me see if I can grab a copy at different stages of the test.

[jonathanpeppers]

Yeah, I think I see a difference in the files: obj.zip

Comparing the two folders with WinMerge:

The problem also occurred here, maybe the diff is smaller:

• [release/9.0.1xx-preview2] Update dependencies from dotnet/installer dotnet/android#8767
• dotnet/installer@0a73f81...e416bb0
• NuGet/NuGet.Client@8b658e2...39c4f37

[nkolev92]

Thanks @jonathanpeppers

That's really, really helpful!

[nkolev92]

I was unable to figure out where the regression is, but I did make some progress.

GetRestoreSettingsTask seems to return different values for the sources, but the inputs are the same.
This is happening for Library1 only.

I was able to reproduce a duplicate nuget.config, but both with 6.9 and 6.10 of NuGet (so not a regression within the commits above). To reproduce this, I just added the same source with different ids in the nuget.config.
Unfortunately, I don't understand why it'd differ from run to run.

I attempted to debug the test, but I was unable to, since it's requiring a commercial build, but I'm having a tough time making progress on that.

The contributing instructions seem to be outdated: https://github.com/xamarin/xamarin-android/blob/main/Documentation/building/windows/instructions.md#running-specific-nunit-tests.

I originally thought it'd be something like: NuGet/NuGet.Client#5620, but I'm still not sure why the same inputs would change the output.

@jonathanpeppers Any advice you can provide here?
I was hoping to get a repro disconnected from the test infrastructure with the SDK, or at least something I can try to debug within the test.

Unfortunately the binlogs are not enough to figure out where the regression is.

[jonathanpeppers]

@nkolev92 it doesn't feel like the problem here would be specific to Android, but maybe just our NuGet.config usage?

Let me see if I can make a simpler repro with two class libraries and a console app.

[zivkan]

A few months ago I refactored PackageSourceProvider, in order to make it easier to implement auditSources. The previous PackageSourceProvider code had a specific API to deduplicate sources based on source URL (value). I thought I couldn't find evidence of it actually being used, so I changed it to only deduplicate on source key. Maybe this is what's happening here.

I'm very busy with other work, that is well behind "schedule", so I haven't tried myself, but try a repro where nuget.config has two package sources with different keys, but the same url. You can also check dotnet nuget list source in the android repo, and see if it lists different sources with the same url/value.

[nkolev92]

I checked source deduping with 6.9 only, since I assumed that any regressions would've been introduced in 6.10, but may we already have that problem in 6.9.

I'll check against 6.8.

[nkolev92]

@zivkan

6.8 doesn't have any duplicate sources.

So NuGet/NuGet.Client@1942c17#diff-93482a8631efd30f5df833baa903bcf342fb99a2e93cf9a3dea6a672b44d3a61L81 definitely introduced a problem.

I'm still not sure why this would lead to a break in no-op though. So while that needs fixed, there's probably something else going on here.

[zivkan]

It's the end of my day, so I'm not going to investigate in the next few hours, but to test the hypothesis that my change broke something, you can create a console app that references NuGet.Configuration, use Settings.LoadDefaultFrom("path to android repo"), then use PackageSourceProvider.LoadPackageSources, and write the list to console. Then change the NuGet.Configuration package version to the previous GA version and check again.

I think this should be an "easy" way to validate, and if it does show a difference, as long as your debugger is using microsoft's symbol server, you should be able to debug into NuGet's source.

[nkolev92]

It's the end of my day, so I'm not going to investigate in the next few hours, but to test the hypothesis that my change broke something, you can create a console app that references NuGet.Configuration, use Settings.LoadDefaultFrom("path to android repo"), then use PackageSourceProvider.LoadPackageSources, and write the list to console. Then change the NuGet.Configuration package version to the previous GA version and check again.

I think this should be an "easy" way to validate, and if it does show a difference, as long as your debugger is using microsoft's symbol server, you should be able to debug into NuGet's source.

That's basically what I did, and meant to say in #13269 (comment).

For the same set of configs, the API now returns more sources than it did in 6.8.

[jonathanpeppers]

@nkolev92 did you see a duplicate source in our NuGet.config? I could fix that in our repo for now.

This happened during an MSBuild integration test, perhaps it generates a NuGet.config with a duplicate:

• https://github.com/xamarin/xamarin-android/blob/0d489b157453f19bb2d1f9567e30d2b40de1bf9d/NuGet.config#L10
• https://github.com/xamarin/xamarin-android/blob/0d489b157453f19bb2d1f9567e30d2b40de1bf9d/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/XamarinProject.cs#L420-L421

[jonathanpeppers]

Ok, I found:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <clear />
...
    <add key="dotnet8" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json" />
...
    <add key="testsource2" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json" />
  </packageSources>
</configuration>

[nkolev92]

We're making good progress here!

One thing I'm having trouble understand is why the restore would change within the same version.

This feels like one of those that'd be consistent within the same version, but buggy when comparing say 6.9 and 6.10 of NuGet.

basically, the change we've confirmed may have regressed something, I'm not confident that's all.

[jonathanpeppers]

Trying this for now: dotnet/android#8772

[nkolev92]

After more investigation, I think NuGet/NuGet.Client#5666 is sufficient.

        [Fact]
        public void LoadPackageSources_DeduplicatesBasedOnValue()
        {
            // Arrange
            using TestDirectory testDirectory = TestDirectory.Create();

            const string sourceUrl = "https://contoso.test/nuget/index.json";
            const string contents = $@"<configuration>
  <packageSources>
    <add key=""s1"" value=""{sourceUrl}"" />
    <add key=""s2"" value=""{sourceUrl}"" />
  </packageSources>
</configuration>
";
            var path = Path.Combine(testDirectory.Path, Settings.DefaultSettingsFileName);
            File.WriteAllText(path, contents);

            Settings settings = new Settings(testDirectory.Path);

            // Act
            List<PackageSource> sources = LoadPackageSources(settings);

            // Assert
            sources.Should().HaveCount(1);
        }

Returns 2 in both the commit before Andy's change and latest dev.