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

ci: dedicated profile for release + debug_assert #84

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

conorsch
Copy link
Contributor

We want to use the --release behavior, but doing so directly would disable evaluation of debug_assert lines, so we'll selectively override that one option.

Updates the "check" and "test" jobs to use the --profile ci. Also ensures that "check" covers all targets, including benchmarks, and accordingly cleans up an unused import in same.

Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

Very nice!

@conorsch
Copy link
Contributor Author

To ensure this change does what we want, I applied this diff locally:

index c4f1f58..c25c8ef 100644
--- a/src/elligator.rs
+++ b/src/elligator.rs
@@ -52,6 +52,8 @@ impl Element {
             inner: EdwardsProjective::new(E * H, F * G, E * G, F * H),
         };
 
+        // Intentional panic to verify this line works in tests
+        debug_assert!(false);
         debug_assert!(
             result.inner.is_on_curve(),
             "resulting point must be on the curve",

and confirmed that:

  • cargo nextest run --profile ci --cargo-profile release passed, because debug_asserts are dropped
  • cargo nextest run --profile ci --cargo-profile ci failed, due to the debug_assert(false) being evaluated

That's good enough for me.

We want to use the `--release` behavior, but doing so directly would
disable evaluation of `debug_assert` lines, so we'll selectively
override that one option.

Updates the "check" and "test" jobs to use the `--profile ci`.
Also ensures that "check" covers all targets, including benchmarks,
and accordingly cleans up an unused import in same.
@cronokirby cronokirby merged commit da391d6 into main Jan 31, 2024
6 checks passed
@cronokirby cronokirby deleted the ci-custom-profile-for-tests branch January 31, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants