jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.22k stars 1.74k forks source link

Add multi-plat nuget package testing #1070

Closed enclave-alistair closed 3 years ago

enclave-alistair commented 3 years ago

Figured out this morning how we can fairly easily multi-arch-test the nuget package process:

Using the self-contained publish means that we can directly execute the generated Tests binary (built for the appropriate processor architecture), without needing to install the .NET Runtime or SDK.

Also fixed the wildcard problem with uploading the nuget package by switching to the new version of the upload-artifact action.

enclave-alistair commented 3 years ago

@ektrah, the build doesn't run when targeting master, but you can see the build output on stable in my fork: https://github.com/enclave-alistair/libsodium/actions/runs/881541317

ektrah commented 3 years ago

That’s pretty neat, thank you very much!

I’m not sure if it’s testing the right thing, though. Let me think about it... Thinking out loud, we already know that the precompiled binaries work on each platform, because we’re running make check. So the interesting part is whether the nupkg works, i.e., whether the dotnet tool correctly finds the binaries in the package. When you publish the app, it clearly does. But then, by providing the RID as a parameter, you override the automatic RID detection that would normally happen when you use the SDK to compile and run your app on the platform directly... 🧐

enclave-alistair commented 3 years ago

Well, there's two phases to the build process here:

  1. RID Detection by the .NET SDK (which I'm overriding in the publish instruction). Our nuget package has no impact on this process. The tests I've added do not test this, because it's not related to our nupkg.
  2. The SDK looks at our NuGet package and checks if the libsodium package provides a runtime binary to copy to the build output for the selected RID. This is what we are testing.

I'm not sure we gain much value from testing the .NET RID detection. Perhaps we could change to target ubuntu-arm64 instead, to test the fallback to linux-arm64? Again the value for me is marginal. Fundamentally we're following the runtime guidance described here, so we have to trust that to some extent.

ektrah commented 3 years ago

OK, I'm convinced 😃

Background: .NET RID detection is actually the number 1 problem with the libsodium package (which really speaks for libsodium -- either .NET can find the binary in the nupkg, then it just works, or it doesn't). In particular on Alpine there are often bugs in the .NET RID detection that then result in bug reports for the libsodium package (or rather the packages having libsodium as a dependency). I'd like to ensure that the libsodium package is tested and really works, so I can close the bug reports and blame the broken .NET RID detection 😁

enclave-alistair commented 3 years ago

Yeah, fundamentally there's not a lot we can do if the RID detection is flawed; all we can really do is follow the rules defined in the runtime.json. Not a lot we can do beyond that.

enclave-alistair commented 3 years ago

@jedisct1, I believe this set of changes are ready to merge? The LGTM analysis checks seems to be failing due to a git issue, but I believe @ektrah and I are agreed on the approach.

That then unblocks the release of the 1.0.18.1 version on stable.

jedisct1 commented 3 years ago

Thanks a lot!

(and to @ektrah for reviewing this!)

enclave-alistair commented 3 years ago

Great, thanks @jedisct1; I think since you've already bumped the versions to 1.0.18.1, the only things to do now are merge to next/stable, then upload the built nupkg file to nuget.org.

enclave-alistair commented 3 years ago

@jedisct1, sorry to ask, but is there something else pending before you intend to upload 1.0.18.1 to the main nuget repository?

I'm happy to help if there is something else in the way, or anything I can do.