Skip to content
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

Fix detection of torn SDK #41907

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Fix detection of torn SDK #41907

merged 2 commits into from
Jul 15, 2024

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Jul 2, 2024

If I understand correctly, the SDK is torn in case we have an old msbuild and new SDK. So e.g., if $(_MSBuildVersionMajorMinor) is 17.10 and $(_BundledMSBuildVersionMajorMinor) is 17.11 then $(_IsDisjointMSBuildVersion) should be true.

Part of #41791.

@jjonescz jjonescz requested a review from baronfel July 2, 2024 13:19
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Jul 2, 2024
@marcpopMSFT
Copy link
Member

@jaredpar shouldn't we set this whenever the two values are different? I know the SDK being newer is the majority of the issues but I believe you can still have issues the other way and wouldn't it be easier to do it whenever they were different?

@jjonescz
Copy link
Member Author

jjonescz commented Jul 3, 2024

shouldn't we set this whenever the two values are different?

The issue is only with old msbuild + new SDK. Then we get an error like

CSC : warning CS9057: The analyzer assembly '..\dotnet\sdk\8.0.200\Sdks\Microsoft.NET.Sdk.Razor\source- generators\Microsoft.CodeAnalysis.Razor.Compiler.SourceGenerators.dll' references version '4.9.0.0' of the compiler, which is newer than the currently running version '4.8.0.0'.

We don't get that the other way around - with new msbuild and old SDK - that should work fine (the new compiler should be able to load the old generators/analyzers, it's backwards-compatible). It also seems better to use the latest compiler with hopefully fewer bugs than the old one.

@jjonescz jjonescz requested a review from jaredpar July 6, 2024 13:50
@jaredpar
Copy link
Member

jaredpar commented Jul 8, 2024

It also seems better to use the latest compiler with hopefully fewer bugs than the old one.

True but part of the rationale for this proposal is to ensure that msbuild and dotnet build have a consistent experience. Including bug fixes, even well meaning ones, would mean that we fall right back into them having differing experiences. That is why I prefer saying a torn state is when the versions differ (up or down).

@jjonescz
Copy link
Member Author

jjonescz commented Jul 8, 2024

This should probably be merged after #41951, because it looks like the current Toolset.Framework installation is broken even in simple cases. I created a new blazor app and trying to build it with msbuild 17.10 and SDK 9p5 (and this disjoint detection fixed), I'm getting the Toolset.Framework PackageReference without a Version specified - so it gets installed as 4.6.0 (the lowest available version) causing even more trouble (more errors about analyzers referencing newer compiler) than without it.

@jjonescz
Copy link
Member Author

@jaredpar I need a signoff here so I can merge it after #41951

@jjonescz jjonescz merged commit 1eed0b8 into dotnet:main Jul 15, 2024
37 checks passed
@jjonescz jjonescz deleted the tearing-detection branch July 15, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants