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

Add README docs for proper use of std and no-std features #1711

Merged
merged 1 commit into from Mar 17, 2023

Conversation

notmandatory
Copy link
Contributor

@notmandatory notmandatory commented Mar 16, 2023

Building this crate requires the std and/or no-std features be enabled. This PR documents this build constraint in the README and gives an error is anyone tries to build without enabling one or both of these features.

See discussion in rust-bitcoin/rust-miniscript#533.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK the doc, NACK build error. Just remove it and it's OK.

bitcoin/build.rs Outdated Show resolved Hide resolved
@Kixunil
Copy link
Collaborator

Kixunil commented Mar 16, 2023

Looking at it now, I think you meant to submit the compile error to miniscript (please use compile_error! still).

@notmandatory
Copy link
Contributor Author

notmandatory commented Mar 16, 2023

Looking at it now, I think you meant to submit the compile error to miniscript (please use compile_error! still).

Yes it first came up in the discussion about miniscript but I thought both crates were missing the check. I've updated my miniscript PR to use the same compile_error! used here.

rust-bitcoin/rust-miniscript#534

@notmandatory notmandatory changed the title Add build error and README docs for proper use of std and no-std features Add README docs for proper use of std and no-std features Mar 16, 2023
README.md Outdated
@@ -93,6 +93,10 @@ versions than the current stable one (see MSRV section).

## Building

The cargo feature `std` is enabled by default. At least one of the features `std` or `no-std` or both must be enabled.

Enabling the `no-std` feature does not disable `std`, to disable the `std` feature you must disable default features. The `no-std` feature only enables additional features required for this crate to be usable without `std` and both can be enabled without conflict.
Copy link
Member

Choose a reason for hiding this comment

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

Grammar nit. I'd break this into multiple sentences:

Enabling the `no-std` feature does not disable `std`.
To disable the `std` feature you must disable default features. The `no-std` feature only enables additional features required for this crate to be usable without `std`.
Both can be enabled without conflict.

This is identical to your text, I just split it into 3 sentences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that does read more clearly. I've updated as your suggested.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 33ee7a5

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 33ee7a5

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 33ee7a5

@apoelstra apoelstra merged commit 5125155 into rust-bitcoin:master Mar 17, 2023
22 checks passed
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

4 participants