Multi-target C# compiler by jaredpar · Pull Request #41370 · dotnet/roslyn

jaredpar · 2020-02-03T18:54:05Z

This changes the C# compiler so that it multi-targets between
netstandard2.0 and netcoreapp3.1.

This changes the C# compiler so that it multi-targets between `netstandard2.0` and `netcoreapp3.1`.

333fred

Preventing merge until signed build is completed.

jaredpar · 2020-02-04T07:04:51Z

Signed build validation is here https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3439702&view=results

jaredpar · 2020-02-04T07:05:11Z

@dotnet/roslyn-compiler PTAL

Dismissing as signed build validation for this change has completed here https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3439702&view=results

jasonmalinowski · 2020-02-04T19:50:42Z

    -->
    <UserRuntimeConfig Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' AND '$(OutputType)' == 'Exe'">$(RepositoryEngineeringDir)config\runtimeconfig.template.json</UserRuntimeConfig>
+
+    <DisableNullableWarnings Condition="'$(DisableNullableWarnings)' == '' AND '$(TargetFramework)' != '' AND '$(TargetFramework)' == 'netcoreapp3.1'">true</DisableNullableWarnings>


Why is the second clause in the AND necessary? It would seem if the third clause (it's netcoreapp3.1) is true, the second is implied.

My question here is of course rooted in a deep mistrust of MSBuild and TFMs, so it wouldn't surprise me in the slightest if you tell me about something evil going on. But maybe that needs a comment then.

Why is the second clause in the AND necessary? It would seem if the third clause (it's netcoreapp3.1) is true, the second is implied.

In a multi-targetted build this file gets evaluated three times: once in the outer build where only $(TargetFrameworks) is defined and twice for each $(TargetFramework) value. Need the middle clause so we don't set the property on the outer evaluation.

Welcome to one hour of my life that I lost here ...

'$(TargetFramework)' != '' AND '$(TargetFramework)' == 'netcoreapp3.1'

This is an A && B scenario, where B implies A. It's possible the condition isn't written the expected way, because as-is the '$(TargetFramework)' != '' is unnecessary.

Blarg I checked in a hack I was validiting locally with. Just a sec.

The new warning check is the correct (albeit strange at a glance) one

jasonmalinowski · 2020-02-04T19:51:13Z

+  </PropertyGroup>
+
+  <PropertyGroup Condition="'$(DisableNullableWarnings)' == 'true'">
+    <NoWarn>$(NoWarn);8600;8601;8602;8603;8604;8605;8610;8616;8617;8714</NoWarn>


If/when the compiler adds another one, will we remember to update this?

We'll find out the moment we get a warning in a netstandard2.0 build.

I don't know of a better way to manage this. Defining this property has made me re-think how we use our overall <Nullable> property though. Outside that though I think this is the best approach.

The alternative, which is perhaps better, is what winforms is using. Define Nullable=enable for the project and add #nullable disable to all files. Then remove that from each file to enable it. With that approach, we could set Nullable=annotations for the netstandard2.0 build, Nullable=enable for the netcoreapp3.1 build.

I'd rather maintain a simple warning set than modify every file and project in our repo

That or change the compiler to hold a property in our targets files that listed every nullable warning.

Either way seems like this is a feature that would be generally valuable in compilation.

@jcouv reminded me that this could be simplified to use the Nullable warning item and remove the individual code listing. Updated the PR

jasonmalinowski · 2020-02-04T19:52:06Z

    </PackageDescription>
+
+    <!-- Disable the nullable warnings when compiling for netcoreapp3.1 during our transition period -->
+    <DisableNullableWarnings Condition="'$(TargetFramework)' == 'netcoreapp3.1'" />


Isn't this going to fight with your other property in the Imports.targets?

It's meant to fight with that. Specifically for the moment we want the inverse of the default: warn on netstandard2.0 not netcoreapp3.1. That's becuase there are a host of new warnings to work through. Once we fix all those we'll delete this property.

This property set is off though, need to explicitly set to true.

jasonmalinowski · 2020-02-04T19:52:50Z

+      <CoreClrCompilerBinArtifact Include="$(ArtifactsBinDir)Microsoft.CodeAnalysis.CSharp\$(Configuration)\netcoreapp3.1\Microsoft.CodeAnalysis.CSharp.dll" />
+      <CoreClrCompilerBinArtifact Include="$(ArtifactsBinDir)Microsoft.CodeAnalysis.CSharp\$(Configuration)\netcoreapp3.1\**\Microsoft.CodeAnalysis.CSharp.resources.dll" />

      <CoreClrCompilerBinArtifact Include="$(ArtifactsBinDir)Microsoft.CodeAnalysis.VisualBasic\$(Configuration)\netstandard2.0\Microsoft.CodeAnalysis.VisualBasic.dll" />


Not moving VB along as well?

VB is blocked until this issue is fixed dotnet/sdk#10591

…proj

nguerrera · 2020-02-04T21:01:19Z

@nguerrera

jaredpar · 2020-02-05T17:32:12Z

@dotnet/roslyn-compiler can I get a review on this? It's mostly an infra change but I did have to change part of the compiler here to account for types like Index being available in our netcoreapp3.1 targets.

333fred · 2020-02-06T00:37:55Z

    -->
    <UserRuntimeConfig Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' AND '$(OutputType)' == 'Exe'">$(RepositoryEngineeringDir)config\runtimeconfig.template.json</UserRuntimeConfig>
+
+    <DisableNullableWarnings Condition="'$(DisableNullableWarnings)' == '' AND '$(TargetFramework)' != '' AND '$(TargetFramework)' != 'netcoreapp3.1'">true</DisableNullableWarnings>


Given how many people that confused, perhaps this is worth a comment.

333fred · 2020-02-06T00:38:22Z

    <!-- Bypass a check in msbuild that .NET Framework 3.5 is installed on the machine when targeting .NET < 4.0 -->
    <BypassFrameworkInstallChecks>true</BypassFrameworkInstallChecks>
-  </PropertyGroup>
+ </PropertyGroup>


Unrelated whitespace change?

333fred

LGTM (commit 6) with a couple small suggestions.

jaredpar · 2020-02-06T23:48:57Z

The roslyn integration tests are failing every where right now. I did run the MSBuild integration tests locally on my change and they passed. There was one failure but it was due to me not having the 4.5 targets installed. Going to go ahead and merge.

Multi-target C# compiler

b17fbed

This changes the C# compiler so that it multi-targets between `netstandard2.0` and `netcoreapp3.1`.

jaredpar added the Area-Compilers label Feb 3, 2020

333fred previously requested changes Feb 3, 2020

View reviewed changes

jaredpar added 2 commits February 3, 2020 11:14

Use correct target framework

c61be78

Fix target framework

1f144e1

jaredpar marked this pull request as ready for review February 4, 2020 07:04

jaredpar requested review from a team as code owners February 4, 2020 07:04

jasonmalinowski reviewed Feb 4, 2020

View reviewed changes

jaredpar commented Feb 4, 2020

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.csproj Outdated

Update src/Compilers/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.cs…

21f0c4d

…proj

jaredpar added 2 commits February 4, 2020 13:33

Fix the nullable warning condition

b32e8c9

Simplify the NoWarn clause

4b0cd67

cston approved these changes Feb 5, 2020

View reviewed changes

333fred reviewed Feb 6, 2020

View reviewed changes

333fred approved these changes Feb 6, 2020

View reviewed changes

jcouv added this to the 16.6 milestone Feb 6, 2020

nguerrera mentioned this pull request Feb 6, 2020

[release/3.1.2xx] Update dependencies from dotnet/roslyn dotnet/toolset#4223

Merged

PR feedback

e3b4a97

jaredpar merged commit aaed113 into dotnet:master Feb 6, 2020

jaredpar deleted the mt branch February 6, 2020 23:49

Conversation

jaredpar commented Feb 3, 2020

Uh oh!

333fred left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Feb 4, 2020

Uh oh!

jaredpar commented Feb 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nguerrera commented Feb 4, 2020

Uh oh!

jaredpar commented Feb 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Feb 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

333fred left a comment •

edited

Loading