75f3886812 Add cargo fmt to pre-commit githook (Tobin C. Harding)
0516ddeb8d Add formatting check to CI (Tobin C. Harding)
c7807dff9c Run the formatter (Tobin C. Harding)
Pull request description:
We recently introduced `rustfmt` to the codebase but I forgot to turn it on in CI.
- Patch 1: Preparatory formatting fixes, introduced since we merged the [formatting PR](https://github.com/rust-bitcoin/rust-secp256k1/pull/499)
- Patch 2: Enable formatting in CI
- Patch 3: Add formatting to the pre-commit hook
ACKs for top commit:
apoelstra:
ACK 75f3886812
Tree-SHA512: 5ac4ab4015a9728ef890e0c4fe90afcb5e45ab7665da5a8ee289dc877c1ea5c6236e54b68b7122841597864b04606c8bfae7dec86c4b6be74d32437299057b5f
Add code to the CI script, guarded on env var `DO_FMT` to run the
formatter.
Add a formatting job to the nightly CI job as a separate step, in a
similar fashion to how the other nightly steps are done.
Currently we have a few problems with our feature gating, attempt to
audit all feature gating and fix it by doing:
1. Do not enable features on optional dependencies (`rand` and
`bitcoin-hashes`) in dev-dependencies, doing so hides broken feature
gating in unit tests.
2. Do not use unnecessary feature combinations when one feature enables
another e.g. `any(feature = "std", feature = "alloc")`.
3. Enable "std" from "rand-std" and "bitcoin-std" (and fix features
gating as for point 2).
4. Clean up code around `rand::thread_rng`, this is part of this patch
because `thread_rng` requires the "rand-std" feature.
5. Clean up CI test script to test each feature individually now that
"rand-std" and "bitcoin-hashes-std" enable "std".
Add `-- -D rustdoc::broken-intra-doc-links` to the docs build in CI to
check for broken links. In order to use this flag use `cargo rustdoc`
instead of `cargo doc`.
Currently we have a feature `bitcoin-hashes-std` and a dependency
`bitcoin_hashes`, this means one has to think about and change the `_`
and `-` when coding. The underscore in `bitcoin_hashes` is an artifact
of days gone by and we cannot fix it but we can cover it up and make our
lives easier, especially now we have `bitcoin-hashes-std`.
Improve feature usage of the `bitcoin_hashes` library by:
- Add a feature `bitcoin-hashes` that enables `bitcoin_hashes`.
- Use the new feature in all feature gated code
- Use `bitcoin-hashes-std` in feature gated code that includes other
`std` features (e.g. `rand-std`)
The current version of clang is 14, there is no obvious reason why we
use clang-9 (as far as I can tell on my local machine).
Use `clang` instead of `clang-9` so that the latest version is used by
default. This effects the version installed by CI as well as the
version used to run commands in `test.sh`.
shellcheck emits these two warnings:
SC2097: This assignment is only seen by the forked process.
SC2098: This expansion will not see the mentioned assignment.
Set `RUSTDOCFLAGS` explicitly to `--cfg=fuzzing` instead of trying to
use the `RUSTFLAGS` variable.
We define a list of features that should be tested along with "std" but
we don't actually use it. Add a call to `cargo test` that enables "std"
and all the features from `STD_FEATURES`.
Found by `shellcheck`.
As we did in rust-bitcoin [0] create a configuration conditional `bench`
that we can use to guard bench mark code. This has the benefit of
making our features additive i.e., we can now test with `--all-features`
with a stable toolchain (currently this fails because of our use of the
`test` crate).
[0] - https://github.com/rust-bitcoin/rust-bitcoin/pull/1092
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`.
97dc0ea9ac Run correct clang --version (Tobin Harding)
a3582ff77d test.sh: Use set -e to exit on failure (Tobin Harding)
7bec31c3a6 test.sh: explicitly return 0 (Tobin Harding)
Pull request description:
Change the test script to exit with non-zero status code if any command fails.
The `test.sh` script is silently failing, that means changes causing failures are slipping through our CI pipeline and being merged.
Resolves: #419
## Note
Just the last 3 patches, the first 6 are from #420. re-base just shows it works on top of 420, it is going to have to be rebased again when 420 merges.
ACKs for top commit:
apoelstra:
ACK 97dc0ea9ac
Tree-SHA512: b86a6876d8c45a2b90b7b3c8adbc08ad6f49b430b1cfaec31cd2de8441cb96af39c63da02b98d6ed71dfab045d466d71d3757297886b5e44ebb6cbaeb4ed32dd
For the test that uses `clang-9` do the sanity call using `clang-9`
instead of `clang`.
For the test that uses `clang` add a sanity call to `clang --version`.
Currently the `test.sh` script is silently failing because we do not
exit if a command fails. We can achieve this by using the Bash builtin
`set -e`.
For some reason I cannot explain a chain of commands that fails does not
fail the script. Instead of working out _why_ just remove the chain and
run each command on its own. This is functionally the same and, I hazard
a guess, is what the original author hoped to achieve with the chaining.
The test that checks for a panic uses `cargo test --exact`, it makes
sense to put it at the top of the script right after we run `cargo test`
so we can run the test without triggering a re-build.
We have unnecessary runs of the `test.sh` script. We can simplify the CI
pipeline and at the same time improve the docs build by using `--cfg
docsrs`.
- Remove the `wasm` job, replace it by enabling the `DO_WASM` env var for
the stable toolchain run in the `Tests` job.
- Add `--cfg docrs` flag to the docs build and set the `DO_DOCS` env var
as part of the nightly toolchain run in `Tests` job.
The end result is one less run of the `test.sh` script and better test
coverage.
Currently we use 'no default features' for the `bitcoin_hashes`
dependency. Doing so means that if users want the `std` feature they
need to explicitly add a `bitcoin_hashes` dependency even though we
re-export `bitcoin_hashes` as `hashes`. This means that in the common
case the re-export is pointless. As an example, `rust-bitcoin`
unnecessarily requires an explicit dependency on `bitcoin_hashes`.
Add `bitcoin-hashes-std` feature so that users do not need an explicit
dependency in the common use case.
Change the test matrix to only test '*-std' features when 'std' is
enabled since enabling one without the other is illogical. Please note,
this replaces the test run of feature 'std'+'rand'+'rand-std' with just
'std'+'rand-std' because enabling 'rand-std' enables 'rand' so the
explicit additional feature is redundant.
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.
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".
Panicking from C is not UB in newer rust versions and will reliably
trigger an abort (without unwinding). In older rust versions, it is
technically UB but empirically it seems to "just work" (and what should
it realistically do except crashing, which is what we intent).
Since there's potentially no unwinding, we can't test this behavior
using [should_panic]. This PR will instead check the libtest output
explicitly in our CI tests.
Fixes#228.
It's super dangerous to use Cargo features for this, since they can be set
accidentally (or maliciously by any crate in a user's entire dep tree). Instead
we can just require users set `RUSTFLAGS` appropriately, which we can easily
do in our fuzzing scripts.
Andrew Poelstra
Tobin C. Harding
Devrandom
Tim Ruffing
Elichai Turkel