Skip to content
This repository has been archived by the owner on Mar 11, 2024. It is now read-only.

Run libursa tests on Ubuntu 20.04 #180

Merged
merged 8 commits into from
Jun 2, 2021
Merged

Run libursa tests on Ubuntu 20.04 #180

merged 8 commits into from
Jun 2, 2021

Conversation

nhwn
Copy link

@nhwn nhwn commented May 14, 2021

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.

Nathan Nguyen added 5 commits May 13, 2021 22:22
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
@dcmiddle
Copy link
Contributor

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?

@@ -130,6 +130,7 @@ mod ecdh_secp256k1 {
use sha2::digest::generic_array::typenum::U32;
use sha2::Digest;
use zeroize::Zeroize;
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to remove the dead code. What was coming up?

Copy link
Author

Choose a reason for hiding this comment

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

It's not used anywhere, I was just taking a super conservative approach for my changes. I'll go ahead and remove it now.

@nhwn
Copy link
Author

nhwn commented May 14, 2021

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
mac-arrap previously approved these changes May 26, 2021
Copy link
Contributor

@mac-arrap mac-arrap left a comment

Choose a reason for hiding this comment

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

I have nothing to add more than @dcmiddle has mentioned about the code review. Thank you for contributing this. I will look into what you mentioned above

@mac-arrap
Copy link
Contributor

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

@dcmiddle
Copy link
Contributor

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.
#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)

@mac-arrap mac-arrap dismissed their stale review May 26, 2021 16:46

per statement above

Nathan Nguyen added 2 commits May 28, 2021 23:48
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
Signed-off-by: Nathan Nguyen <nathan.nguyen@evernym.com>
@nhwn
Copy link
Author

nhwn commented May 29, 2021

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.

@dcmiddle dcmiddle merged commit 92d7521 into hyperledger-archives:main Jun 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants