Skip to content

Fix TFM inconsistency in BuildHost #76354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 13, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eng/targets/TargetFrameworks.props
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
<NetRoslyn>$(NetCurrent)</NetRoslyn>
<NetRoslynSourceBuild>$(NetCurrent);$(NetPrevious)</NetRoslynSourceBuild>
<NetRoslynAll>$(NetCurrent);$(NetPrevious)</NetRoslynAll>
<NetRoslynBuildHostNetCoreVersion>$(NetCurrent)</NetRoslynBuildHostNetCoreVersion>
<NetRoslynBuildHostNetCoreVersion>$(NetPrevious)</NetRoslynBuildHostNetCoreVersion>
Copy link
Member

@ViktorHofer ViktorHofer Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@jjonescz jjonescz Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<NetRoslynNext>$(NetCurrent)</NetRoslynNext>
</PropertyGroup>
</When>
Expand Down