Improve build reliability

[dougbu]

• see if this solves Mysterious type/namespace errors in release/3.1 #20687
• ensure ResolveCustomReferences target executes before packages are used
• ResolveAssemblyReferences and ResolveAssemblyReferencesDesignTime targets run too late
  • e.g. failed builds of Microsoft.AspNetCore.WebUtilities or Microsoft.AspNetCore.Hosting when building from root
• add GetReferenceProjectTargetPathMetadata for ease of use as well as reliability
  • avoids extra work to get existing metadata (ref/ projects execute no tasks in this target)

nit: rename @(ReferenceProjectMetadata) -> @(ReferenceProjectTargetPathMetadata)

[dougbu]

@wtgodbe I dug this out of my work to eliminate most *.Manual.cs files in the 'release/3.1' branch. Sorry that it took me a while to remember I'd seen strange cases in which ResolveCustomReferences didn't run everywhere it should. Fortunately, this should work w/o the rest of my (much larger) change.

[dougbu]

/fyi there were two things wrong w/ the old BeforeTargets value:

1. ResolveAssemblyReferencesDesignTime didn't matter because that depends on ResolveAssemblyReferences
2. ResolveAssemblyReferences runs later (closer to CoreCompile) than needed to ensure information needed from packages is available where / when it needs to be

[dougbu]

/fyi depending on %(OutputItemType) metadata works most of the time but requires the ref/ project to build in some contexts where it isn't. This is more important when doing ApiCompat checks but might help in general.

[wtgodbe]

@dougbu you are a superhero! Looking forward to hearing the explanation of this one on Monday

[dougbu]

@wtgodbe no heroes here, especially because template tests are failing. (Did you kick off a retry? That produced the same 11/29 template test failures as the first try.)

@markwilkie why does Arcade use 42.42.42.42 as the default assembly version in non-official builds? Because the dotnet/aspnetcore infrastructure chooses 3.1.0.0 for ref/ assemblies, we end up with implementation assemblies that are incompatible i.e. implementation assemblies have a much higher assembly version. Actually a bit surprising this works at all in our test projects. But, the version change and a mix of .NET Core assemblies that build against ref/ assemblies with .NET Standard assemblies that don't explains the template test failures.

Is there a major downside to setting $DotNetUseShippingVersions) to true in all builds? That would lead to the usual (compatible) 3.1.0.0 / 3.1.4.0 assembly versions for ref/ and src/ assemblies. I see the note in the Arcade SDK's tools/Version.BeforeCommonTargets.targets file about builds not being deterministic without a passed-in $(OfficialBuild) value i.e. outside official builds but am not certain that's a major problem. Do we need PR builds and rolling builds to produce unchanged outputs for some reason?

If changing $DotNetUseShippingVersions) isn't viable, could add more direct references to the template projects. But, that gets pretty ugly pretty fast.

[dougbu]

/fyi https://dev.azure.com/dnceng/public/_build/results?buildId=597609 was the build that failed multiple template tests due to incompatible assembly versions.

[dougbu]

https://dev.azure.com/dnceng/public/_build/results?buildId=597668 shows $(DotNetUseShippingVersions) setting doesn't make assembly versions more consistent.

Not sure why yet but most if not all template test failures involve Microsoft.AspNetCore.SpaServices.Extensions, Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore, or Microsoft.AspNetCore.ApiAuthorization.IdentityServer failing to load multiple versions of Microsoft.AspNetCore.Http.Abstractions or Microsoft.AspNetCore.ApiAuthorization.IdentityServer failing to load multiple versions of Microsoft.AspNetCore.Authentication.

[markwilkie]

why does Arcade use 42.42.42.42 as the default assembly version in non-official builds?

Sadly, I don't remember anymore why. @tmat or @mmitche - can you refresh out memory?

[tmat]

why does Arcade use 42.42.42.42 as the default assembly version in non-official builds?

Using an artificial version number that's higher than any version we will ship to discover issues where a binding redirect is missing or some code uses a hard-coded version. This artificial version number makes version mismatches more obvious earlier.

Is there a major downside to setting $(DotNetUseShippingVersions) to true in all builds?

Yes, this property should not be set in targets/props files. It's there to provide a local override in case a dev wants to build private bits with the right versions locally. It should not be a permanent setting.

[dougbu]

Thanks for the added information @markwilkie and @tmat. It turns out I can't turn off the 42.42.42.42 assembly versioning without significant hacks. $((AutoGenerateAssemblyVersion), $(DotNetUseShippingVersions), and the two together leave the template tests failing in about the same way.

I've reproduced the problem locally and will let this PR sit 'til I have a working fix. Suspect the issue is a missing workaround in our template tests. Those tests run without the Arcade SDK and the majority of our infrastructure. And, the problem shows up more clearly in this PR because the build is more consistent now.

[wtgodbe]

Example of template failures:

Project new angular failed to publish.\r\nProcess exited with code 1\nStdErr: \nStdOut: Microsoft (R) Build Engine version 16.4.0+e901037fe for .NET Core\r\nCopyright (C) Microsoft Corporation. All rights reserved.\r\n\r\nF:\workspace_work\1\s.dotnet\sdk\3.1.103\MSBuild.dll -maxcpucount -property:Configuration=Release -restore -target:Publish -verbosity:m /bl .\AspNet.angularnoauth.2srn44u1ibc.csproj\r\n Restore completed in 45.52 ms for F:\workspace_work\1\s\src\ProjectTemplates\test\bin\Release\netcoreapp3.1\TestTemplates\AspNet.angularnoauth.2srn44u1ibc\AspNet.angularnoauth.2srn44u1ibc.csproj.\r\nStartup.cs(49,17): error CS1705: Assembly 'Microsoft.AspNetCore.SpaServices.Extensions' with identity 'Microsoft.AspNetCore.SpaServices.Extensions, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60' uses 'Microsoft.AspNetCore.Http.Abstractions, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60' which has a higher version than referenced assembly 'Microsoft.AspNetCore.Http.Abstractions' with identity 'Microsoft.AspNetCore.Http.Abstractions, Version=3.1.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' [F:\workspace_work\1\s\src\ProjectTemplates\test\bin\Release\netcoreapp3.1\TestTemplates\AspNet.angularnoauth.2srn44u1ibc\AspNet.angularnoauth.2srn44u1ibc.csproj]\r\nStartup.cs(61,13): error CS1705: Assembly 'Microsoft.AspNetCore.SpaServices.Extensions' with identity 'Microsoft.AspNetCore.SpaServices.Extensions, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60' uses 'Microsoft.AspNetCore.Http.Abstractions, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60' which has a higher version than referenced assembly 'Microsoft.AspNetCore.Http.Abstractions' with identity 'Microsoft.AspNetCore.Http.Abstractions, Version=3.1.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' [F:\workspace_work\1\s\src\ProjectTemplates\test\bin\Release\netcoreapp3.1\TestTemplates\AspNet.angularnoauth.2srn44u1ibc\AspNet.angularnoauth.2srn44u1ibc.csproj]\r\n\r\nExpected: True\r\nActual: False

Will look locally

[dougbu]

This is apparently a problem in how a few of the projects are built, not something in the templates or the tests. For example, the references in Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.dll are incorrect for Microsoft.AspNetCore.Http.Abstractions:

// Microsoft.AspNetCore.Http.Abstractions, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60 (unresolved)
// Microsoft.AspNetCore.Http.Features, Version=3.1.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60 (unresolved)
// Microsoft.EntityFrameworkCore, Version=3.1.4.0, Culture=neutral, PublicKeyToken=adb9793829ddae60 (unresolved)
// Microsoft.EntityFrameworkCore.Relational, Version=3.1.4.0, Culture=neutral, PublicKeyToken=adb9793829ddae60 (unresolved)
// Microsoft.Extensions.Logging.Abstractions, Version=3.1.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60 (unresolved)
...

My reliability improvement is insufficient. Digging…

[dougbu]

Root cause is the src/ project's GetReferenceProjectTargetPathMetadata target calls the ref/ project's GetTargetPathMetadata target without passing the $(TargetFramework) (see https://github.com/dotnet/aspnetcore/blob/dougbu/reliable.builds.20687/eng/targets/ResolveReferences.targets#L273). Every ref/ project multi-targets and, so, GetTargetPathMetadata returns nothing The upshot is metadata about ref/ assemblies isn't gathered correct. Fix is pretty simple though testing locally will take a bit longer.

(The branch I pulled the change from doesn't multi-target ref/ projects, avoiding this issue.)

[dougbu]

🆙📅 with fix and to revert @wtgodbe's #20736 workarounds. Also rebased on latest from 'release/3.1' branch.

@wtgodbe @Pilchie we did #20736 as a tell-mode fix. I believe this fits there too. But, timing on merging this (assuming validation succeeds beyond my laptop) is up to you. Thoughts?

[wtgodbe]

I think we should hold off on this for 3.1.5, given that it doesn't strictly fix a bug, but rather is a more desirable solution to a bug we've already "fixed" - and we are relatively close to getting a build for 3.1.4 already

[Pilchie]

I agree, though I think we need to be extra careful to validate the dependencies of our packages in 3.1.4 to make sure nothing changed.

[dougbu]

Sounds like a plan. Anyone already have a script that runs through two sets of .nupkg files and compares the contained .nuspec?

[dougbu]

Any objection to creating the 3.1.5 milestone?

[wtgodbe]

I did a good amount of manual validation on my fix - the references in stuff I checked in the shared framework & in a few packages looked good, but I didn't programmatically validate everything

[dougbu]

Back to this PR:

• @dotnet/aspnet-build please review
• Are we ready to add a 3.1.4 milestone?

[wtgodbe]

Looks good, assuming assembly references look good locally compared to 3.1.3

[Pilchie]

Are we ready to add a 3.1.4 milestone?

I think you mean 3.1.5, right? Fine with me.

For my understanding, this has always been a race condition and we got lucky, or...?

[dougbu]

Yes, 3.1.5 ☹️

For my understanding, this has always been a race condition and we got lucky, or...?

Right, it's always been a race condition.