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

Hot Fix: Use fuzzing not feature = "fuzzing" #466

Merged
merged 1 commit into from Jun 28, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jun 28, 2022

Currently the following command fails

RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS='--cfg=fuzzing' cargo test --all --all-features

This is because fuzzing is not a feature, we should be using fuzzing directly not feature = "fuzzing".

I have no idea how this got past CI, found while trying to upgrade secp in bitcoin.

This got past CI because of the feature gate combination #[cfg(all(test, feature = "unstable"))], we never run tests on CI with both DO_FEATURE_MATRIX and DO_BENCH.

if [ "$DO_FEATURE_MATRIX" = true ]; then
...
    if [ "$DO_BENCH" = true ]; then  # proxy for us having a nightly compiler
        cargo test --all --all-features
        RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS='--cfg=fuzzing' cargo test --all --all-features
    fi
fi

Currently the following command fails

`RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS='--cfg=fuzzing' cargo test --all --all-features`

This is because `fuzzing` is not a feature, we should be using `fuzzing`
directly not `feature = "fuzzing"`.

I have no idea how this got past CI.
@tcharding tcharding changed the title Use fuzzing not feature = "fuzzing" Hot Fix: Use fuzzing not feature = "fuzzing" Jun 28, 2022
@apoelstra
Copy link
Member

Neat, thanks! Glad (though unsurprised) that this was only an issue in an obscure edge case (benchmarking with the fuzz-crypto enabled).

@apoelstra
Copy link
Member

I guess this breaks compilation of the fuzztests in rust-bitcoin? So we should publish another point release?

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 0c15c01

@apoelstra apoelstra merged commit 16b7402 into rust-bitcoin:master Jun 28, 2022
@tcharding
Copy link
Member Author

I guess this breaks compilation of the fuzztests in rust-bitcoin? So we should publish another point release?

Means we can't upgrade to 0.23.2 in rust-bitcoin because CI fails. 0.23.3 hear we come :)

@tcharding tcharding deleted the 06-28-cfg-fuzzing branch June 29, 2022 00:50
apoelstra added a commit that referenced this pull request Jun 29, 2022
5f611f6 Conditionally compile the hex macro (Tobin C. Harding)
69349a8 Add NIGHTLY variable to CI script (Tobin C. Harding)

Pull request description:

  We are currently using the DO_BENCH variable as a proxy for whether or not we are using a nightly toolchain, while this is technically correct we use it from within an if guarded statement that is guarded by DO_FEATURE_MATRIX and we never run the CI script with _both_ of these variables set to true. This means that the all features test is never being run.

  Add a NIGHTLY variable and set it based on the output of `cargo --version`.

  This PR catches the bug fixed in: #466 as such it will not be able to be merged until #466 merges.

ACKs for top commit:
  apoelstra:
    ACK 5f611f6

Tree-SHA512: 231bbff8e8944026183a87f681c2d7152c4dcfaaafb6cbd99404e8912d61dbc53c40bb24473c156e893c5b8de79462cb944ed94ffe5429f8b31eaef76dbc0694
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