Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.NET 5.0 with C# 9 #123

Merged
merged 1 commit into from
Nov 15, 2020
Merged

.NET 5.0 with C# 9 #123

merged 1 commit into from
Nov 15, 2020

Conversation

NN---
Copy link
Member

@NN--- NN--- commented Nov 13, 2020

Merge after #124

@NN--- NN--- force-pushed the net50 branch 2 times, most recently from 1c1b030 to 8169c00 Compare November 13, 2020 14:19
<Project>
<PropertyGroup>
<!-- Compile Include="x.cs" doesn't use LangVersion defined in the PropertyGroup -->
<LangVersion>9.0</LangVersion>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to keep the setting either in csproj or in \Build\Props\CodeJam.Default.props

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that it doesn't help.
Files such as <Compile Include="JetBrains.Annotations.cs"/> don't receive LangVersion 9.0.
I submitted a bug.

It it possible to include a file from Build\Props... , but then there is no difference between setting it here or including.

Copy link
Member Author

Choose a reason for hiding this comment

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

I simply didn't find other working solution.

Copy link
Contributor

@ig-sinicyn ig-sinicyn Nov 13, 2020

Choose a reason for hiding this comment

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

Aha! Let's remove <LangVersion> from csproj files then

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -11,8 +11,8 @@
<PropertyGroup>
<CopyMeMinimalTargetFrameworks>net461;netstandard2.0</CopyMeMinimalTargetFrameworks>
<CopyMeMinimalTestTargetFrameworks>net461;netcoreapp2.0</CopyMeMinimalTestTargetFrameworks>
<CopyMeTargetFrameworks>net472;net461;net45;net40;net35;net20;netstandard2.1;netstandard2.0;netstandard1.5;netstandard1.3;netcoreapp3.0;netcoreapp2.0;netcoreapp1.0</CopyMeTargetFrameworks>
<CopyMeTestTargetFrameworks>net48;net472;net471;net47;net461;net45;net40;net35;net20;netcoreapp3.0;netcoreapp2.1;netcoreapp1.1</CopyMeTestTargetFrameworks>
<CopyMeTargetFrameworks>net50;net472;net461;net45;net40;net35;net20;netstandard2.1;netstandard2.0;netstandard1.5;netstandard1.3;netcoreapp3.0;netcoreapp2.0;netcoreapp1.0</CopyMeTargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to regenerate FW monikers in CodeJam.TargetingTests.EmitTargetingConstants

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will add there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually didn't know it is autogenerated :)
Why not T4 as done in other places ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's fix it after this PR.

@ig-sinicyn
Copy link
Contributor

@NN--- Thanks for the great work!)
I'm going to look more closely at the PR tomorrow.

@ig-sinicyn
Copy link
Contributor

As suggestion:

#pragma warning disable SYSLIB0012 // 'Assembly.CodeBase' is obsolete: 'Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility. Use Assembly.Location instead.

			var uri = new Uri(assembly.CodeBase);

#pragma warning restore SYSLIB0012

I think we should follow the recommendation and use CodeBase only when targeting to the net FW

@ig-sinicyn
Copy link
Contributor

#if LESSTHAN_NET50
		[ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
#endif

I'm not sure but this looks kinda strange for me.
The line was ok for all prev targets, what changed with FW 5.0 ?

@ig-sinicyn
Copy link
Contributor

As for me, everything else is ok.

One more time, a great thanks you! :)

@NN---
Copy link
Member Author

NN--- commented Nov 13, 2020

@ig-sinicyn Next part is to

#if LESSTHAN_NET50
		[ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
#endif

I'm not sure but this looks kinda strange for me.
The line was ok for all prev targets, what changed with FW 5.0 ?

.NET 5.0 added ObsoleteAttribute to this :)
Before .NET 5.0 it didn't have any meaning in .NET Core, but was not obsolete.
dotnet/runtime@329f0db#diff-997b403ad145cdd790196073af16aa4df8b1fc688415ce38b1c8a788e438dd2a

@ig-sinicyn
Copy link
Contributor

.NET 5.0 added ObsoleteAttribute to this :)

Wow, nice catch!

@NN---
Copy link
Member Author

NN--- commented Nov 13, 2020

@ig-sinicyn AppVeyor still don't have VS 16.8 , hence the preview version.
I think it is stable enough for CodeJam.
Once they update their image, we can switch back to release.

@NN---
Copy link
Member Author

NN--- commented Nov 13, 2020

Fixed comments. Good enough to commit ?:)
I started adding nullability, but it requires C# 9.0 in order to work correctly with generic methods.

@NN--- NN--- requested a review from ig-sinicyn November 13, 2020 16:36
@ig-sinicyn
Copy link
Contributor

Fixed comments. Good enough to commit ?:)

Yep, LGTM 👍

@NN---
Copy link
Member Author

NN--- commented Nov 13, 2020

Thanks.
Then waiting for appveyor.
It takes some time to compile :)

@NN---
Copy link
Member Author

NN--- commented Nov 13, 2020

Do you know why the build is so slow ?
42 min 8 sec

@ig-sinicyn
Copy link
Contributor

Have no idea

@NN---
Copy link
Member Author

NN--- commented Nov 13, 2020

Why the build failed ?

@NN--- NN--- force-pushed the net50 branch 2 times, most recently from 2163b00 to c99164d Compare November 13, 2020 20:19
@NN---
Copy link
Member Author

NN--- commented Nov 13, 2020

nunit3-console doesn't support net50 yet.

@NN---
Copy link
Member Author

NN--- commented Nov 14, 2020

@ig-sinicyn Thanks for approval.
I see we encountered the new .NET 5.0 breaking change with strings.:(
Need to investigate and fix before merging.

@NN---
Copy link
Member Author

NN--- commented Nov 14, 2020

Here is what fails.
Need to figure out how to make a better test.
dotnet/dotnet-docker#1877 (comment)

@NN---
Copy link
Member Author

NN--- commented Nov 14, 2020

Found the relevant documentation: https://docs.microsoft.com/en-us/dotnet/standard/base-types/string-comparison-net-5-plus

@NN---
Copy link
Member Author

NN--- commented Nov 14, 2020

@ig-sinicyn any idea how to test invariant culture ?
I used ss and ß characters but they are now not the same in 5.0

@ig-sinicyn
Copy link
Contributor

ig-sinicyn commented Nov 14, 2020

@ig-sinicyn any idea how to test invariant culture ?
I used ss and ß characters but they are now not the same in 5.0

Feel free to disable tests (with some comment, of course).
I'm going to scan all codebase for the
CA1307: Specify StringComparison for clarity
CA1309: Use ordinal StringComparison
after PR completion.

UP. #125

@NN---
Copy link
Member Author

NN--- commented Nov 14, 2020

I made test which compares ss with ß , validating invariant comparison is used.
But in .NET 5.0 they are not the same even with invariant comparison.
The question do we need to find some other invariant equal character or just remove this case ?:)

@ig-sinicyn
Copy link
Contributor

The question do we need to find some other invariant equal character or just remove this case ?:)

Just remove

@NN--- NN--- force-pushed the net50 branch 11 times, most recently from ea9570f to d81ee50 Compare November 15, 2020 09:44
@NN---
Copy link
Member Author

NN--- commented Nov 15, 2020

Found the problem. AppVeyor has .NET 5.0 preview which defined NETCOREAPP3_1 for .NET 5.0 :(
Added a workaround.

@NN--- NN--- merged commit 87c9a3b into master Nov 15, 2020
@NN--- NN--- deleted the net50 branch November 15, 2020 12:56
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.

2 participants