Skip to content

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented May 7, 2025

This PR is adding compiler toolset package tests (which caught a real bug 🐛, see #48837 (comment)) and updating docs.

@jjonescz jjonescz requested a review from jaredpar May 7, 2025 07:38
@jjonescz jjonescz marked this pull request as ready for review May 12, 2025 13:03
@jjonescz
Copy link
Member Author

@jaredpar for a review, thanks

@jjonescz jjonescz requested a review from jaredpar June 16, 2025 09:10
@jjonescz jjonescz changed the title Prefer compiler toolset package if installed over the default RoslynCompilerType=Core logic Add roslyn compiler toolset tests Jun 19, 2025
@jjonescz
Copy link
Member Author

jjonescz commented Jun 19, 2025

@jaredpar for another look (this is a test+doc only PR now), thanks


## RoslynCompilerType

Based on the value of the `RoslynCompilerType` property, the SDK sets property `RoslynTasksAssembly` to a full path to a [Roslyn build task DLL][roslyn-build-task],
Copy link
Member

Choose a reason for hiding this comment

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

Lets mention RoslynTargetsPath as well here as getting a compiler properly setup for msbuild seems to require both of these.

Copy link
Member Author

@jjonescz jjonescz Jun 20, 2025

Choose a reason for hiding this comment

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

Do you think RoslynTargetsPath will be still required for msbuild even with this: dotnet/msbuild#12045 ? (Assuming that PR works which it currently does not :D)

Anyway, I'm not sure how to define RoslynTargetsPath. Is it also "guaranteed" to be set by the SDK just like RoslynTasksAssembly? But the toolset package doesn't set RoslynTargetsPath, it sets only RoslynTasksAssembly, isn't that wrong?

Copy link
Member

Choose a reason for hiding this comment

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

I think that it should be guaranteed to be set by SDK projects. That is because it's guaranteed to be set by non-SDK projects, due to msbuid app config, and hence we need to make that it's properly set in all cases. It's been around so long it's seemingly guaranteed that others are depending on it.

@jjonescz jjonescz merged commit a1f249c into dotnet:main Jun 23, 2025
30 checks passed
@jjonescz jjonescz deleted the core-toolset branch June 23, 2025 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants