Enable publishing to build storage

[NikolaMilosavljevic]

Contributes to dotnet/source-build#4101

Enables publishing to local build storage. It will be used in VMR and requires a new optional task parameter.

Some highlights:

• A bit of refactoring of some of arcade projects to enable building in source-only build
• Removal of NuGet.Packaging dependency and implementation of required APIs
• Exclusion of a set of sources in Microsoft.DotNet.Build.Tasks.Feed project
• Renaming of PushToAzureDevOpsArtifacts task to PushToBuildStorage

I've verified that regular repo build and source-build are working fine with these changes. I have also fully verified Windows and Linux VMR build Additional PRs for installer and other repos will follow soon.

Full set of VMR changes for my testing can be seen here: https://github.com/dotnet/dotnet/compare/d3e9fdb1..9ac3fddf

[ViktorHofer]

You only need the new switch.

Was there a reason to not build this project? I think I would opt towards building as much as possible in source-build to reduce the build diff.

[NikolaMilosavljevic]

Building this would introduce a dozen of prebuilts. The project was already excluded before at the higher level dir.

[ViktorHofer]

Which prebuilts? I only see it referencing System.CommandLine which is available in source-build. I understand that it was excluded before.

[NikolaMilosavljevic]

I'm going to keep both properties. This essentially moves the existing infra from

arcade/src/Microsoft.DotNet.VersionTools/Directory.Build.props

Lines 5 to 9 in 2215d18

	<!-- Don't include VersionTools in source-build until we find a reason to use it. -->
	<PropertyGroup>
	<ExcludeFromSourceBuild>true</ExcludeFromSourceBuild>
	<ExcludeFromSourceOnlyBuild>true</ExcludeFromSourceOnlyBuild>
	</PropertyGroup>

into project file.

Removing ExcludeFromSourceBuild property would likely require building more of the other arcade projects, that this one depends on, that are currently disabled with ExcludeFromSourceBuild.

I believe the comment is still true - we do not enable this unless we find a reason to use it.

[ViktorHofer]

ExcludeFromSourceBuild is unnecessary in Arcade's own build. The old switch is only required in repositories that still use Arcade 8. Arcade main depends on Arcade 9 so it doesn't need the old switch anymore. I'm happy to submit a separate PR if you rather not want to include that change in your PR. EDIT: Submitted #14564

I believe the comment is still true - we do not enable this unless we find a reason to use it.

This doesn't make sense to me. Nowhere else in the stack do we disable projects when building from source unless they would bring in prebuilts or add significant cost to the build, i.e. VS insertion packages, workloads, etc. Reducing the build difference between source-build and msft-build has been a goal for years by your team so I'm surprised that we aren't in agreement here.

[NikolaMilosavljevic]

I don't think there is a disagreement. I'm merely pointing out a large difference in a set of arcade projects that build in MSFT vs source-build. For the work I did to enable just one of the projects, I had to modify an unexpected number of dependent projects. I was trying to limit the set of projects I enable today, to what is actually required by this work.

I will however update these conditions as I misunderstood the usage in arcade build.

[ViktorHofer]

No worries, #14564 will take care of those.

[NikolaMilosavljevic]

Fixed with 913dbfb

[ViktorHofer]

Thanks for removing the unnecessary property. My point still stands that we whould build the Cli and Task projects as those don't introduce new prebuilts do minimize the difference between the msft-build and source-build but I'm fine if you do that as a follow-up after this change is in to reduce the build diff in this PR.

[ViktorHofer]

Addressed via #14588

[ViktorHofer]

You are probably aware of this but just to mention it here, this will need reaction in multiple repos that currently call this task directly: https://github.com/search?q=org%3Adotnet%20PushToAzureDevOpsArtifacts&type=code

[NikolaMilosavljevic]

You are probably aware of this but just to mention it here, this will need reaction in multiple repos that currently call this task directly: https://github.com/search?q=org%3Adotnet%20PushToAzureDevOpsArtifacts&type=code

Yes, we've discussed this in dotnet/source-build#4101. We'll need to update those references when new arcade flows to those repos.

[MilenaHristova]

@NikolaMilosavljevic I recently added DotNetReleaseShipping=true metadata in the manifest for assets that get shipped with the release infra, is this preserved with your change?

Here is the issue: https://github.com/dotnet/release/issues/785 - I added a property ProducesDotNetReleaseShippingAssets in (Publishing.props) in the shipping repos and it's getting propagated to the assets manifest in Publish.proj. It should be set for all shipping assets and packages produced from the repos that have ProducesDotNetReleaseShippingAssets set to true.

[NikolaMilosavljevic]

DotNetReleaseShipping

Yes, the metadata exists in several repo manifests - aspnetcore, emsdk, roslyn-analyzers, runtime, sdk and templating.

[ViktorHofer]

Suggested change

	private readonly string _id;
	private readonly string _version;
	public PackageIdentity(string id, string version)
	{
	if (id == null)
	{
	throw new ArgumentNullException(nameof(id));
	}
	_id = id;
	_version = version;
	}
	public string Id
	{
	get { return _id; }
	}
	public string Version
	{
	get { return _version; }
	}
	public string Id { get; init; }
	public string Version { get; init; }
	public PackageIdentity(string id, string version)
	{
	if (id == null)
	{
	throw new ArgumentNullException(nameof(id));
	}
	Id = id;
	Version = version;
	}

[NikolaMilosavljevic]

Interestingly, this did not compile in VMR build. Of course, it works fine in simple VS projects.

I will revert this change when I push the next set of changes to address other comments. Old code was totally fine anyway, and is used by NuGet.Client as well: https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Packaging/Core/PackageIdentity.cs

[ViktorHofer]

Interesting, this would mean that we use a different compiler that doesn't support this syntax when building arcade inside the VMR. That would be very problematic. I will play around with this post-merge.

[mmitche]

Pretty interesting that this wouldn't compile. Maybe we're passing an older langversion somehow?

[ViktorHofer]

Addressed via #14589

[ViktorHofer]

This should be possible in-memory without writing to disk by using the Open API.

[ViktorHofer]

Addressed via #14589

[ViktorHofer]

@mmitche I did a review of most of the msbuild stuff but I only glanced at the publishing C# code. Can you please also take look?