MergeAssetManifests task doesn't work correctly

[ViktorHofer]

he produced merged manifest has the wrong VMR build number right now: ".

See https://github.com/dotnet/installer/blob/58e4c4f2e4fc66597ebf4ff54809dc88f286e217/src/SourceBuild/content/build.proj#L42 & https://github.com/dotnet/installer/blob/58e4c4f2e4fc66597ebf4ff54809dc88f286e217/src/SourceBuild/content/Directory.Build.props#L221

The VmrBuildNumber property has a typo in it, the trailing ". If I remove that, then I get errors that VmrBuildNumber isn't set. This makes me assume that BUILD_BUILDNUMBER isn't set.

@dkurepa

[dkurepa]

Yeah this is weird. It should be set according to the docs. Might be safer to pass it as a parameter through yaml. I was going to do that for the VerticalName anyway, since we need this information to know which artifact to download

[akoeplinger]

hm weird, I looked at the console log and BUILD_BUILDNUMBER is in the list of environment variables we print before each repo build.

[dkurepa]

yeah, this variable is used by the Publish.proj when creating the AssetManifests, and we change set it before every repo build to match the BuildNumber in BAR for that commit. However, it should also be available before we start changing it

[ViktorHofer]

Looking at the binlog I see that it is manually set as an env var by us in the inner repo builds but it isn't available in Build.proj where we need it.

This task should work when doing a non-official build as well, therefore we need a different value than OfficialBuildId.

Could we use _BuildNumber which is set by Arcade? https://github.com/dotnet/arcade/blob/b6fada3ec4fa37e08dcbafaa6ddf59213f3f8687/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L47

For that you would need to set DotNetUseShippingVersions in Build.proj.

[ViktorHofer]

cc @mmitche

[dkurepa]

Looking at the binlog I see that it is manually set as an env var by us in the inner repo builds but it isn't available in Build.proj where we need it.

This task should work when doing a non-official build as well, therefore we need a different value than OfficialBuildId.

Could we use _BuildNumber which is set by Arcade? https://github.com/dotnet/arcade/blob/b6fada3ec4fa37e08dcbafaa6ddf59213f3f8687/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L47

For that you would need to set DotNetUseShippingVersions in Build.proj.

wouldn't this cause the VmrBuildNumber to be the same for all builds in the same day?

If we decided to pass the Build.BuildNumber as a parameter, then we could also put something there when doing non official builds

[ViktorHofer]

wouldn't this cause the VmrBuildNumber to be the same for all builds in the same day?

That's already the current state when doing a dev or ci build. You will get package versions like 9.0.0-dev or 9.0.0-ci. Thinking more about this, I think what we actually want to set here is just Version which respects the OfficialBuildId in official builds.

We should also update these properties: https://github.com/dotnet/dotnet/blob/896eba009b4a97dd505199bed888dfc2f04c4b44/eng/Versions.props#L4-L5

to something like this:

<MajorVersion>9</MajorVersion>
<MinorVerison>0</MinorVersion>
<PatchVersion>0</PatchVersion>
<PreReleaseVersionLabel>alpha</PreReleaseVersionLabel>
<PreReleaseVersionIteration>1</PreReleaseVersionIteration>

[mmitche]

In the repos, it's passed in via OfficialBuildID. I think we should continue that pattern here.

[dkurepa]

In my dev branch I'm currently doing this:

• Passing the OfficialBuildID as a parameter for MSBUILD
• If it's not passed setting it to something in base Directory.Build.props
• When doing repo-proj builds, removing it with RemoveProperties, since the repo builds want to change it, but are not able to since it's a global property

[ViktorHofer]

In the repos, it's passed in via OfficialBuildID. I think we should continue that pattern here.

What is used as the input when not doing an official build?

[mmitche]

When not doing an official build (and not using a switch that emulates it), the build number isn't set. Instead the build revision and other info derived from the build number end up as constants.

https://github.com/dotnet/arcade/blob/b6fada3ec4fa37e08dcbafaa6ddf59213f3f8687/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L46-L47

[ViktorHofer]

Ok. But that sounds like the OfficialBuildId property itself isn't the correct input to the task as it isn't set outside of an official build. Or am I'm missing something?

[mmitche]

I think most likely, people aren't generating manifests outside of official builds. In arcade right now, it only happens when you pass -publish and /p:DotNetPublishUsingPipelines=true

https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Arcade.Sdk/tools/Publish.proj#L76

It does, however, look like BUILD_BUILDNUMBER is used: https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Arcade.Sdk/tools/Publish.proj#L207

[mmitche]

I'm not quite sure why they chose to use that instead of BUILD_BUILDNUMBER to be honest.

[mmitche]

Oh, this is much sillier. @dkurepa @ViktorHofer The issue is that this repo uses explicit docker commands to run its Linux builds. The ambient environment isn't passed through implicitly.

Check the Windows manifest. it has a correct build number (with a quote)

[ViktorHofer]

I think most likely, people aren't generating manifests outside of official builds. In arcade right now, it only happens when you pass -publish and /p:DotNetPublishUsingPipelines=true

Right. Repos do use the -sign -publish switches on CI because Arcade's cibuild.cmd/sh appends those but DotNetPublishUsingPipelines only gets set to true when doing an official build.

Now inside the VMR, the inner repos do publish and set DotNetPublishUsingPipelines to true copy the files to disk. An example from command-line-api public CI build:

When we stop treating everything as an official build, we won't pass OfficialBuildId in anymore. What would the value then be?

[mmitche]

Probably an arbitrary value? A date? Empty string?

[ViktorHofer]

If that metadata doesn't matter unless we actually publish to AzDO / BAR then using OfficialBuildId sounds right. That would then just be empty in non-official builds.

[mmitche]

To summarize:

• I want to go back on my statement that OfficialBuildId should be used for the outer manifest creation. It looks like we use BUILD_BUILDNUMBER in arcade to generate the manifest. OfficialBuildId is used to generate package versions. While we could use OfficialBuildId in place of BUILD_BUILDNUMBER, there are other properties that are AzDO specific when generating manifests so keeping it consistent is better IMO.
• The actual metadata does not matter except when publishing to BAR, so empty strings and all that are fine in local builds
• BUILD_BUILDNUMBER is unset (along with other AzDO properties) because the env vars aren't explicitly fed into the docker commands.

[ViktorHofer]

This task parameter needs an update to allow an empty string. Please also update the xml doc above with some of this information:

The actual metadata does not matter except when publishing to BAR, so empty strings and all that are fine in local builds

Please also add a comment in the target where you pass the BUILD_BUILDNUMBER in and mention that it's fine if this value is empty.

[ViktorHofer]

This got fixed by dotnet/installer#19217