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

Make std an optional dependency. Require alloc instead. #869

Merged
merged 3 commits into from
Jul 10, 2019
Merged

Conversation

briansmith
Copy link
Owner

This raises the minimum version of Rust required to 1.36.

@josephlr
Copy link
Contributor

This is great! If you're making backwards-incompatible changes to the Cargo features, it might be worth reconsidering the dev_urandom_fallback feature.

If I'm understanding the motivation correctly, the main reason to not have the dev_urandom_fallback feature is to avoid depending on libstd. However, if we now have separate std and alloc features, it might make sense to have the std feature control if we read from /dev/urandom on Linux.

Once #558 is fixed, there's not a good security reason to disable the fallback. However, this might be better addressed in a followup PR.

Copy link
Contributor

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

Just a few cfg related nits.

Edit: I now see that the commit messages that said alloc is required for testing. If that's the case then adding the following code to lib.rs might help:

#[cfg(all(test, not(feature = "alloc")))]
compile_error!("Testing requires the alloc feature");

@@ -36,7 +36,7 @@ use untrusted;
/// enum Error {
/// CryptoError,
///
/// # #[cfg(feature = "use_heap")]
/// # #[cfg(feature = "alloc")]
Copy link
Contributor

Choose a reason for hiding this comment

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

feature = "std"

extern crate std;

#[macro_use]
mod debug;

#[cfg(any(test, feature = "use_heap"))]
#[macro_use]
Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs a #[cfg(test)] in order for cargo build --no-default-features to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, #[cfg(feature = "alloc")] is what you want here, so that the integration tests keep working.

src/lib.rs Outdated
@@ -62,13 +65,15 @@
#![no_std]
#![cfg_attr(feature = "internal_benches", allow(unstable_features), feature(test))]

#[cfg(any(test, feature = "use_heap"))]
#[cfg(any(test, feature = "alloc"))]
Copy link
Contributor

@josephlr josephlr Jul 10, 2019

Choose a reason for hiding this comment

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

#[cfg(feature = "alloc")] is all that's needed here, as test requires alloc.

In general, there a lot of #[cfg(any(test, feature = "alloc"))] in the code. These should be simplified to #[cfg( feature = "alloc")]

src/lib.rs Outdated
#[cfg(any(test, feature = "alloc"))]
extern crate alloc;

#[cfg(any(test, feature = "std"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

#[cfg(feature = "std")] is what's needed here, otherwise the current cfg make tests require std.


use crate::{digest, error};
#[cfg(feature = "alloc")]
Copy link
Contributor

Choose a reason for hiding this comment

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

If #[cfg(feature = "alloc")] is added to lib.rs then these annotations are not needed in this file (here and elsewhere).

@briansmith
Copy link
Owner Author

Thanks. I updated the PR to address most of your concerns.

There is more work that needs to be done to properly support testing for no_std environments, e.g. adding the non-default configurations to CI. Those changes will have to wait until my pipeline is cleared.

Copy link
Contributor

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

Looks awesome. cargo test --no-default-features --features=alloc and cargo build --no-default-features work w/o errors.

@briansmith briansmith merged commit 9c42fa1 into master Jul 10, 2019
@briansmith briansmith deleted the b/alloc branch July 10, 2019 07:15
@briansmith
Copy link
Owner Author

Thanks for the review!

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