V4 Publishing should only place assets marked CouldBeStable on isolated feeds

src/Microsoft.DotNet.Arcade.Sdk/tools/Publish.proj

@@ -321,22 +321,22 @@
     </ItemGroup>
 
     <!-- Propagate stability data to the asset level for packages. We do this so that VMR builds which have some repo builds that are not stable, and others that are, do not end up having to use the default logic
-          where ALL assets that are shipping in a stable build go to isolated feeds. Instead, we can just choose CouldBeStable==true, CouldBeStable==false.
-          This only applies currently to Packages. Blobs are required to provide paths that are always non-stable. Pdbs don't have a stability concept.
+         where ALL assets that are shipping in a stable build go to isolated feeds. Instead, we can just choose CouldBeStable==true, CouldBeStable==false.
+         This only applies currently to Packages. Blobs are required to provide paths that are always non-stable. Pdbs don't have a stability concept.
 
-          NOTE: This flag doesn't strictly mean that the package is stable versioned. A repo might suppress a stable version for a given package. -->
+         NOTE: This flag doesn't strictly mean that the package is stable versioned. A repo might suppress a stable version for a given package. -->
     <ItemGroup>
       <ItemsToPushToBlobFeed>
-			  <ManifestArtifactData Condition="'%(ItemsToPushToBlobFeed.Kind)' == 'Package' and
+			  <ManifestArtifactData Condition="'$(PublishingVersion)' == '4' and
+                                         '%(ItemsToPushToBlobFeed.Kind)' == 'Package' and
 																			   '$(IsStableBuild)' == 'true' and
 																			   '$(IsReleaseOnlyPackageVersion)' != 'true' and
 																			   '%(ItemsToPushToBlobFeed.IsShipping)' == 'true'">%(ItemsToPushToBlobFeed.ManifestArtifactData);CouldBeStable=true</ManifestArtifactData>
-				<ManifestArtifactData Condition="'%(ItemsToPushToBlobFeed.Kind)' == 'Package' and
-																				 (
-                                          '$(IsStableBuild)' != 'true' or
-																				  '$(IsReleaseOnlyPackageVersion)' == 'true' or
-																				  '%(ItemsToPushToBlobFeed.IsShipping)' != 'true'
-                                         )">%(ItemsToPushToBlobFeed.ManifestArtifactData);CouldBeStable=false</ManifestArtifactData>
+        <!-- We narrowly set CouldBeStable=false only for packages we know won't generate a stable version because of IsReleaseOnlyPackageVersion == true.
+             This allows for the default 'stable asset to non-isolated feed' detection behavior to still pick up accidental cases of stabilization. -->
+				<ManifestArtifactData Condition="'$(PublishingVersion)' == '4' and
+                                         '%(ItemsToPushToBlobFeed.Kind)' == 'Package' and
+																				 '$(IsReleaseOnlyPackageVersion)' == 'true'">%(ItemsToPushToBlobFeed.ManifestArtifactData);CouldBeStable=false</ManifestArtifactData>
       </ItemsToPushToBlobFeed>
     </ItemGroup>
 

src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestBase.cs

@@ -5,7 +5,6 @@
 using System.Collections.Concurrent;
 using System.Collections.Frozen;
 using System.Collections.Generic;
-using System.Collections.Immutable;
 using System.Collections.ObjectModel;
 using System.ComponentModel;
 using System.Diagnostics;
@@ -40,7 +39,7 @@
 namespace Microsoft.DotNet.Build.Tasks.Feed
 {
 
-#if !NET472_OR_GREATER
+#if NET
     /// <summary>
     /// The intended use of this task is to push artifacts described in
     /// a build manifest to a static package feed.
@@ -376,11 +375,6 @@ protected void CheckForInternalBuildsOnPublicFeeds(TargetFeedConfig feedConfig)
         /// </summary>
         public void CheckForStableAssetsInNonIsolatedFeeds()
         {
-            if (BuildModel.Identity.IsReleaseOnlyPackageVersion || SkipSafetyChecks)
-            {
-                return;
-            }
-
             foreach (var packagesPerCategory in PackagesByCategory)
             {
                 var category = packagesPerCategory.Key;
@@ -391,13 +385,13 @@ public void CheckForStableAssetsInNonIsolatedFeeds()
                     foreach (var feedConfig in feedConfigsForCategory)
                     {
                         // Look at the version numbers. If any of the packages here are stable and about to be published to a
-                        // non-isolated feed, then issue an error. Isolated feeds may recieve all packages.
+                        // non-isolated feed, then issue an error. Isolated feeds may receive all packages.
                         if (feedConfig.Isolated)
                         {
                             continue;
                         }
 
-                        HashSet<PackageArtifactModel> filteredPackages = FilterPackages(packages, feedConfig);
+                        HashSet<PackageArtifactModel> filteredPackages = SplitPackageByAssetSelection(packages, feedConfig);
 
                         foreach (var package in filteredPackages)
                         {
@@ -414,7 +408,7 @@ public void CheckForStableAssetsInNonIsolatedFeeds()
                             // use by the public. This is for technical (can't overwrite the original packages) reasons as well as 
                             // to avoid confusion. Because .NET core generally brands its "final" bits without prerelease version
                             // suffixes (e.g. 3.0.0-preview1), test to see whether a prerelease suffix exists.
-                            else if (!version.IsPrerelease)
+                            else if (!version.IsPrerelease && package.CouldBeStable != false)
                             {
                                 Log.LogError($"Package '{package.Id}' has stable version '{package.Version}' but is targeted at a non-isolated feed '{feedConfig.TargetURL}'");
                             }
@@ -778,7 +772,7 @@ protected async Task HandlePackagePublishingAsync(ReadOnlyDictionary<string, Ass
                 {
                     foreach (var feedConfig in feedConfigsForCategory)
                     {
-                        HashSet<PackageArtifactModel> filteredPackages = FilterPackages(packages, feedConfig);
+                        HashSet<PackageArtifactModel> filteredPackages = SplitPackageByAssetSelection(packages, feedConfig);
 
                         foreach (var package in filteredPackages)
                         {
@@ -817,7 +811,7 @@ protected async Task HandlePackagePublishingAsync(ReadOnlyDictionary<string, Ass
             Log.LogMessage(MessageImportance.High, "\nCompleted publishing of packages: ");
         }
 
-        private HashSet<PackageArtifactModel> FilterPackages(HashSet<PackageArtifactModel> packages, TargetFeedConfig feedConfig)
+        protected virtual HashSet<PackageArtifactModel> SplitPackageByAssetSelection(HashSet<PackageArtifactModel> packages, TargetFeedConfig feedConfig)
         {
             return feedConfig.AssetSelection switch
             {

src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestV3.cs

@@ -181,7 +181,10 @@ public override async Task<bool> ExecuteAsync()
                     }
                 }
 
-                CheckForStableAssetsInNonIsolatedFeeds();
+                if (!BuildModel.Identity.IsReleaseOnlyPackageVersion && !SkipSafetyChecks)
+                {
+                    CheckForStableAssetsInNonIsolatedFeeds();
+                }
 
                 if (Log.HasLoggedErrors)
                 {

src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestV4.cs

@@ -140,11 +140,10 @@ public override async Task<bool> ExecuteAsync()
 
                     var targetFeedsSetup = new SetupTargetFeedConfigV4(
                         targetChannelConfig: targetChannelConfig,
+                        buildModel: BuildModel,
                         isInternalBuild: targetChannelConfig.IsInternal,
-                        isStableBuild: BuildModel.Identity.IsStable,
                         repositoryName: BuildModel.Identity.Name,
                         commitSha: BuildModel.Identity.Commit,
-                        publishInstallersAndChecksums: PublishInstallersAndChecksums,
                         feedKeys: FeedKeys,
                         feedSasUris: FeedSasUris,
                         feedOverrides: AllowFeedOverrides ? FeedOverrides : Array.Empty<ITaskItem>(),
@@ -177,7 +176,10 @@ public override async Task<bool> ExecuteAsync()
                     }
                 }
 
-                CheckForStableAssetsInNonIsolatedFeeds();
+                if (!SkipSafetyChecks)
+                {
+                    CheckForStableAssetsInNonIsolatedFeeds();
+                }
 
                 if (Log.HasLoggedErrors)
                 {
@@ -219,6 +221,21 @@ public string GetFeed(string feed, string feedOverride)
             return (AllowFeedOverrides && !string.IsNullOrEmpty(feedOverride)) ? feedOverride : feed;
         }
 
+        protected override HashSet<PackageArtifactModel> SplitPackageByAssetSelection(HashSet<PackageArtifactModel> packages, TargetFeedConfig feedConfig)
+        {
+            return feedConfig.AssetSelection switch
+            {
+                AssetSelection.All => packages,
+                AssetSelection.NonShippingOnly => packages.Where(p => p.NonShipping && p.CouldBeStable != true).ToHashSet(),
+                AssetSelection.ShippingOnly => packages.Where(p => !p.NonShipping && p.CouldBeStable != true).ToHashSet(),
+                AssetSelection.CouldBeStable => packages.Where(p => p.CouldBeStable == true).ToHashSet(),
+
+                // Throw NIE here instead of logging an error because error would have already been logged in the
+                // parser for the user.
+                _ => throw new NotImplementedException($"Unknown asset selection type '{feedConfig.AssetSelection}'")
+            };
+        }
+
         public PublishArtifactsInManifestV4(AssetPublisherFactory assetPublisherFactory = null) : base(assetPublisherFactory)
         {
         }

src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishSignedAssets.cs

@@ -5,6 +5,7 @@
 using Azure.Identity;
 using Microsoft.Build.Framework;
 using Microsoft.DotNet.Build.Tasks.Feed.Model;
+using Microsoft.DotNet.VersionTools.BuildManifest.Model;
 using NuGet.Packaging;
 using NuGet.Packaging.Core;
 using System;

src/Microsoft.DotNet.Build.Tasks.Feed/src/model/SetupTargetFeedConfigBase.cs

@@ -10,7 +10,6 @@ namespace Microsoft.DotNet.Build.Tasks.Feed
     public abstract class SetupTargetFeedConfigBase
     {
         protected bool IsInternalBuild { get; set; }
-        protected bool IsStableBuild { get; set; }
         protected string RepositoryName { get; set; }
         protected string CommitSha { get; set; }
         protected bool PublishInstallersAndChecksums { get; set; }
@@ -24,8 +23,8 @@ public abstract class SetupTargetFeedConfigBase
         protected ImmutableList<string> LatestLinkShortUrlPrefixes { get; set; }
         protected string AzureDevOpsFeedsKey { get; set; }
 
-        protected SetupTargetFeedConfigBase(bool isInternalBuild,
-            bool isStableBuild,
+        protected SetupTargetFeedConfigBase(
+            bool isInternalBuild,
             string repositoryName,
             string commitSha,
             bool publishInstallersAndChecksums,
@@ -40,7 +39,6 @@ protected SetupTargetFeedConfigBase(bool isInternalBuild,
             string azureDevOpsFeedsKey)
         {
             IsInternalBuild = isInternalBuild;
-            IsStableBuild = isStableBuild;
             RepositoryName = repositoryName;
             CommitSha = commitSha;
             PublishInstallersAndChecksums = publishInstallersAndChecksums;

src/Microsoft.DotNet.Build.Tasks.Feed/src/model/SetupTargetFeedConfigV3.cs

@@ -30,6 +30,9 @@ public class SetupTargetFeedConfigV3 : SetupTargetFeedConfigBase
 
         public string AzureDevOpsOrg => "dnceng";
 
+        protected bool IsStableBuild { get; set; }
+
+
         public SetupTargetFeedConfigV3(
             TargetChannelConfig targetChannelConfig,
             bool isInternalBuild,
@@ -48,8 +51,21 @@ public SetupTargetFeedConfigV3(
             ImmutableList<string> filesToExclude = null,
             bool flatten = true,
             TaskLoggingHelper log = null) 
-            : base(isInternalBuild, isStableBuild, repositoryName, commitSha, publishInstallersAndChecksums, null, null, null, null, null, null, null, latestLinkShortUrlPrefixes, null)
+            : base(isInternalBuild: isInternalBuild,
+                   repositoryName: repositoryName,
+                   commitSha: commitSha,
+                   publishInstallersAndChecksums: publishInstallersAndChecksums,
+                   installersTargetStaticFeed: null,
+                   installersAzureAccountKey: null,
+                   checksumsTargetStaticFeed: null,
+                   checksumsAzureAccountKey: null,
+                   azureDevOpsStaticShippingFeed: null,
+                   azureDevOpsStaticTransportFeed: null,
+                   azureDevOpsStaticSymbolsFeed: null,
+                   latestLinkShortUrlPrefixes: latestLinkShortUrlPrefixes,
+                   azureDevOpsFeedsKey: null)
         {
+            IsStableBuild = isStableBuild;
             _targetChannelConfig = targetChannelConfig;
             BuildEngine = buildEngine;
             StableSymbolsFeed = stableSymbolsFeed;

src/Microsoft.DotNet.Build.Tasks.Feed/src/model/SetupTargetFeedConfigV4.cs

@@ -9,6 +9,7 @@
 using Microsoft.Build.Framework;
 using Microsoft.Build.Utilities;
 using Microsoft.DotNet.Build.Tasks.Feed.Model;
+using Microsoft.DotNet.VersionTools.BuildManifest.Model;
 
 namespace Microsoft.DotNet.Build.Tasks.Feed
 {
@@ -30,13 +31,14 @@ public class SetupTargetFeedConfigV4 : SetupTargetFeedConfigBase
 
         public string AzureDevOpsOrg => "dnceng";
 
+        private readonly BuildModel _buildModel;
+
         public SetupTargetFeedConfigV4(
             TargetChannelConfig targetChannelConfig,
+            BuildModel buildModel,
             bool isInternalBuild,
-            bool isStableBuild,
             string repositoryName,
             string commitSha,
-            bool publishInstallersAndChecksums,
             ITaskItem[] feedKeys,
             ITaskItem[] feedSasUris,
             ITaskItem[] feedOverrides,
@@ -48,9 +50,22 @@ public SetupTargetFeedConfigV4(
             ImmutableList<string> filesToExclude = null,
             bool flatten = true,
             TaskLoggingHelper log = null)
-            : base(isInternalBuild, isStableBuild, repositoryName, commitSha, publishInstallersAndChecksums, null, null, null, null, null, null, null, latestLinkShortUrlPrefixes, null)
+            : base(isInternalBuild: isInternalBuild,
+                   repositoryName: repositoryName,
+                   commitSha: commitSha,
+                   publishInstallersAndChecksums: true,
+                   installersTargetStaticFeed: null,
+                   installersAzureAccountKey: null,
+                   checksumsTargetStaticFeed: null,
+                   checksumsAzureAccountKey: null,
+                   azureDevOpsStaticShippingFeed: null,
+                   azureDevOpsStaticTransportFeed: null,
+                   azureDevOpsStaticSymbolsFeed: null,
+                   latestLinkShortUrlPrefixes: latestLinkShortUrlPrefixes,
+                   azureDevOpsFeedsKey: null)
         {
             _targetChannelConfig = targetChannelConfig;
+            _buildModel = buildModel;
             BuildEngine = buildEngine;
             StableSymbolsFeed = stableSymbolsFeed;
             StablePackagesFeed = stablePackagesFeed;
@@ -85,15 +100,14 @@ public override List<TargetFeedConfig> Setup()
 
         private IEnumerable<TargetFeedConfig> Feeds()
         {
-            // If the build is stable, we need to create two new feeds (if not provided)
-            // that can contain stable packages. These packages cannot be pushed to the normal
-            // feeds specified in the feed config because it would mean pushing the same package more than once
-            // to the same feed on successive builds, which is not allowed.
+            // We create stable feeds if any of the package assets within the build
+            // could be stable. In V4, build stability is ignored.
+
+            bool generateStableFeeds = _buildModel.Artifacts.Packages.Any(p => p.CouldBeStable.HasValue && p.CouldBeStable.Value == true);
 
-            if (IsStableBuild)
+            if (generateStableFeeds)
             {
                 CreateStablePackagesFeedIfNeeded();
-                CreateStableSymbolsFeedIfNeeded();
 
                 yield return new TargetFeedConfig(
                     TargetFeedContentType.Package,
@@ -103,20 +117,7 @@ private IEnumerable<TargetFeedConfig> Feeds()
                     LatestLinkShortUrlPrefixes,
                     _targetChannelConfig.AkaMSCreateLinkPatterns,
                     _targetChannelConfig.AkaMSDoNotCreateLinkPatterns,
-                    assetSelection: AssetSelection.ShippingOnly,
-                    symbolPublishVisibility: SymbolServerVisibility,
-                    isolated: true,
-                    @internal: IsInternalBuild,
-                    flatten: Flatten);
-
-                yield return new TargetFeedConfig(
-                    TargetFeedContentType.Symbols,
-                    StableSymbolsFeed,
-                    FeedType.AzDoNugetFeed,
-                    AzureDevOpsFeedsKey,
-                    LatestLinkShortUrlPrefixes,
-                    _targetChannelConfig.AkaMSCreateLinkPatterns,
-                    _targetChannelConfig.AkaMSDoNotCreateLinkPatterns,
+                    assetSelection: AssetSelection.CouldBeStable,
                     symbolPublishVisibility: SymbolServerVisibility,
                     isolated: true,
                     @internal: IsInternalBuild,
@@ -127,21 +128,6 @@ private IEnumerable<TargetFeedConfig> Feeds()
             {
                 foreach (var type in spec.ContentTypes)
                 {
-                    if (!PublishInstallersAndChecksums)
-                    {
-                        if (PublishingConstants.InstallersAndChecksums.Contains(type))
-                        {
-                            continue;
-                        }
-                    }
-
-                    // If dealing with a stable build, the package feed targeted for shipping packages and symbols
-                    // should be skipped, as it is added above.
-                    if (IsStableBuild && ((type is TargetFeedContentType.Package && spec.Assets == AssetSelection.ShippingOnly) || type is TargetFeedContentType.Symbols))
-                    {
-                        continue;
-                    }
-
                     var oldFeed = spec.FeedUrl;
                     var feed = GetFeedOverride(oldFeed);
                     if (type is TargetFeedContentType.Package &&
@@ -188,34 +174,6 @@ private IEnumerable<TargetFeedConfig> Feeds()
             }
         }
 
-        /// <summary>
-        /// Create the stable symbol packages feed if one is not already explicitly provided
-        /// </summary>
-        /// <exception cref="Exception">Throws if the feed cannot be created</exception>
-        private void CreateStableSymbolsFeedIfNeeded()
-        {
-            if (string.IsNullOrEmpty(StableSymbolsFeed))
-            {
-                var symbolsFeedTask = new CreateAzureDevOpsFeed()
-                {
-                    BuildEngine = BuildEngine,
-                    AzureDevOpsOrg = AzureDevOpsOrg,
-                    AzureDevOpsProject = IsInternalBuild ? "internal" : "public",
-                    AzureDevOpsPersonalAccessToken = AzureDevOpsFeedsKey,
-                    RepositoryName = RepositoryName,
-                    CommitSha = CommitSha,
-                    ContentIdentifier = "sym"
-                };
-
-                if (!symbolsFeedTask.Execute())
-                {
-                    throw new Exception($"Problems creating an AzureDevOps (symbols) feed for repository '{RepositoryName}' and commit '{CommitSha}'.");
-                }
-
-                StableSymbolsFeed = symbolsFeedTask.TargetFeedURL;
-            }
-        }
-
         /// <summary>
         /// Create the stable packages feed if one is not already explicitly provided
         /// </summary>

src/Microsoft.DotNet.Build.Tasks.Feed/src/model/TargetFeedConfig.cs

@@ -189,6 +189,7 @@ public enum AssetSelection
     {
         All,
         ShippingOnly,
-        NonShippingOnly
+        NonShippingOnly,
+        CouldBeStable, // V4 only. Assets that could be stable (whether shipping or non-shipping)
     }
 }