Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to new vsss-rs 5.x stable version #502

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

cr-tk
Copy link
Collaborator

@cr-tk cr-tk commented Feb 24, 2025

Summary & Motivation (Problem vs. Solution)

vsss-rs is an important cryptography library for us.

Based on the Michael Lodder's feedback, he's primarily planning to maintain the latest major version, which is now 5.x. This is a motivation for us to move over to it from the current 4.x pinning.

The new major version also has a new zeroize target, which we want to use for security hardening.

How I Tested These Changes

This change is not extensively tested yet.

Pre merge check list

  • Update CHANGELOG.MD

@cr-tk cr-tk added the enhancement New feature or request label Feb 24, 2025
@cr-tk
Copy link
Collaborator Author

cr-tk commented Feb 24, 2025

The stagex-build / build artifacts (pull_request) step seems to fail on env -C /src/init cargo build --locked --no-default-features --release --target x86_64-unknown-linux-musl.

Based on some initial debugging, this is actually not a new problem introduced by the dependency changes, but an existing problem introduced in 9082021 which affects the current main ae94332 unrelated to this PR.

cargo build --locked --release builds, but cargo build --locked --no-default-features --release does not. I think this is due to us making use of qos_net::cli::CLI in qos_net regardless of settings, but then marking qos_net::cli as dependent on the proxy cargo target:

   Compiling qos_host v0.1.0 (qos/src/qos_host)
error[E0432]: unresolved import `qos_net::cli`
 --> qos_net/src/main.rs:1:14
  |
1 | use qos_net::cli::CLI;
  |              ^^^ could not find `cli` in `qos_net`
  |
note: found an item that was configured out
 --> qos/src/qos_net/src/lib.rs:9:9
  |
9 | pub mod cli;
  |         ^^^
note: the item is gated behind the `proxy` feature
 --> qos/src/qos_net/src/lib.rs:8:7
  |
8 | #[cfg(feature = "proxy")]
  |       ^^^^^^^^^^^^^^^^^

I think this bug is normally hidden by the fact that the qos_net default target includes proxy. Building with --no-default-features overrides this behavior, triggering the problem.

@r-n-o Can you take a look at this and confirm the above? I'm not sure why the test didn't fail back when we introduced #449 , but perhaps the stagex container behavior/action was different back then.

@r-n-o
Copy link
Contributor

r-n-o commented Feb 26, 2025

@cr-tk I checked out your branch and ran cargo build --locked --no-default-features --release; also getting the same error locally.

I have no idea why this didn't surface earlier. Your assessment is correct, when default features are disabled, main.rs can't be built because it requires the proxy feature. I think the safest thing to do is to define a main function when the feature is disabled, but because qos_net's CLI is the thing which starts the proxy, it doesn't make sense for it to do anything useful hence a potential patch that looks like this:

iff --git a/src/qos_net/src/main.rs b/src/qos_net/src/main.rs
index 76ebae57..c4398422 100644
--- a/src/qos_net/src/main.rs
+++ b/src/qos_net/src/main.rs
@@ -1,5 +1,14 @@
+#[cfg(feature = "proxy")]
 use qos_net::cli::CLI;
 
+#[cfg(feature = "proxy")]
 pub fn main() {
        CLI::execute();
 }
+
+
+#[cfg(not(feature = "proxy"))]
+pub fn main() {
+       panic!("Cannot run qos_net CLI without proxy feature enabled")
+}

@cr-tk
Copy link
Collaborator Author

cr-tk commented Feb 27, 2025

@r-n-o thanks for looking into this!
I think your proposed patch is good.
Creating an always panic'ing CLI isn't ideal, but I don't think there's an easy way to make cargo ignore the main.rs file completely in some target configurations. This is probably the most explicit way to handle it, and make it clear that the CLI isn't functioning.

Should we bring in this patch via a separate PR to fast-track, or include it here? I have a slight preference for a separate simple PR, and can create it based on your patch 🙂

@r-n-o
Copy link
Contributor

r-n-o commented Feb 27, 2025

Should we bring in this patch via a separate PR to fast-track, or include it here? I have a slight preference for a separate simple PR, and can create it based on your patch 🙂

Separate PR sounds good! Here it is: #504

@r-n-o
Copy link
Contributor

r-n-o commented Feb 28, 2025

@cr-tk the fix has been merged, you should be able to rebase your PR and get a clean build now!

@cr-tk cr-tk force-pushed the christian/vsss-rs-5 branch from a6ddbe1 to 98bfca3 Compare February 28, 2025 16:54
@cr-tk
Copy link
Collaborator Author

cr-tk commented Feb 28, 2025

@r-n-o thanks, I pushed the rebased version. Looks good locally - I think this is ready for review if the CI tests succeed.

@cr-tk cr-tk marked this pull request as ready for review February 28, 2025 16:56
@cr-tk cr-tk requested a review from a team as a code owner February 28, 2025 16:56
@cr-tk cr-tk force-pushed the christian/vsss-rs-5 branch from 98bfca3 to a486f05 Compare March 12, 2025 15:20
@cr-tk
Copy link
Collaborator Author

cr-tk commented Mar 12, 2025

We identified the second CI problem. src/init/Cargo.lock also needs updates, but is excluded from the normal Rust workspace, which obscured the issue.

Let's see if this change resolves it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants