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

Enable test with --no-default-features #386

Merged
merged 7 commits into from
Feb 3, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jan 25, 2022

As indicated by the comment in contrib/test.sh we should be able to test with --no-default-features.

  • Patch 1 uses fully qualified path to remove a build warning.
  • Patch 2 adds additional Debug implementations for secrets, uses bitcoin_hashes if available, please review carefully.
  • Patch 3 adds std as an explicit requirement for the three examples
  • Patch 4 enables `cargo test --no-default-features, fixes all the feature gating in unit tests.
  • Patch 5 fixes lint warnings generated while running the feature matrix in contrib/test.sh.

Please Note: Currently the alloc feature cannot be built with Rust 1.29, this made it into master because we don't build ever with the alloc feature enabled in CI. This PR should add alloc to the features matrix but it does not. Adds a TODO comment to contrib/test.sh to add it once we bump MSRV.

When building with --no-default-features the compiler emits:

  warning: unused import: `mem`

The call site is feature gated so we either need to feature gate the
import or use a fully qualified path. Since 'core' is quite short elect
to use the fully qualified path.
@tcharding tcharding marked this pull request as draft January 26, 2022 02:37
@tcharding tcharding changed the title Enable build with --no-default-features Enable test with --no-default-features Jan 26, 2022
@tcharding tcharding force-pushed the no-default-features branch 5 times, most recently from bfc5721 to 38b5128 Compare February 1, 2022 02:27
The `Debug` implementation for secrets is feature gated on `std` because
it uses a hasher from `std`. If `bitcoin_hashes` is enabled we can use
it for hashing. If neither `std` nor `bitcoin_hashes` is enabled fall
back to outputting:

<secret requires std or bitcoin_hashes feature to display>

Remove the docs conditional since we now implement `Debug` always.
@tcharding tcharding force-pushed the no-default-features branch 3 times, most recently from f086f7d to 755bde2 Compare February 1, 2022 04:10
The examples depend on having the "std" feature [1]. In preparation for
being able to run tests with `--no-default-features` add the "std"
feature as a requirement for all three examples. While we are at it use
the correct rand feature requirement: `rand-std`.

[1] Technically we only need "alloc" but "alloc" is not working with
Rust 1.29 currently so just use "std".
Currently various features fail to build when enabled without default
features. This is because many tests need feature gating.

Feature gating the import statements quickly turns into spaghetti when
trying to cover all combinations of two features correctly, instead just
allow unused imports on `tests` modules where needed.

Add correct feature requirements to the examples so they also can be run
without default features.

Improve the CI script by doing:

- Add `std` to the feature matrix.
- Add `--no-default-features` to test runs in the CI script.
Various combinations of features trigger lint warnings for unused code,
all warnings are caused by incorrect feature gating.

Correct feature gating to remove Clippy warnings during testing.
Seems there is a bug in cargo, the tests in `key.rs` run successfully
but AFAICT they should fail. Here is an example, running `cargo test
--features=rand` should make this test fail but it doesn't?
```
/// Secret 256-bit key used as `x` in an ECDSA signature.
///
/// # Examples
///
/// Basic usage:
///
/// ```
/// # #[cfg(all(feature = "rand", any(feature =  "alloc", feature = "std")))] {
/// use secp256k1::{rand, Secp256k1, SecretKey};
///
/// let secp = Secp256k1::new();
/// let secret_key = SecretKey::new(&mut rand::thread_rng());
/// # }
/// ```

Anywho, use the correct feature gate: `rand-std`.
@tcharding tcharding force-pushed the no-default-features branch from 755bde2 to f3688ec Compare February 1, 2022 04:21
@tcharding
Copy link
Member Author

Oh lordy me, it passed CI!

@tcharding tcharding marked this pull request as ready for review February 1, 2022 04:45
@apoelstra
Copy link
Member

I think alloc not building with 1.29 is part of our "the MSRV is different for no-std" policy. But yes, this should be explicit and CI-tested.

src/schnorr.rs Outdated
let kp = KeyPair::from_seckey_slice(&secp, &SK_BYTES).expect("sk");

// In fuzzing mode secret->public key derivation is different, so
// hard-code the epected result.
Copy link
Member

Choose a reason for hiding this comment

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

In ae3e06f:

Typo expected. (I know this is just moved code but we might as well fix it here)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha, there were three instances of this typo. I fixed them all as an additional patch. Cheers.

apoelstra
apoelstra previously approved these changes Feb 2, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK f3688ec

Thanks for this!

Fix minor spelling mistake in code comments.
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK c30026d

@apoelstra apoelstra merged commit 9b5c509 into rust-bitcoin:master Feb 3, 2022
@tcharding tcharding deleted the no-default-features branch February 9, 2022 06:18
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.

None yet

2 participants