Build scripts do not separate Restore from Build, causing confusing behavior on builds after package updates

[rainersigwald]

• This issue is blocking
• This issue is causing unreasonable pain

This is the root cause of NuGet/Home#12437, which has caused @jaredpar and the Roslyn team a lot of pain recently.

The Arcade build.proj can invoke NuGet restore and build in the same build request, often when running build.ps1 -restore -build:

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj

Lines 254 to 270 in 3840d43

	<MSBuild Projects="@(_ProjectToRestore)"
	Properties="@(_SolutionRestoreProps)"
	RemoveProperties="$(_RemoveProps);TreatWarningsAsErrors"
	Targets="Restore"
	SkipNonexistentTargets="true"
	BuildInParallel="%(_ProjectToRestore.RestoreInParallel)"
	Condition="'$(Restore)' == 'true'"/>
	<!--
	Build solution.
	-->
	<MSBuild Projects="@(ProjectToBuild)"
	Properties="@(_SolutionBuildProps);__BuildPhase=SolutionBuild;_NETCORE_ENGINEERING_TELEMETRY=Build"
	RemoveProperties="$(_RemoveProps)"
	Targets="@(_SolutionBuildTargets)"
	BuildInParallel="%(ProjectToBuild.BuildInParallel)"
	Condition="'@(_SolutionBuildTargets)' != ''" />

This is not correct. Specifically, it will cause incorrect builds when

• Projects reference NuGet packages
• Those NuGet packages deliver build logic (.props/.targets files)
• the references to those packages have changed since the last build (removed or version changed).

In that case, MSBuild will use stale versions of the imported build logic in the build, because they are imported in the restore operation and MSBuild tries to retain a coherent set of imports for the duration of a build.

It's not possible to do this correctly today (dotnet/msbuild#2811). However, the Arcade/restore integration can be altered to behave correctly by hooking into MSBuild's -restore behavior.

Specifically, instead of passing Restore as a property to Build.proj

arcade/eng/common/build.ps1

Line 116 in 3840d43

/p:Restore=$restore `

the wrapper script should pass (or not pass) -restore, so MSBuild can flush caches at the appropriate point between restore and build.

For that to work, the restore logic would need to be separated from Execute into its own target, which must be named Restore. MSBuild would then call that target, flush caches, then call Execute to do the "build" portion of the build.

This is highly relevant to developer-desktop scenarios where a dev might update a package reference version, and to Roslyn's two-phase build that builds the compiler then rebuilds the repo with it. It is not an issue for from-clean builds like most CI/PR/official builds, which I think is why it has been able to linger for so long.

[missymessa]

/cc @mmitche thoughts?

[mmitche]

I'm all for attempting to fix this. I'm not sure whether it can fit onto dnceng's plate right now. @rainersigwald Is this something your team could fix?

[rainersigwald]

I'm not opposed to it but we need to figure out how it'd fit in. cc @marcpopMSFT and @donJoseLuis.

[markwilkie]

Thoughts on next steps here? I think the "fitting it in" is still the challenge.

[mmitche]

@rainersigwald What are the risks here?

[missymessa]

@rainersigwald ping :)

[rainersigwald]

The risks are basically "continues to be a source of super subtle bugs like the one that bit Roslyn". I don't know if @donJoseLuis has team capacity to schedule this vs the other stuff we could be doing.

[missymessa]

@donJoseLuis if your team has capacity to work on this, please feel free to take it, otherwise we'll close it. Thanks!

[donJoseLuis]

Time is tight, as the end of release approaches and we are shorthanded. A teammate has 4 days of availability in AUG, we'll spend it looking at this. If the fix requires more than those 4 days, we will not be able to fund it.

[missymessa]

Time is tight, as the end of release approaches and we are shorthanded. A teammate has 4 days of availability in AUG, we'll spend it looking at this. If the fix requires more than those 4 days, we will not be able to fund it.

Sounds good! Thanks for checking back in :)