Use package source mappings in VMR build #19114

NikolaMilosavljevic · 2024-03-20T17:41:30Z

Fixes: dotnet/source-build#4206, dotnet/source-build#4229

Overview of changes:

Remove temporary nuget-client patch that reverted root cause of non-determinism in package restore - Revert non-determinism in Nuget local sources #19074
Delete original SBRP packages after they get copied to reference packages dir
Add package source mappings for all source-build sources, following the logic in original issue - also explained below
In offline build, remove all existing package source mappings for online sources
In online build, filter existing package source mappings to remove anything that exists in any source-build source
Add package source mappings for source-build-reference-package-cache source utilized in SBRP repo build. Unlike all other sources, this one is dynamic and populated during SBRP repo build, with new packages as they get built. We need to update NuGet.config before repo build. Solution was to discover all packages that will be built by this repo, from all nuspec files in repo's sources.

Package source mappings logic:

Add all packages from current source-build sources, i.e. source-built-*, reference-packages
For previously source-built sources, add only the packages that do not exist in any of the current source-built sources. Also add packages if PSB source contains a package version that does not exist in current package sources. (This was required for aspnetcore build, which brings transitive dependency on ILLink.Tasks package from PSB, via crossgen2 package dependency.)

ViktorHofer

I'll leave some feedback which you can react to post-merge as I understand that this is high priority.

ViktorHofer · 2024-03-20T17:50:04Z

src/SourceBuild/content/eng/tools/init-build.proj

+    <!-- Delete the original SBRP packages -->
+    <RemoveDir Directories="$(PrebuiltSourceBuiltPackagesPath)SourceBuildReferencePackages" />


Why do we copy to that location if we then immediately copy to another location and delete this location?

We extract these SBRPs from the PSB archive, together with other packages to prereqs/packages/previously-source-built.

After extraction, SBRPs will be in prereqs/packages/previously-source-built/SourceBuildReferencePackages.

We then copy SBRPs to prereqs/packages/reference folder, but still leave the original extracted content that could cause poisoned files. With package mappings work, this is totally unused.

Fixed with 3f02bc9

ViktorHofer · 2024-03-20T17:50:40Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

@@ -34,45 +37,126 @@ public class UpdateNuGetConfigPackageSourcesMappings : Task
        /// </summary>
        public string[] SourceBuildSources { get; set; }

+        public string VmrRoot { get; set; }


I would rather call this RepoRoot. VMR isn't the best keyword to use in source.

Fixed with 3f02bc9

ViktorHofer · 2024-03-20T17:51:00Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

@@ -34,45 +37,126 @@ public class UpdateNuGetConfigPackageSourcesMappings : Task
        /// </summary>
        public string[] SourceBuildSources { get; set; }

+        public string VmrRoot { get; set; }
+
+        private string SBRPCacheSourceName = "source-build-reference-package-cache";


This should be const and follow naming conventions.

Fixed with 3f02bc9

MichaelSimons · 2024-03-20T17:51:41Z

src/SourceBuild/content/eng/tools/init-build.proj

@@ -52,6 +52,9 @@

    <Copy SourceFiles="@(UnpackedSourceBuildReferencePackages)" DestinationFiles="$(ReferencePackagesDir)%(Filename)%(Extension)" />


Instead of copy and delete, wouldn't it be better to change this to a move?

Yes, I'll fix it.

Fixed with 3f02bc9

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

ViktorHofer · 2024-03-20T17:53:51Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+            Hashtable allSourcesPackages = new Hashtable();
+            Hashtable currentPackages = new Hashtable();
+            Hashtable referencePackages = new Hashtable();
+            Hashtable previouslyBuiltPackages = new Hashtable();
+            Hashtable oldSourceMappingPatterns = new Hashtable();


We usually recommend to use Dictionary<T,V> instead of Hashtable.

Yeah, that's a better option - will update, thanks!

Fixed with 3f02bc9

ViktorHofer · 2024-03-20T17:59:09Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+            if (string.IsNullOrEmpty(VmrRoot))
+            {
+                throw new InvalidDataException(string.Format(CultureInfo.CurrentCulture, "VmrRoot is not set - cannot determine SBRP packages."));
+            }


I would probably just add the Required attribute to the VmrRoot property and remove this block.

Fixed with 3f02bc9

MichaelSimons · 2024-03-20T18:10:30Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

@@ -34,45 +37,126 @@ public class UpdateNuGetConfigPackageSourcesMappings : Task
        /// </summary>
        public string[] SourceBuildSources { get; set; }

+        public string VmrRoot { get; set; }


It looks like this is only used to get the SBRP repo path. Consider refactoring to be the SBRP src path. IMO it would be better to have this logic encapsulated in the msbuild logic rather than within this task.

Yes, that sounds good - will fix it.

Fixed with 3f02bc9

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

MichaelSimons · 2024-03-20T19:44:41Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs


-            if (!BuildWithOnlineFeeds)
+            DiscoverPackagesFromAllSourceBuildSources(pkgSourcesElement, allSourcesPackages, currentPackages, referencePackages, previouslyBuiltPackages);


IMO it would help the readability of this method to break it apart - it is around 130 lines. The three chucks are obvious targets to encapsulate in helper methods.

Thanks. Yeah - I did some refactoring but stopped short. Wanted to get some early feedback, before refactoring some more.

Fixed with 3f02bc9

ViktorHofer · 2024-03-20T19:49:54Z

How will this work when not building from source? I assume the SBRP logic will then get skipped?

MichaelSimons · 2024-03-20T19:56:17Z

@NikolaMilosavljevic - would you mind sharing a few processed nuget.configs with this change. e.g. a repo w/existing mappings, a repo without, online, and offline?

NikolaMilosavljevic · 2024-03-20T20:02:31Z

How will this work when not building from source? I assume the SBRP logic will then get skipped?

Yes, SBRP logic is conditioned on presence of SBRP cache source, which only exists for SBRP repo.

NikolaMilosavljevic · 2024-03-20T20:11:34Z

@NikolaMilosavljevic - would you mind sharing a few processed nuget.configs with this change. e.g. a repo w/existing mappings, a repo without, online, and offline?

configs.zip

MichaelSimons · 2024-03-20T20:31:03Z

@NikolaMilosavljevic - would you mind sharing a few processed nuget.configs with this change. e.g. a repo w/existing mappings, a repo without, online, and offline?

configs.zip

The online configs that do not contain package source mappings do not look right to me. All online feeds need a packageSourceMapping added with a * package pattern. IIRC, without this, the online feeds will not be considered at all and therefore would break the build when prebuilts exist.

MichaelSimons · 2024-03-20T20:35:17Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+                    }
+                }
+
+                throw new InvalidDataException(string.Format(CultureInfo.CurrentCulture, "Did not extract nuspec file from package: {0}", path));


Throwing an exception in a try..catch is an anti-pattern and does not feed necessary here. This could be moved outside the try..catch

Fixed with 3f02bc9

MichaelSimons · 2024-03-20T20:37:57Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+        }
+    }
+
+    public class NupkgInfo


Consider making a private nested type as it should not be used outside the task.

Fixed with 3f02bc9

NikolaMilosavljevic · 2024-03-20T20:42:50Z

@NikolaMilosavljevic - would you mind sharing a few processed nuget.configs with this change. e.g. a repo w/existing mappings, a repo without, online, and offline?

configs.zip

The online configs that do not contain package source mappings do not look right to me. All online feeds need a packageSourceMapping added with a * package pattern. IIRC, without this, the online feeds will not be considered at all and therefore would break the build when prebuilts exist.

Makes sense - I've missed this. Will update. Thanks!

NikolaMilosavljevic · 2024-03-20T22:55:10Z

@NikolaMilosavljevic - would you mind sharing a few processed nuget.configs with this change. e.g. a repo w/existing mappings, a repo without, online, and offline?

configs.zip

The online configs that do not contain package source mappings do not look right to me. All online feeds need a packageSourceMapping added with a * package pattern. IIRC, without this, the online feeds will not be considered at all and therefore would break the build when prebuilts exist.

Fixed with 3f02bc9

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

MichaelSimons · 2024-03-21T01:50:30Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+
+        private void AddDefaultMappingsForOnlineSources(XElement pkgSrcMappingClearElement, XElement pkgSourcesElement)
+        {
+            throw new NotImplementedException();


Is this intentional or an incomplete refactoring?

Not intentional - not used as I refactored it more. Will fix.

Fixed with 3f458e5

MichaelSimons · 2024-03-21T01:51:49Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+
+        private void AddToDictionary(Dictionary<string, List<string>> dictionary, string key, string value)
+        {
+            if (dictionary.ContainsKey(key))


TryGetValue looks to be a better choice here as it avoids indexing the dictionary multiple times.

Will fix, for simpler code (one less line). Not sure about perf as TryGetValue essentially invokes ContainsKey and does the indexing.

Fixed with 3f458e5

MichaelSimons · 2024-03-21T01:54:29Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+            }
+        }
+
+        private void GetExistingFilteredSourceMappings(XElement pkgSrcMappingElement, Dictionary<string, List<string>> currentPackages, Dictionary<string, List<string>> referencePackages, Dictionary<string, List<string>> previouslyBuiltPackages, Dictionary<string, List<string>> oldSourceMappingPatterns)


This line is too long IMO.

Suggested change

private void GetExistingFilteredSourceMappings(XElement pkgSrcMappingElement, Dictionary<string, List<string>> currentPackages, Dictionary<string, List<string>> referencePackages, Dictionary<string, List<string>> previouslyBuiltPackages, Dictionary<string, List<string>> oldSourceMappingPatterns)

private void GetExistingFilteredSourceMappings(

XElement pkgSrcMappingElement,

Dictionary<string, List<string>> currentPackages,

Dictionary<string, List<string>> referencePackages,

Dictionary<string, List<string>> previouslyBuiltPackages,

Dictionary<string, List<string>> oldSourceMappingPatterns)

Fixed with 3f458e5

MichaelSimons · 2024-03-21T02:10:08Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

-                .Select(e => e.Attribute("pattern").Value)
-                .Distinct()
-                .ToArray();
+            var allSourcesPackages = new Dictionary<string, List<string>>();


Consider noting that not all these dictionaries contain the same type of value, consider calling it out here. What I am referring to is that all but the allSourcesPackages stores the versions.

Fixed with 3f458e5

NikolaMilosavljevic · 2024-03-21T15:26:36Z

Online build is failing due to vstest NuGet.config. Will have a fix shortly.

…eBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs Co-authored-by: Michael Simons <msimons@microsoft.com>

NikolaMilosavljevic · 2024-03-21T17:09:32Z

Online build is failing due to vstest NuGet.config. Will have a fix shortly.

Fixed with 3f458e5

MichaelSimons · 2024-03-21T17:49:38Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+                .Where(e => e.Name == "add" && !allSourcesPackages.Keys.Contains(e.Attribute("key").Value))
+                .Select(e => e.Attribute("key").Value)
+                .Distinct()
+                .ToArray())


ToArray is unnecessary overhead here isn't it?

Oops - you're right - will fix.

Fixed with 0a1e0b1

MichaelSimons

One minor comment. LGTM.

NikolaMilosavljevic · 2024-03-21T18:30:00Z

Setting DO NOT MERGE until all validations, including local and internal, are done and successful.

mthalman · 2024-03-21T19:12:01Z

Current error from nuget-client:

  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.CommandLine.XPlat/NuGet.CommandLine.XPlat.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Credentials/NuGet.Credentials.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Common/NuGet.Common.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Build.Tasks/NuGet.Build.Tasks.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Packaging/NuGet.Packaging.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Configuration/NuGet.Configuration.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Build.Tasks.Console/NuGet.Build.Tasks.Console.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Commands/NuGet.Commands.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Resolver/NuGet.Resolver.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.DependencyResolver.Core/NuGet.DependencyResolver.Core.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Protocol/NuGet.Protocol.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Versioning/NuGet.Versioning.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.ProjectModel/NuGet.ProjectModel.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Frameworks/NuGet.Frameworks.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/Microsoft.Build.NuGetSdkResolver/Microsoft.Build.NuGetSdkResolver.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.LibraryModel/NuGet.LibraryModel.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Packaging.Core/NuGet.Packaging.Core.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.PackageManagement/NuGet.PackageManagement.csproj]
      0 Warning(s)
      19 Error(s)

NikolaMilosavljevic · 2024-03-21T19:16:28Z

Current error from nuget-client:

  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.CommandLine.XPlat/NuGet.CommandLine.XPlat.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Credentials/NuGet.Credentials.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Common/NuGet.Common.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Build.Tasks/NuGet.Build.Tasks.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Packaging/NuGet.Packaging.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Configuration/NuGet.Configuration.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Build.Tasks.Console/NuGet.Build.Tasks.Console.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Commands/NuGet.Commands.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Resolver/NuGet.Resolver.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.DependencyResolver.Core/NuGet.DependencyResolver.Core.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Protocol/NuGet.Protocol.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Versioning/NuGet.Versioning.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.ProjectModel/NuGet.ProjectModel.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Frameworks/NuGet.Frameworks.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/Microsoft.Build.NuGetSdkResolver/Microsoft.Build.NuGetSdkResolver.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.LibraryModel/NuGet.LibraryModel.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.Packaging.Core/NuGet.Packaging.Core.csproj]
  /vmr/.dotnet/sdk/9.0.100-preview.2.24103.2/NuGet.targets(694,5): error : Package source 'dotnet-libraries' must have at least one package pattern. Path: '/vmr/artifacts/obj/nuget-client/NuGet.Config' [/vmr/src/nuget-client/src/NuGet.Core/NuGet.PackageManagement/NuGet.PackageManagement.csproj]
      0 Warning(s)
      19 Error(s)

Yeah - have a fix - just pushed - b985bea

mthalman

All of my comments are just about maintainability. Feel free to defer those updates to address later in order to unblock things.

mthalman · 2024-03-21T19:18:38Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+        [Required]
+        public string SbrpRepoSrcPath { get; set; }
+
+        private const string SbrpCacheSourceName = "source-build-reference-package-cache";


It would be ideal if this could be passed in as a task property so it's shared with the value from here:

installer/src/SourceBuild/content/repo-projects/source-build-reference-packages.proj

Line 30 in 79f35dc

SourceName="source-build-reference-package-cache"

mthalman · 2024-03-21T19:23:08Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+        private XElement GetPackageMappingsElementForSource(string packageSource)
+        {
+            bool isCurrentSourceBuiltSource =
+                packageSource.StartsWith("source-built-") ||


The source-built- string should be passed as a task property so it can be shared with the value from here:

installer/src/SourceBuild/content/repo-projects/Directory.Build.targets

Line 81 in 79f35dc

<ShippingSourceName>source-built-%(Identity)</ShippingSourceName>

mthalman · 2024-03-21T19:24:02Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+            bool isCurrentSourceBuiltSource =
+                packageSource.StartsWith("source-built-") ||
+                packageSource.Equals(SbrpCacheSourceName) ||
+                packageSource.Equals("reference-packages");


Similar comment of using a task property to share with:

installer/src/SourceBuild/content/repo-projects/Directory.Build.targets

Line 123 in 79f35dc

<ReferencePackagesNuGetSourceName>reference-packages</ReferencePackagesNuGetSourceName>

mthalman · 2024-03-21T19:25:08Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+                    if (packageSource.StartsWith("source-built-"))
+                    {
+                        AddToDictionary(currentPackages, id, version);
+                    }
+                    else if (packageSource.Equals("reference-packages"))


Reuse strings from task properties that I commented on.

NikolaMilosavljevic · 2024-03-21T19:42:35Z

All of my comments are just about maintainability. Feel free to defer those updates to address later in order to unblock things.

Thanks - I'll create an issue to fix those in a follow-up PR.

NikolaMilosavljevic · 2024-03-21T20:04:11Z

All of my comments are just about maintainability. Feel free to defer those updates to address later in order to unblock things.

Thanks - I'll create an issue to fix those in a follow-up PR.

dotnet/source-build#4242

NikolaMilosavljevic · 2024-03-22T04:05:49Z

With latest changes to add mappings to online sources, PR validation checks are passing. Internal SB and UB pipelines are in progress.

Some of the configs:
configs.zip
ub* files are for unified-build, to test the custom feed.

mthalman · 2024-03-22T12:35:33Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+            foreach (string packageSource in allSourcesPackages.Keys)
+            {
+                // Skip sources with zero package patterns
+                if (allSourcesPackages[packageSource] != null)


In the other occurrence of this comment, it used a pattern like allSourcesPackages[packageSource]?.Count > 0 to account for empty lists. Should that be used here?

MichaelSimons

My comments can be addressed in a follow-up PR. I think we should get this in ASAP to unblock Preview 3.

MichaelSimons · 2024-03-22T13:19:16Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

@@ -54,6 +54,8 @@ public class UpdateNuGetConfigPackageSourcesMappings : Task
        private Dictionary<string, List<string>> previouslyBuiltPackages = [];
        private Dictionary<string, List<string>> oldSourceMappingPatterns = [];

+        private string[] CustomSources = ["prebuilt", "net-sdk-supporting-feed"];


Suggested change

private string[] CustomSources = ["prebuilt", "net-sdk-supporting-feed"];

private readonly string[] CustomSources = ["prebuilt", "net-sdk-supporting-feed"];

I question if prebuilt should be handled the same as net-sdk-supporting-feed. The prebuilt feed is a local feed which can be enumerated and have full packageIds added for each of it's packages. It should not have the cumulative list of PackageIDs added.

Yeah, makes sense. It seemed that we should cover it when we discussed it yesterday, but it does not seem correct.

Please open an issue to track this.

MichaelSimons · 2024-03-22T13:28:12Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

            {
-                XElement pkgSrc = new XElement("packageSource", new XAttribute("key", packageSource));
-                foreach (string packagePattern in packagePatterns)
+                if (oldSourceMappingPatterns.Count > 0)


Nit, this if condition seems unnecessary. It is adding unnecessary complexity and nesting, the foreach will no-op with an empty collection.

MichaelSimons · 2024-03-22T13:33:38Z

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs

+        {
+            foreach (string sourceName in CustomSources)
+            {
+                if (null != pkgSourcesElement.Descendants().FirstOrDefault(e => e.Name == "add" && e.Attribute("key").Value == sourceName))


It looks like this logic is duplicated a number of times. Consider a lookup helper method.

NikolaMilosavljevic · 2024-03-22T14:32:42Z

/backport to release/9.0.1xx-preview3

github-actions · 2024-03-22T14:32:55Z

Started backporting to release/9.0.1xx-preview3: https://github.com/dotnet/installer/actions/runs/8391929435

Use package source mappings in VMR build

4217e44

NikolaMilosavljevic requested review from a team as code owners March 20, 2024 17:41

ViktorHofer approved these changes Mar 20, 2024

View reviewed changes

ViktorHofer reviewed Mar 20, 2024

View reviewed changes

MichaelSimons reviewed Mar 20, 2024

View reviewed changes

ViktorHofer reviewed Mar 20, 2024

View reviewed changes

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs Show resolved Hide resolved

ViktorHofer reviewed Mar 20, 2024

View reviewed changes

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs Show resolved Hide resolved

ViktorHofer reviewed Mar 20, 2024

View reviewed changes

MichaelSimons reviewed Mar 20, 2024

View reviewed changes

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs Show resolved Hide resolved

MichaelSimons reviewed Mar 20, 2024

View reviewed changes

Addressing review comments

3f02bc9

MichaelSimons reviewed Mar 21, 2024

View reviewed changes

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs Outdated Show resolved Hide resolved

MichaelSimons reviewed Mar 21, 2024

View reviewed changes

...ls/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs Show resolved Hide resolved

MichaelSimons reviewed Mar 21, 2024

View reviewed changes

NikolaMilosavljevic and others added 2 commits March 21, 2024 09:17

Update src/SourceBuild/content/eng/tools/tasks/Microsoft.DotNet.Sourc…

ddae954

…eBuild.Tasks.XPlat/UpdateNuGetConfigPackageSourcesMappings.cs Co-authored-by: Michael Simons <msimons@microsoft.com>

Some refactoring

3f458e5

MichaelSimons reviewed Mar 21, 2024

View reviewed changes

MichaelSimons approved these changes Mar 21, 2024

View reviewed changes

small fix

0a1e0b1

NikolaMilosavljevic added the DO NOT MERGE label Mar 21, 2024

Fix for empty online source mapping

b985bea

mthalman reviewed Mar 21, 2024

View reviewed changes

NikolaMilosavljevic mentioned this pull request Mar 21, 2024

Improve maintainability of UpdateNuGetConfigPackageSourcesMappings task dotnet/source-build#4242

Closed

Fix for unified build

2339b95

mthalman reviewed Mar 22, 2024

View reviewed changes

MichaelSimons reviewed Mar 22, 2024

View reviewed changes

NikolaMilosavljevic merged commit 049c92b into dotnet:main Mar 22, 2024
22 checks passed

github-actions bot mentioned this pull request Mar 22, 2024

[release/9.0.1xx-preview3] Use package source mappings in VMR build #19143

Merged

This was referenced Mar 25, 2024

Improve UpdateNuGetConfigPackageSourcesMappings task dotnet/source-build#4256

Closed

Package source mappings - follow up #19223

Merged

SBRPs are present in multiple locations during source-build dotnet/source-build#4229

Closed

NikolaMilosavljevic removed the DO NOT MERGE label Mar 29, 2024

mthalman mentioned this pull request Apr 9, 2024

NuGet feed non-determinism should be accounted for in Arcade source build dotnet/source-build#4311

Open

		<!-- Delete the original SBRP packages -->
		<RemoveDir Directories="$(PrebuiltSourceBuiltPackagesPath)SourceBuildReferencePackages" />

		@@ -52,6 +52,9 @@

		<Copy SourceFiles="@(UnpackedSourceBuildReferencePackages)" DestinationFiles="$(ReferencePackagesDir)%(Filename)%(Extension)" />


		if (!BuildWithOnlineFeeds)
		DiscoverPackagesFromAllSourceBuildSources(pkgSourcesElement, allSourcesPackages, currentPackages, referencePackages, previouslyBuiltPackages);

	private string[] CustomSources = ["prebuilt", "net-sdk-supporting-feed"];
	private readonly string[] CustomSources = ["prebuilt", "net-sdk-supporting-feed"];

Use package source mappings in VMR build #19114

Use package source mappings in VMR build #19114

Conversation

NikolaMilosavljevic commented Mar 20, 2024

ViktorHofer 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

ViktorHofer Mar 20, 2024 • 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

Choose a reason for hiding this comment

ViktorHofer Mar 20, 2024 • 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

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

ViktorHofer commented Mar 20, 2024

MichaelSimons commented Mar 20, 2024

NikolaMilosavljevic commented Mar 20, 2024

NikolaMilosavljevic commented Mar 20, 2024

MichaelSimons commented Mar 20, 2024

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

NikolaMilosavljevic commented Mar 20, 2024

NikolaMilosavljevic commented Mar 20, 2024

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

NikolaMilosavljevic commented Mar 21, 2024

NikolaMilosavljevic commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSimons left a comment

Choose a reason for hiding this comment

NikolaMilosavljevic commented Mar 21, 2024

mthalman commented Mar 21, 2024

NikolaMilosavljevic commented Mar 21, 2024 • edited Loading

mthalman 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

NikolaMilosavljevic commented Mar 21, 2024

NikolaMilosavljevic commented Mar 21, 2024

NikolaMilosavljevic commented Mar 22, 2024

mthalman Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

MichaelSimons 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

NikolaMilosavljevic commented Mar 22, 2024

ViktorHofer Mar 20, 2024 •

edited

Loading

ViktorHofer Mar 20, 2024 •

edited

Loading

NikolaMilosavljevic commented Mar 21, 2024 •

edited

Loading

mthalman Mar 22, 2024 •

edited

Loading