Fix TFM inconsistency in BuildHost

[jjonescz]

Should fix failures reported in dotnet/sdk#45353 (comment).

[ViktorHofer]

I'm surprised to see $(NetPrevious) in that list. Shouldn't roslyn's source-built product (built inside the VMR) only target NetCurrent? cc @MichaelSimons

[MichaelSimons]

deferring to @mthalman and @jaredpar who worked on this pattern.

[mthalman]

cc @jasonmalinowski

[jaredpar]

Shouldn't roslyn's source-built product (built inside the VMR) only target NetCurrent?

Every time we've tried this in the past we've ended up breaking some other part of the build (there is always an older runtime or dependency hanging around that forces us into $(NetPrevious)). This is why we keep both current + prev even when in full source build. If you all are confident this no longer holds we can remove the all the $(NetPrevious) from this <When> block ... but I will small stakes bet we will find some way this breaks us again 😉

Alternatively if we want to keep building current + previous I think the right fix is to change the project files so that they agree on TFM as they depend on each other:

Project	TFM Set
Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj	$(NetRoslynBuildHostNetCoreVersion);net472
Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj	$(NetRoslynSourceBuild);net472

Basically make the latter $(NetRoslynBuildHostNetCoreVersion)

[jjonescz]

Note that NetRoslynBuildHostNetCoreVersion is used as TargetFramework (no plural) here

roslyn/src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj

Line 101 in c8fba92

<TargetFramework>$(NetRoslynBuildHostNetCoreVersion)</TargetFramework>

so that will need to be updated somehow if NetRoslynBuildHostNetCoreVersion is changed to contain multiple values. (Perhaps .Split(';')[0] or something like that would work.)

[jaredpar]

. In full product source-build, NetCurrent should be enough as it automatically adapts to the TFM that is required in the given context

In programming should is a four letter word. ;)

Again, I feel like every time we try this we convince ourselves it's safe, find out that in practice it's not and we revert. I'm happy to take the bet again but it's more up to you all.

it would be great if the repo source-build leg would also target the TFM t

I'm hopeful that once we get VMR source build working that we can eliminate repo source build entirely. It seems to be of diminishing value given how quickly we get full VMR validation post merge.

[ViktorHofer]

I'm hopeful that once we get VMR source build working that we can eliminate repo source build entirely. It seems to be of diminishing value given how quickly we get full VMR validation post merge.

We were actually considering the opposite by adding another repo validation leg to mimic the product build (VMR) when not building from source. I'm not sure whether we would prefer to find breaks post-merge during insertion from roslyn -> sdk. I admit that the current (source-build) validation leg doesn't validate all important cases which is why I asked above whether it should target the newer TFM and run with the newer Arcade SDK and .NET SDK.

cc @mmitche @MichaelSimons

[jaredpar]

My mental model is that we have repo source build because the time between when a change merges and when we get full source build validation is very long. For a normal change the validation is probably ~3 days, and pretty regularly it can week(s). It's also hard to spot test a change because you really need an official build + darc flow to do a source build test.

None of that is true with VMR source build. The time between merging a change and getting full source build validation is hours. That is because we flow code not binaries. So the moment we merge the flow should happen to VMR. Also as a dev if I think a change is source build risky it's trivial to validate locally. Just apply my change to VMR, run the source build leg (can even put up a PR) and I will get validation.

At that point I would push back on the need for repo local validation of source build.

[ViktorHofer]

I forwarded your message to our internal communication channel. Thanks for providing your input. Now is the best time to share opinions as we haven't started the repo level validation epic yet. We will discuss this in length (presumably in our next sync meeting) and get back to you.

[jaredpar]

Happy to come to a sync and elaborate in detail about this.

[ViktorHofer]

Can we get at least a workaround merged in sooner than later to unblock dotnet/sdk#45353? Doesn't need to be the perfect solution. Just want to get builds unblocked.

[jjonescz]

The current state of the PR should work right? If yes, I just need a sign off to merge it :)

[ViktorHofer]

Looks good to me but I'm not a roslyn repo expert...

[jaredpar]

The current state of the PR should work right?

It works but I'm not sure it's the best. @jasonmalinowski this is really up in your layer. My intuition is that you need consistency in the TFM sets you have in the workspaces layer. At the moment they don't look consistent. If we take this change it's possible this will cause you diff headaches down the road.

[jasonmalinowski]

tl;dr: I think this PR is fine as it is, let's merge it.

Longer set of thoughts:

Minimally two things must be using NetRoslynBuildHostNetCoreVersion. This line in the BuildHost:

roslyn/src/Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj

Line 7 in 7c7927c

<TargetFrameworks>$(NetRoslynBuildHostNetCoreVersion);net472</TargetFrameworks>

and this line in the consuming library:

roslyn/src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj

Line 101 in 7c7927c

<TargetFramework>$(NetRoslynBuildHostNetCoreVersion)</TargetFramework>

And right now that's the currently the case. What the actual value is for purposes of source build doesn't really matter much, as long as it's not newer than the SDK where this has to get inserted.

The other place that can cause the missync is we have this version here:

roslyn/src/Workspaces/Core/MSBuild/Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj

Line 7 in 7c7927c

<TargetFrameworks>$(NetRoslynSourceBuild);net472</TargetFrameworks>

Which I guess is why this broke originally: we end up with a NetPrevious project with a reference to a NetCurrent since the BuildHost also gets consumed as a library via the project reference. That general mess was being cleaned up in #72257, which I plan to resurrect next month in some form. That'd move the main library to be netstandard and severs some other ProjectReferences so things aren't so coupled. At that point we shouldn't have problems going forward, other than just picking an appropriate value for NetRoslynBuildHostNetCoreVersion, where the logic would generally be:

1. For source build, probably go with NetPrevious because of Jared's point we insert into more than one SDK.
2. For our official build that produces our NuGets, pick whatever is the oldest TFM you can target that doesn't anger your infrastructure folks. And the reason for this is because our MSBuildWorkspace can be used in third-party apps, and it's not uncommon for folks to add them to whatever is the oldest in-support .NET. Or, consider the upgrade assistant case, where they want their app or analysis to be able to run on older runtimes precisely so users can analyze their apps before the upgrade them to something new. So maybe we don't need .NET 5 anymore, but .NET 6 would still be good lest we block people trying to upgrade!

[mmitche]

re; NetPrevious vs. NetCurrent. In .NET 9 NetPrevious was empty (due to issues in restore when NetCurrent== NetPrevious).

    <!-- The TFM of the major release of .NET that the Arcade SDK aligns with. -->
    <NetCurrent>net9.0</NetCurrent>

    <!-- The previously released version of .NET.
         Undefined when NetMinimum and NetPrevious are identical. -->
    <NetPrevious />

    <!-- Lowest supported version of .NET at the time of the release of NetCurrent.
         E.g. net8.0 when NetCurrent is net9.0. -->
    <NetMinimum>net8.0</NetMinimum>

https://github.com/dotnet/arcade/blob/b1e481cac6e02ff965206de919cd46837181e5a6/src/Microsoft.DotNet.Arcade.Sdk/tools/TargetFrameworkDefaults.props#L12-L21.

I think it is acceptable to use NetPrevious here, since this is not inserting into a 9.0.2xx VMR where we would need to source build, as long as targeting NetPrevious does not require bringing in the downlevel runtime packs. If that is the case, then roslyn (and whoever else is forcing roslyn to NetPrevious) must use net10/NetCurrent.

[ViktorHofer]

tl;dr: I think this PR is fine as it is, let's merge it.

Let's merge it to unblock the arcade build.