Use Roslyn from SDK always for SDK-based project builds #48028
Conversation
ghost
added
baronfel
left a comment
baronfel
left a comment
There was a problem hiding this comment.
Awesome to see this starting!
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Common.targets
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets
Outdated
Show resolved
Hide resolved
|
Do we know all of the other tools/products that use Csc to compile things internally? (WPF, Azure Functions, etc) - I'd expect we'd want to make sure that those can conditionally pass the new property if present so make sure that users don't flip between different kinds of compilers in the same logical build. |
|
We know some but don't have a comprehensive list we can consult. Probably should start keeping one as we work through this change during this release |
|
@RikkiGibson, @chsienki PTAL |
|
Note that I first need to merge the Roslyn side but waiting for roslyn infra to work again before I do that. |
eng/Versions.props
Outdated
| <MicrosoftCodeAnalysisWorkspacesCommonPackageVersion>5.0.0-1.25210.7</MicrosoftCodeAnalysisWorkspacesCommonPackageVersion> | ||
| <MicrosoftCodeAnalysisWorkspacesMSBuildPackageVersion>5.0.0-1.25210.7</MicrosoftCodeAnalysisWorkspacesMSBuildPackageVersion> | ||
| <MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion>5.0.0-1.25210.7</MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion> | ||
| <MicrosoftNetCompilersToolsetVersion>4.14.0-3.25210.1</MicrosoftNetCompilersToolsetVersion> |
There was a problem hiding this comment.
Why are we downgrading the compiler here?
There was a problem hiding this comment.
This PR needs a corresponding change in Roslyn to flow in first. I built it manually and used it here just temporarily (ie, this will be reverted before merging).
| Use Roslyn deployed with SDK for builds of SDK-style projects (regardless of whether the initiator is `dotnet` or `msbuild`). | ||
| See https://github.com/dotnet/sdk/blob/main/documentation/general/decouple-vs-and-net-sdk.md. | ||
| --> | ||
| <Choose> |
There was a problem hiding this comment.
Consider just making these conditions on the property directly rather than using choose, so you don't have to duplicate the property groups.
There was a problem hiding this comment.
That would mean needing to duplicate the conditions though and I think that would be less readable.
| // UseSharedCompilation should be the default, so set it only if false. | ||
| if (!useSharedCompilation) | ||
| { | ||
| project.AdditionalProperties["UseSharedCompilation"] = "false"; | ||
| } |
There was a problem hiding this comment.
Might be simpler to just always be explicit here:
| // UseSharedCompilation should be the default, so set it only if false. | |
| if (!useSharedCompilation) | |
| { | |
| project.AdditionalProperties["UseSharedCompilation"] = "false"; | |
| } | |
| project.AdditionalProperties["UseSharedCompilation"] = useSharedCompilation ? "true" : "false"; | |
There was a problem hiding this comment.
Which you can probably fold into the initializer above actually
There was a problem hiding this comment.
My intention is to not set UseSharedCompilation to true, so the tests also verify that true is the default value
6 participants
Uh oh!
There was an error while loading. Please reload this page.