Fix language check for netcoreapp3.1 #38967

agocke · 2019-09-30T22:29:23Z

Now checks if the version is < netcoreapp3.0 or netstandard2.1 instead of checking
if they're not equal.

…andard2.1

RikkiGibson · 2019-09-30T22:41:21Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

@@ -3,8 +3,8 @@
 <Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="Microsoft.Managed.Core.targets"/>

-  <PropertyGroup Condition="('$(TargetFrameworkIdentifier)' != '.NETCoreApp' OR '$(TargetFrameworkVersion)' != 'v3.0') AND
-                            ('$(TargetFrameworkIdentifier)' != '.NETStandard' OR '$(TargetFrameworkVersion)' != 'v2.1')">
+  <PropertyGroup Condition="('$(TargetFrameworkIdentifier)' != '.NETCoreApp' OR '$(_TargetFrameworkVersionWithoutV)' < '3.0') AND


Is this doing a string comparison?

You mean the < operator? I'm reasonably sure this has been updated in MSBuild to do a real version comparison, but I'd like @nguerrera to confirm that.

Adding @rainersigwald

The answer here is a little convoluted. MSBuild can coerce to version here, but might coerce to float first. We actually have some latent bugs if there's ever a .10+ minor release of a TFM. :(

I would like there to be an intrinsic function that can operate directly on $(TargetFrameworkVersion) (v and all) and compare as we expect in all cases. This is tracked by dotnet/msbuild#3212

Nick is correct; float is a higher precedence than Version. And you can't just add .0 to force to a version, because according to System.Version, 3.0 < 3.0.0 is true. MSBuild's version comparison just wraps that so has the same funky behavior.

Fortunately, here you're breaking on an integer boundary so this implementation is fine (it's irrelevant for these purposes that 2.10 < 2.9 because they're both <3.0).

The right fix is that to-be-written property function but you wouldn't be able to use it for a long time due to the compiler package working on older MSBuild engines.

I think I’ll take a stab at writing that function. Get the clock started at least. Moving forward I’d prefer to say “when you can depend on msbuild X, do this”

agocke · 2019-09-30T22:56:16Z

By the way I haven't actually managed to get a copy of 3.1 installed yet, but I'm checking that manually.

nguerrera · 2019-10-01T01:35:14Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

@@ -3,8 +3,8 @@
 <Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="Microsoft.Managed.Core.targets"/>

-  <PropertyGroup Condition="('$(TargetFrameworkIdentifier)' != '.NETCoreApp' OR '$(TargetFrameworkVersion)' != 'v3.0') AND
-                            ('$(TargetFrameworkIdentifier)' != '.NETStandard' OR '$(TargetFrameworkVersion)' != 'v2.1')">
+  <PropertyGroup Condition="('$(TargetFrameworkIdentifier)' != '.NETCoreApp' OR '$(_TargetFrameworkVersionWithoutV)' &lt; '3.0') AND


This targets file is used in all projects (classic and SDK) so I'm a little nervous about _TargetFrameworkVersionWithoutV being referenced here as it is defined by SDK targets.

(Aside: I'm NOT concerned about the _ ("private") as others are using it and I've basically said elsewhere that it can be considered public at this point. Hoping to get rid of it being heavily used with dotnet/msbuild#3212)

I think msbuild short-circuiting would be good enough as we only check this variable when TFI is NETCoreApp or NETStandard, which are only supported for SDK projects, but there were some weird cases of other IDEs design-time evaluating in an "ignore conditions mode" that caused unexpected expressions to be evaluated and complain about numeric comparison to non-number.

I'll defer to @rainersigwald on this.

My preference is we only depend on publicly supported values. If that means we have to add another AND for every version of .NET Core app for the foreseeable future than so be it. The total amount of code that will take is less than the comment section of this PR.

The problems with short-circuiting were only when an outer scope was conditioned but the inner scope isn't; that's not the case here--the short-circuiting is within a single expression.

I manually applied this patch to my local VS and was able to load a project just fine using OmniSharp, which was the major source of problems there. I think it's good.

@nguerrera Can you elaborate a little on the problem with _TargetFrameworkVersionWithoutV? This is in the CSharp targets so I assumed the SDK would always be available. Is that a bad assumption?

By SDK here (overloaded term), I mean Microsoft.NET.Sdk and its targets. In a classic csproj (no Sdk="Microsoft.NET.Sdk"), this file would be imported but the Microsoft.NET.Sdk wouldn't. And Microsoft.NET.Sdk defines _TargetFrameworkVersionWithoutV. But we are only checking _TargetFrameworkWithoutV when target framework .NETCoreApp or .NETStandard, which would require Microsoft.NET.Sdk.

This change broke our repo with VS 2019 16.4.0 preview 2.0. Our repo uses classic/non-sdk csproj to target netstandard. I worked around by defining _TargetFrameworkVersionWithoutV in a props file.

@timmydo Is there a reason why you're targeting .NET Standard using non-SDK projects?

Long story, but we migrated from corext to stock msbuild and I think this was the recommended approach. Maybe we aren't completely there yet, but when you have thousands of csproj files, it's not like an on-off switch you can flip to move to the latest thing. There are probably a lot of others in the same boat.

Interesting that it would be a recommended approach. IIRC we don't support projects in that state, though I could be wrong.

nguerrera

I'd like to resolve the discussions above with input from @rainersigwald before we take this.

rainersigwald · 2019-10-01T14:47:09Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

@@ -3,8 +3,8 @@
 <Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="Microsoft.Managed.Core.targets"/>

-  <PropertyGroup Condition="('$(TargetFrameworkIdentifier)' != '.NETCoreApp' OR '$(TargetFrameworkVersion)' != 'v3.0') AND
-                            ('$(TargetFrameworkIdentifier)' != '.NETStandard' OR '$(TargetFrameworkVersion)' != 'v2.1')">
+  <PropertyGroup Condition="('$(TargetFrameworkIdentifier)' != '.NETCoreApp' OR '$(_TargetFrameworkVersionWithoutV)' &lt; '3.0') AND


The problems with short-circuiting were only when an outer scope was conditioned but the inner scope isn't; that's not the case here--the short-circuiting is within a single expression.

I manually applied this patch to my local VS and was able to load a project just fine using OmniSharp, which was the major source of problems there. I think it's good.

agocke · 2019-10-01T19:50:46Z

@nguerrera Is this good to go?

Downgrade language version to 7.3 for things < netcoreapp3.0 or netst…

6f1f2cc

…andard2.1

agocke requested a review from a team as a code owner September 30, 2019 22:29

agocke added the Area-Compilers label Sep 30, 2019

agocke added this to the 16.4 milestone Sep 30, 2019

agocke requested review from nguerrera and jaredpar September 30, 2019 22:29

cston approved these changes Sep 30, 2019

View reviewed changes

RikkiGibson reviewed Sep 30, 2019

View reviewed changes

Escape '<' character

beb17fc

333fred approved these changes Oct 1, 2019

View reviewed changes

nguerrera reviewed Oct 1, 2019

View reviewed changes

nguerrera suggested changes Oct 1, 2019

View reviewed changes

rainersigwald approved these changes Oct 1, 2019

View reviewed changes

nguerrera approved these changes Oct 1, 2019

View reviewed changes

agocke merged commit d28899e into dotnet:master Oct 1, 2019

agocke deleted the use-csharp8-netcore3.1-default branch October 1, 2019 19:57

ajaybhargavb mentioned this pull request Oct 8, 2019

Pass through the correct LangVersion to csc dotnet/razor#1220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix language check for netcoreapp3.1 #38967

Fix language check for netcoreapp3.1 #38967

agocke commented Sep 30, 2019

RikkiGibson Sep 30, 2019

agocke Sep 30, 2019

nguerrera Oct 1, 2019 •

edited

Loading

rainersigwald Oct 1, 2019

nguerrera Oct 1, 2019

agocke commented Sep 30, 2019

nguerrera Oct 1, 2019

jaredpar Oct 1, 2019

rainersigwald Oct 1, 2019

agocke Oct 1, 2019

nguerrera Oct 1, 2019

timmydo Oct 18, 2019

cartermp Oct 18, 2019

timmydo Oct 18, 2019

cartermp Oct 18, 2019

nguerrera left a comment

rainersigwald Oct 1, 2019

agocke commented Oct 1, 2019

Fix language check for netcoreapp3.1 #38967

Fix language check for netcoreapp3.1 #38967

Conversation

agocke commented Sep 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented Sep 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented Oct 1, 2019

nguerrera Oct 1, 2019 •

edited

Loading