Skip to content

[C#] Update .NET SDKs to LTS versions #9205

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

Merged

Conversation

JamesNK
Copy link
Contributor

@JamesNK JamesNK commented Nov 9, 2021

Updates SDKs:

  • 2.1 -> 3.1
  • 5.0 -> 6.0

Also fixes inconsistency in parsing out-of-range double values in JSON format.

@google-cla google-cla bot added the cla: yes label Nov 9, 2021
@JamesNK JamesNK changed the title Update .NET SDKs to LTS versions [C#] Update .NET SDKs to LTS versions Nov 9, 2021
@JamesNK
Copy link
Contributor Author

JamesNK commented Nov 9, 2021

@jtattermusch

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM once the tests are green.

@JamesNK
Copy link
Contributor Author

JamesNK commented Nov 11, 2021

It failed because of a conformance test:

ERROR, test=Required.Proto3.JsonInput.DoubleFieldTooSmall: Should have failed to parse, but didn't. request=json_payload: "{"optionalDouble": -1.89769e+308}" requested_output_format: JSON message_type: "protobuf_test_messages.proto3.TestAllTypesProto3" test_category: JSON_TEST, response=json_payload: "{ "optionalDouble": "-Infinity" }"

These tests failed. If they can't be fixed right now, you can add them to the failure list so the overall suite can succeed. Add them to the failure list by running:
./update_failure_list.py failure_list_csharp.txt --add failing_tests.txt

Required.Proto3.JsonInput.DoubleFieldTooSmall

The Double.Parse method from .NET is used to parse doubles from JSON. With .NET Core 3.1 and greater, the small value which is expected to fail is now supported.

Ignore the error? Or add additional logic to Protobuf's double parsing to explicitly fail this small value?

@jtattermusch
Copy link
Contributor

Looks like we are seeing the failure to do dotnet restore on windows again:
https://source.cloud.google.com/results/invocations/1f3042e9-d6f9-4e45-a237-4f1fa4cc9979/log

e.g.

T:\src\github\protobuf\csharp\src\Google.Protobuf.Benchmarks\Google.Protobuf.Benchmarks.csproj : error NU1101: Unable to find package BenchmarkDotNet. No packages exist with this id in source(s): Microsoft Visual Studio Offline Packages [T:\src\github\protobuf\csharp\src\Google.Protobuf.sln]

T:\src\github\protobuf\csharp\src\Google.Protobuf.Benchmarks\Google.Protobuf.Benchmarks.csproj : error NU1101: Unable to find package Microsoft.NETCore.App.Ref. No packages exist with this id in source(s): Microsoft Visual Studio Offline Packages [T:\src\github\protobuf\csharp\src\Google.Protobuf.sln]

T:\src\github\protobuf\csharp\src\Google.Protobuf\Google.Protobuf.csproj : error NU1102: Unable to find package NETStandard.Library with version (>= 1.6.1) [T:\src\github\protobuf\csharp\src\Google.Protobuf.sln]

T:\src\github\protobuf\csharp\src\Google.Protobuf\Google.Protobuf.csproj : error NU1102:   - Found 1 version(s) in Microsoft Visual Studio Offline Packages [ Nearest version: 1.6.0 ] [T:\src\github\protobuf\csharp\src\Google.Protobuf.sln]

T:\src\github\protobuf\csharp\src\Google.Protobuf\Google.Protobuf.csproj : error NU1101: Unable to find package Microsoft.SourceLink.GitHub. No packages exist with this id in source(s): Microsoft Visual Studio Offline Packages [T:\src\github\protobuf\csharp\src\Google.Protobuf.sln]

T:\src\github\protobuf\csharp\src\Google.Protobuf\Google.Protobuf.csproj : error NU1101: Unable to find package Microsoft.NETFramework.ReferenceAssemblies. No packages exist with this id in source(s): Microsoft Visual Studio Offline Packages [T:\src\github\protobuf\csharp\src\Google.Protobuf.sln]

I'm wondering why it's only complaining about the nuget source "Microsoft Visual Studio Offline Packages". Are the nuget sources somehow misconfigured so that download from nuget.org is never attempted?

@jtattermusch
Copy link
Contributor

-1.89769e+308

This is odd. How can Double.Parse suddenly succeed with too small input value in .NET Core 3+?

Double.MinValue is -1.7976931348623157E+308 (see https://docs.microsoft.com/en-us/dotnet/api/system.double.minvalue?view=net-5.0), so being able to parse the tested value -1.89769e+308 (which is clearly much smaller) seems like a bug. We should add a C# unit test for this and make sure we understand what's going on.

@elharo
Copy link
Contributor

elharo commented Nov 11, 2021

Per docs OverflowException is thrown by Double.parse in "NET Framework and .NET Core 2.2 and earlier versions only" so something has indeed changed. I'm not sure what.

@elharo
Copy link
Contributor

elharo commented Nov 11, 2021

OK, looks like it now returns +/- Inf in these cases.

@elharo
Copy link
Contributor

elharo commented Nov 11, 2021

"In .NET Core 3.0 and later, values that are too large to represent are rounded to PositiveInfinity or NegativeInfinity as required by the IEEE 754 specification. In prior versions, including .NET Framework, parsing a value that was too large to represent resulted in failure."

@elharo
Copy link
Contributor

elharo commented Nov 11, 2021

This is going to be tricky. We might not want to simply accept the infinity values. We might instead want to check for them explicitly and then fail like we used to in 2.1 and earlier.

@JamesNK
Copy link
Contributor Author

JamesNK commented Nov 11, 2021

The simple solution could be to throw if Double.Parse returns infinity. That matches existing behavior.

@JamesNK
Copy link
Contributor Author

JamesNK commented Nov 11, 2021

@jtattermusch

I'm wondering why it's only complaining about the nuget source "Microsoft Visual Studio Offline Packages". Are the nuget sources somehow misconfigured so that download from nuget.org is never attempted?

I don't know. Perhaps something has changed in newer SDKs to require a package source to be explicitly configured? I added a nuget.org file to this PR to explicitly configure nuget.org as a source.

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking care of this.

@jtattermusch jtattermusch merged commit 0ca4c1a into protocolbuffers:master Nov 12, 2021
@jtattermusch jtattermusch mentioned this pull request Nov 16, 2021
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.

4 participants