hyperledger-archives / ursa

Hyperledger Ursa (a shared cryptographic library) has moved to end-of-life status, with the components of Ursa still in use moved to their relevant Hyperledger projects (AnonCreds, Indy, Aries and Iroha).
https://wiki.hyperledger.org/display/ursa
Apache License 2.0
321 stars 142 forks source link

Run libursa tests on Ubuntu 20.04 #180

Closed nhwn closed 3 years ago

nhwn commented 3 years ago

This PR adds libursa-specific jobs to GH Actions (namely building libursa and running its tests).

One thing I'm not sure about is the workaround for getting libursa to build on its own -- currently, running cargo build in the libursa directory on the main branch will yield this error:

error: current package believes it's in a workspace when it's not:
current:   ursa/libursa/Cargo.toml
workspace: ursa/Cargo.toml

this may be fixable by adding `libursa` to the `workspace.members` array of the manifest located at: ursa/Cargo.toml
Alternatively, to keep it out of the workspace, add the package to the `workspace.exclude` array, or add an empty `[workspace]` table to the package's manifest.

I heeded the second suggestion and excluded libzmix and libursa from the workspace, but I'm not sure if this is the correct course of action. Feel free to request/make changes!

As for the changes to libursa/src/kex/secp256k1.rs, the compatibility test was never updated after libsecp256k1 was replaced with k256 in #171, so this PR also fixes that.

dcmiddle commented 3 years ago

This is great. Thanks for catching & fixing the compatibility test.

For the workspace, I think the endgame is that libzmix is gone. I think Mike's intent is that libursa gets split up into the other components, like ursa_signatures so libursa goes away too.

Until that's all refactored, maybe we should just put libursa into the workspace members list so it gets built and tested?

nhwn commented 3 years ago

If we want to put libursa in the workspace members, I think the package name listed in the top-level Cargo.toml needs to change to something other than ursa (otherwise, we'll have 2 packages named ursa in the workspace, which gets rejected by cargo). Honestly, I'm not quite sure what the top-level crate is for since it only re-exports ursa_sharing at the moment, so I don't know if it's risky to rename it to something else or not.

Also, adding libursa to the workspace members won't actually build libursa via cargo build unless it's listed as a dependency in the top-level crate (the other option is to build all the workspace members via cargo build --workspace). Again, I'm unfamiliar with what the top-level crate is supposed to do, so I'll need further input here.

As for actually getting libursa's tests to run in this configuration, we'll need to change the job's cargo invocation to either directly test the libursa package (e.g. cargo test -p ursa) or test all the packages in the workspace (e.g. cargo test --workspace) because cargo test will only run the tests in the top-level crate (and there are none).

Do we want to build and test all the packages simultaneously, or do we want some sort of granularity? (to be clear, I think having everything in the workspace makes sense -- I just want to make sure these changes don't get in the way later)

mac-arrap commented 3 years ago

@mikelodder7 Do you have any input for the concerns listed above?

dcmiddle commented 3 years ago

Based on the conversation in today's community meeting we should rely on the Operation Oso issue description. Essentially don't "fix" libursa in the workspace and CI. Instead to get those components back into testing they should be refactored into the new structure e.g. signatures subproject. https://github.com/hyperledger/ursa/issues/151

The fixes for k256k1 etc. should still remain in this PR though (recognizing that it's awkward that those changes aren't validated in CI until the refactoring happens)

nhwn commented 3 years ago

I've reverted the changes to the workspace/CI as per what we discussed in Wednesday's meeting. At this point, this PR just fixes that one regression test and some compiler warnings, so I'll be opening up more PRs geared at #151 to follow up on this one.