nethermind icon indicating copy to clipboard operation
nethermind copied to clipboard

Replace submodules with packages

Open rubo opened this issue 2 years ago • 3 comments

Changes:

IF APPROVED, DO NOT MERGE

The following submodules have been replaced with their NuGet packages:

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply

  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] Documentation Update
  • [ ] Code style update (formatting, renaming)
  • [x] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] Other (please describe):

Testing

Requires testing

  • [ ] Yes
  • [x] No

In case you checked yes, did you write tests??

  • [ ] Yes
  • [x] No

Further comments (optional)

Replacing packages contain the same code as the replaced submodules. For the upcoming refactoring, packages have a bit different naming structure than the underlying code. For instance, the package is named Nethermind.Numerics.Dirichlet while its code is in the namespace Nethermind.Dirichlet.Numerics. The latter should be updated to match the package name later.

Some redundant/obsolete files have been removed.

rubo avatar Aug 06 '22 15:08 rubo

Generally like it, but:

  1. We will release those packes to public nuget?
  2. Do we have the process of realeasing to public nuget?
  3. Can we automate the process in all those repos and have a simple github action to click?
  4. Do we still need Dirchlet? I think we should use a very small subset if any.
  5. Are we fine license wise? Please take https://github.com/NethermindEth/Math.Gmp.Native/pull/6 into consideration
  6. My biggest excitement here is for releasing world-class Int256 package to the world, something that was on my TODO list. Can we make sure it will be easily used by public (like readme or anything else there).

LukaszRozmej avatar Aug 08 '22 10:08 LukaszRozmej

In short, everything you mentioned is already addressed.

Now, a bit more detailed answers:

  1. Yes, we'll publish the packages to nuget.org. Well, they already are, but to nugettest.org.
  2. Yes, we do. Packages can be packed and published using GitHub Actions.
  3. Yes, we do. See above.
  4. Some of our tests have a reference to that. And only that subset is published as a package, not the entire repo.
  5. Although I'm not a lawyer, I concluded that in our particular case we could use MIT with an LGPL-licensed library which GMP is. Also, we retain all the copyright notices and original licenses in the NOTICE file in our repos.
  6. Yes, of course. This is just the first and the heaviest step to simply ditch the submodules. Once done, we can improve our packages step by step.

This said, if approved, the packages will be published to nuget.org, this PR will be merged, and new PRs will be opened in package project repos. Then, @matilote (Angkor team) will take control. In a nutshell, everything is done, I just need a green light to proceed.

rubo avatar Aug 08 '22 14:08 rubo

For licensing, the best answer is the one from a legal advisor but take a look at this. There are a lot of other licenses combined with LGPL. Basically, we release our code under MIT while GMP remains under LGPL. IMO, for our use case, it's ok while others should take care of their cases themselves if any.

I'd consider making secp256k1 a package as well as a next step. The deleted secp256k1 files clearly were leftovers from the initial code. Generally, I see a lot of old stuff in the repo that looks either abandoned or obsolete. But I left that out as a next step.

With this said, I'm pretty sure we're good to go. This is not a risky change and we have a few days to play around.

rubo avatar Aug 08 '22 20:08 rubo