Add Join-Verticals task, use it at the final Join Point

[dkurepa]

dotnet/source-build#4199
Test run https://dev.azure.com/dnceng/internal/_build/results?buildId=2426483&view=results

A small note, we're publishing VerticalManifests as build artifacts because we want every leg to publish it's own into the same artifact. This is causing some SBOM manifest generation errors. The pipeline still runs as expected, but it's not green

[dkurepa]

@ViktorHofer I wasn't able to figure out how to do this in msbuild, so I made it an optional task property, let me know if you're OK with this

[mmitche]

I don't think you can have a required parameter that is null.

[ViktorHofer]

I would call this project "publish-final.proj" and the target "PublishFinal":

Suggested change

	<Target Name="JoinVerticals " AfterTargets="Build">
	<Target Name="PublishFinal" AfterTargets="Build">

[dkurepa]

I don't think we should call it publish-final, as this will also be used to download artifacts for join point builds like we discussed. I named it join-verticals since it can join any number of verticals, including all of them

[ViktorHofer]

I was under the impression that we can't directly use this target for the join but if we can, great.

[dkurepa]

We should be able to, there's a property called VerticalSubSet that has a list of verticals we want to join, so it doesn't have to be the full set. On second thought, this isn't a very good name, perhaps VerticalsToJoin is better.
There's also a FlatCopy property, that tells the task how we want it to copy artifacts, if it's set to true, all packages will be in a flat layout in the packages folder, and if it's not, we'll keep the same layout as it is in the artifacts, so for the non final join points we should keep it false. Same goes for assets

[dkurepa]

Also not sure if we want to publish binlogs for this, we are using a secret as a property so it would appear there

[premun]

Also not sure if we want to publish binlogs for this, we are using a secret as a property so it would appear there

Which secret? Afaik there's a binlog secret redaction tool in Arcade now - maybe we should run it (maybe it runs automatically).

[dkurepa]

Also not sure if we want to publish binlogs for this, we are using a secret as a property so it would appear there

Which secret? Afaik there's a binlog secret redaction tool in Arcade now - maybe we should run it (maybe it runs automatically).

There's an Azdo token we're passing as a property to authenticate to AzDo api

[ViktorHofer]

I'm not sure if vertical is an appropriate term to use here. What this captures is a unique build identifier, right? cc @mmitche

[ViktorHofer]

Would something like ManifestName make more sense? We have the same in Arcade's Publish.proj: https://github.com/dotnet/arcade/blob/87b015b938e5400d6e57afd7650348c17a764b73/src/Microsoft.DotNet.Arcade.Sdk/tools/Publish.proj#L70

[mmitche]

Most repos, like installer, that produce multiple manifests do not call each build a 'vertical', though in essence that is what they are. They also do often have a concept of a primary vertical, but there isn't really any formal term used. It's usually along the lines of: "/p:PublishNugetPackages=true". It's not consistent across repos.

You could just say "ManifestName" here. What differentiates this usage, I think, is that we're really formalizing the 'primary' vertical for join points, and we have to give that some kind of name. Primary/main vertical makes the most sense in that context, vs. "Primary Manifest" or such. Then I think it makes sense to use Vertical here too.

[mmitche]

The project value can come from azdo build variables (System.TeamProject)

[mmitche]

You might consider not explicitly specifying this value, and instead using System.CollectionUri. That would be the base URI (w/collection) that would get passed to the task.

[mmitche]

I'm kinda surprised this works as blank, but I like it if it does. Otherwise System.AccessToken would potentially work.

[mmitche]

Why not print info about the pass being the final pass?

[mmitche]

nit: it would be good to include the build pass in the info printed.

[mmitche]

This functionality probably needs a try/catch w/retry on certain responses.

[mmitche]

A common pattern is EnsureSuccessStatusCode() here with the corresponding try/catch and retry

[mmitche]

Suggested change

	try{
	try
	{

[mmitche]

In addition a number of these objects (e.g. HttpResponseMessage) are IDisposable and should have using blocks.

[mmitche]

I would use SingleOrDefault here to protect against multiple files with the same name being found in the SourceFiles list. In addition, this use be Path.GetFileName(f) rather than .Contains()

[mmitche]

The need/purpose of this method isn't super obvious. Can you add a comment as to why this is written this way?

[mmitche]

Can you add a comment as to what might be missing and why?

[dkurepa]

Pausing this work for a bit, will pick it up at a later point

[ViktorHofer]

As part of the installer->sdk repository consolidation we are already switching the VMR over into dotnet/sdk. ETA: today or tomorrow. The last manual code sync happened today and we don't plan on doing another sync. Please close this PR and submit it into dotnet/sdk. Sorry for the inconvenience.