Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Support .NET Standard 2.1(Take two) #499

Merged
merged 12 commits into from
Dec 7, 2022

Conversation

devhawk
Copy link
Contributor

@devhawk devhawk commented Nov 9, 2022

This is an alternative version of #498. That PR uses C# 8.0. This PR uses C# 9.0. Technically, C# 9 is only supported on .NET 5 and later, but Microsoft itself uses approaches from this PR to target older frameworks from C# 9.0. (Example).

cc @erikzhang @shargon @igormcoelho

  • in order to support init only properties, Neo.VM needs a local internal copy of IsExternalInit. This is a marker class with no code, and is only used by the compiler for tracking metadata.
  • .NET 5 is no longer supported, so this PR only targets .NET Standard 2.1 and 6.0

Other changes in this PR that match changes made in #498:

  • Implement BitOperations.RotateLeft, ReferenceEqualityComparer and BigInteger.GetBitLength on .NET Standard 2.1 (.NET 6 uses the BCL provided versions)
  • Use non generic version of Enum.IsDefined for .NET standard 2.1 compat
  • Use new byte[] instead of GC.AllocateUninitializedArray on .NET Standard 2.1 (.NET 6 continues to use GC.AllocateUninitializedArray)
  • Suppress warning of value parameter nullability mismatch in IDictionary<K,V>.TryGetValue implementations
  • Tarjan.StrongConnectNonRecursive uses mixed declarations and assignment in deconstruction, which is a C# 10 feature. As this is the only post C# 9.0 feature used across the entire assembly, I changed the code to separate assignment and declarations into separate statements. So (v, T? w, IEnumerator<T>? s, int n) = state; became v = state.node; var (_, w, s, n) = state;

Note, like #498, this PR should not be merged until after Neo 3.5 has shipped.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

This version seems better than the other one, it's possible to add the framework test to the workflow?

@devhawk
Copy link
Contributor Author

devhawk commented Nov 10, 2022

This version seems better than the other one, it's possible to add the framework test to the workflow?

Which framework test? I'm not sure I follow

@shargon
Copy link
Member

shargon commented Nov 10, 2022

Which framework test? I'm not sure I follow

Test it twice with the different compiler directives.

@devhawk
Copy link
Contributor Author

devhawk commented Nov 10, 2022

Test it twice with the different compiler directives.

Done. Tests now target netcoreapp3.1 and net6.0, which test the netstandard 2.1 and net6.0 versions of neo.vm respectively. However, only the net 6 version of the code coverage report is uploaded to coveralls.io. I don't have access to the coveralls.io account and I'm not sure what would happen if I uploaded both coverage reports.

@logikonline
Copy link

logikonline commented Nov 18, 2022

@devhawk Do you have a timeframe for updating Neo and RpcClient? I'll gladly test it when ready.

@devhawk
Copy link
Contributor Author

devhawk commented Nov 21, 2022

@devhawk Do you have a timeframe for updating Neo and RpcClient? I'll gladly test it when ready.

I'm hoping the full dotnet standard 2.1 compat work will be done for 3.6 release. There are three parts to this work:

  1. Enable dotnet standard 2.1 compat for Neo.VM (i.e. this PR)
  2. Split neo.dll into two parts, creating a new "nucleus" dll which would have business logic needed by RpcClient. The remaining code needed for running a full node would stay in neo.dll and depend on the new "nucleus" dll. The nucleus DLL would be dotnet standard 2.1 compatible.
  3. update RpcClient to depends on new "nucleus" dll instead of full neo.dll, and add dotnet standard 2.1 targeting

Step 2 is the time consuming one. I have started it, but that work is blocked waiting on this PR. Step 3 should be trivial once step 2 is done.

I will likely be able to provide test bits once they are ready

@devhawk
Copy link
Contributor Author

devhawk commented Dec 5, 2022

Now that Neo.VM 3.5 has shipped, this is ready to merge @erikzhang @shargon

@devhawk
Copy link
Contributor Author

devhawk commented Dec 5, 2022

FYI, there's an issue with coverage generation (I think it's related to coverlet-coverage/coverlet#1391) so not quite ready to merge yet

Not sure if it's a transient issue or if it's an issue with actions/setup-dotnet@v3, but I rolled back to actions/setup-dotnet@v1 and the coverage collection issue went away. Ready to merge

@devhawk
Copy link
Contributor Author

devhawk commented Dec 6, 2022

@shargon can you merge this?

@shargon shargon merged commit 3131095 into neo-project:master Dec 7, 2022
@devhawk devhawk deleted the devhawk/nucleus2 branch December 9, 2022 02:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants