Skip to content

Conversation

tmds
Copy link
Member

@tmds tmds commented Oct 6, 2025

This version provides a fix for crashing on Mono (dotnet/msbuild#12570).

@tmds tmds requested a review from a team as a code owner October 6, 2025 12:11
@tmds
Copy link
Member Author

tmds commented Oct 6, 2025

@ViktorHofer
Copy link
Member

ViktorHofer commented Oct 6, 2025

Two updates are necessary:

@tmds
Copy link
Member Author

tmds commented Oct 6, 2025

I've made the requested changes.

@tmds
Copy link
Member Author

tmds commented Oct 6, 2025

CI isn't passing because both this PR and dotnet/dotnet#2675 are needed together.

@tmds
Copy link
Member Author

tmds commented Oct 6, 2025

Ah, nvmd, I need to move the patch here.

This version provides a fix for crashing on Mono (dotnet/msbuild#12570).
@ViktorHofer
Copy link
Member

Looks like another patch also needs an update: 0003-Remove-MicroBuild-signing.patch

@rainersigwald
Copy link
Member

@YuliiaKovalova before the next release we should change MSBuildLocator to make these patches unnecessary (by checking for sourcebuild conditions).

@tmds
Copy link
Member Author

tmds commented Oct 6, 2025

@ViktorHofer build failure is:

  /__w/1/s/src/externalPackages/src/MSBuildLocator/artifacts/clone/src/MSBuildLocator/Microsoft.Build.Locator.csproj : error NU1102: Unable to find package Microsoft.NETCore.App.Ref with version (= 8.0.20)
  /__w/1/s/src/externalPackages/src/MSBuildLocator/artifacts/clone/src/MSBuildLocator/Microsoft.Build.Locator.csproj : error NU1102:   - Found 4 version(s) in SBRP [ Nearest version: 9.0.0 ]

I'm going to see what happens when I target net10.0 instead of 8.0.

@ViktorHofer
Copy link
Member

@tmds as far as I'm aware, MSBuildLocator needs to stay on net8.0. @MichaelSimons @mthalman any idea why that submodule needs the 8.0.20 runtime version? Is the submodule building with the same SDK as the other submodules?

@mthalman
Copy link
Member

mthalman commented Oct 7, 2025

as far as I'm aware, MSBuildLocator needs to stay on net8.0

I'm not aware of any requirement for that. That may have been a thing in the past when we had repo-level prebuilt detection but that's not around anymore.

any idea why that submodule needs the 8.0.20 runtime version

That's because that's the latest runtime version associated with the net8.0 TFM so that's what it targets by default. I don't know what particular thing causes that package name to be restored though.

Is the submodule building with the same SDK as the other submodules?

Yes.

@ViktorHofer
Copy link
Member

ViktorHofer commented Oct 7, 2025

I just checked and you are right, all projects that consume this in the source-build graph target the latest TFM. That said, a goal is to keep the differences between the SB builds and MSFT or external packages as small as possible. Having a net10.0 TFM in SB but net8.0 from the upstream package could result in differences. I let you folks decide whether that's a reason to make net8.0 work or not.

@mthalman
Copy link
Member

mthalman commented Oct 7, 2025

net8.0 can be made to work by following the same pattern that existed in the patch that was removed and have it target 8.0.0 instead of 7.0.0: https://github.com/dotnet/source-build-reference-packages/pull/1399/files#diff-55a2fe703adfc36576f195f486767c265a42560c03d2bd75c1002044810baeb9L28

@rainersigwald
Copy link
Member

I don't have strong opinions on the SBRP side, but MSBuildLocator should certainly work fine compiled against net10, so if that smooths things over no objection from me.

@tmds
Copy link
Member Author

tmds commented Oct 8, 2025

The version that gets compiled with source-only .NET 10 will run on the .NET 10 runtime, so except if MSBuildLocator were doing something really specific to .NET 10 (like #if NET10), it will work the same whether it targets net8.0 or net10.0. Based on @rainersigwald comment, I think we can safely use net10.0.

@ViktorHofer ViktorHofer merged commit 59182f9 into dotnet:release/10.0 Oct 8, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants