Create configuration conditional "bench" #1092

tcharding · 2022-07-10T22:59:13Z

Currently we are unable to build with all features enabled with a non-nightly toolchain, this is because of the use of

`#![cfg_attr(all(test, feature = "unstable"), feature(test))]`

which causes the following error when building:

error[E0554]: #![feature] may not be used on the stable release channel

The "unstable" feature is used to guard bench mark modules, this is widely suggested online but there is a better way.

When running the bench marks use the following incantation:

`RUSTFLAGS='--cfg=bench' cargo bench`

This creates a configuration conditional "bench" that can be used to guard the bench mark modules.

#[cfg(bench)]
mod benches {
    ...
}

apoelstra · 2022-07-11T15:34:51Z

What if you also add cfg=test?

Kixunil · 2022-07-11T15:39:13Z

Perhaps #[cfg(bench)] extern crate test;?

apoelstra · 2022-07-12T01:12:17Z

Very nice! Would be nice to also mention this in the README -- and we should move all the crates in the ecosystem over to this scheme.

apoelstra

ACK 56050115770b51f299258335fbd253566dff82cd

Kixunil · 2022-07-12T10:04:36Z

contrib/test.sh

 if [ "$DO_BENCH" = true ]
 then
-    cargo bench --features unstable
+    RUSTFLAGS='--cfg=bench' cargo bench


Maybe cargo +nightly bench?

This would make it impossible to use specific toolchains for benching, no?

If you have multiple nightlies, yes (is that even possible?) Also I think +beta is also possible. Stable can't work because extern crate test is experimental.

You can definitely have multiple nightlies; you specify them by date. This is sometimes useful for bisecting when a nightly broke things.

Understood that stable won't work...we should, I suppose, check whether the user has specified a toolchain and provide a helpful error message if he hasn't.

By 'specify' you mean by using TOOLCHAIN=foo, right? We don't do that in test.sh so I don't think we should warn
for not being set.

That is what I mean, and I don't understand what you mean by "we don't do that". We have a check for it on line 6.

Oh I mean we never set the TOOLCHAIN env variable in CI. We could check if TOOLCHAIN is set, and check what toolchain is being used, then output an error message if TOOLCHAIN is set and current toolchain is not nightly.

I've added error output to the DO_BENCH code block (as a separate preparatory patch). Its untested because

I'm so confident in my bash foo ;)

The is no DO_FEATURE_MATRIX in rust-bitcoin version of the script so running it (and testing it) takes ages [0]

I started hacking it apart but backed out and started a discussion on the CI scripts: #1096

[0] - I'm sure someone wise once said that if tests are too hard to run they are never run.

Ah, I see, in CI we just install nightly and don't bother with the TOOLCHAIN variable ... ok, that's fine. I don't see any sensible way to output a warning here.

tcharding · 2022-07-13T00:02:03Z

Changes in force push:

Mentioned beta in comment: # Bench if told to, only works with non-stable toolchain (nightly, beta).

tcharding · 2022-07-13T00:03:37Z

Perhaps #[cfg(bench)] extern crate test;?

Forgot to say the other day, you were right @Kixunil, thanks man!

tcharding · 2022-07-13T00:04:07Z

Woops, readme suggestion still to come ...

In markdown triple ticks go on a line of their own. This change does not effect the rendering in GitHub which is managing to parse this section correctly already.

tcharding · 2022-07-13T00:11:20Z

Now includes readme section (and an initial patch fixing up a trivial markdown issue in the readme file). I put it as a sub section of the Building section.

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). Please note, this patch maintains the current behaviour of turning on the `recovery` and `rand-std` features when benching although I was unable to ascertain why this is needed. [0] - rust-bitcoin/rust-bitcoin#1092

tcharding · 2022-07-13T04:51:52Z

I thought that cfg_docs was not an experimental feature but I was wrong: https://doc.rust-lang.org/unstable-book/language-features/doc-cfg.html

Force push puts the comment about experimental features back to how it was and groups cfg_docs together with test as it was.

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). Please note, this patch maintains the current behaviour of turning on the `recovery` and `rand-std` features when benching although I was unable to ascertain why this is needed. [0] - rust-bitcoin/rust-bitcoin#1092

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] - rust-bitcoin/rust-bitcoin#1092

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). Please note, this patch maintains the current behaviour of turning on the recovery and rand-std features when benching although I was unable to ascertain why this is needed. Maintains the use of `--features=compiler` in `test.sh` and in the suggested incantation in `README.md`. [0] - rust-bitcoin/rust-bitcoin#1092

apoelstra

ACK 64d50b9a7a187f56a1713c36effd10ea7c45385c

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] - rust-bitcoin/rust-bitcoin#1092

Add trivial calls to `cargo` and `rustc` as sanity checks before starting the script proper.

If CI script is run with `DO_BENCH=true` and `TOOLCHAIN` set to a non-nightly toolchain the build will fail with a less than meaningful error. To assist runners of the script output an informative error message if an attempt is made at using the wrong toolchain.

Currently we are unable to build with all features enabled with a non-nightly toolchain, this is because of the use of `#![cfg_attr(all(test, feature = "unstable"), feature(test))]` which causes the following error when building: error[E0554]: `#![feature]` may not be used on the stable release channel The "unstable" feature is used to guard bench mark modules, this is widely suggested online but there is a better way. When running the bench marks use the following incantation: `RUSTFLAGS='--cfg=bench' cargo bench` This creates a configuration conditional "bench" that can be used to guard the bench mark modules. #[cfg(bench)] mod benches { ... }

Kixunil

ACK f3b2120

BTW if we renamed TOOLCHAIN to RUSTUP_TOOLCHAIN we could get rid of alias and possibly it'd be more obvious what it does/how it works.

tcharding · 2022-07-14T23:58:35Z

BTW if we renamed TOOLCHAIN to RUSTUP_TOOLCHAIN we could get rid of alias and possibly it'd be more obvious what it does/how it works.

Cool! Added to my todo list as I work on improving CI.

apoelstra

ACK f3b2120

apoelstra · 2022-07-15T13:07:02Z

@tcharding anything more you wanna add to this PR, or should we merge it?

tcharding · 2022-07-17T22:06:35Z

Its good to go mate.

a431edb Create configuration conditional bench (Tobin C. Harding) 2a1c9ab Remove rand-std feature from unstable (Tobin C. Harding) ddc108c Increase heading size (Tobin C. Harding) 596adff Remove unneeded whitespace (Tobin C. Harding) Pull request description: 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). Please note, this patch maintains the current behaviour of turning on the `recovery` and `rand-std` features when benching although I was unable to ascertain why this is needed. [0] - rust-bitcoin/rust-bitcoin#1092 ACKs for top commit: sanket1729: ACK a431edb. apoelstra: ACK a431edb Tree-SHA512: 913f5fbe0da08ec649081bf237c1d31cee58dacdac251d6030afabb99d455286c6d1dbdb6b2ac892b5d3c24584933254d1cfeec8e12f531cc420bd9d455a6531

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] - rust-bitcoin/rust-bitcoin#1092

…l "bench" f3b2120 Create configuration conditional bench (Tobin C. Harding) f60c92c Add informative error message to DO_BENCH (Tobin C. Harding) c6d5a12 Add cargo/rustc sanity calls (Tobin C. Harding) 34d5a31 Put triple ticks on their own line (Tobin C. Harding) Pull request description: Currently we are unable to build with all features enabled with a non-nightly toolchain, this is because of the use of `#![cfg_attr(all(test, feature = "unstable"), feature(test))]` which causes the following error when building: error[E0554]: `#![feature]` may not be used on the stable release channel The "unstable" feature is used to guard bench mark modules, this is widely suggested online but there is a better way. When running the bench marks use the following incantation: `RUSTFLAGS='--cfg=bench' cargo bench` This creates a configuration conditional "bench" that can be used to guard the bench mark modules. ``` #[cfg(bench)] mod benches { ... } ``` ACKs for top commit: Kixunil: ACK f3b2120 apoelstra: ACK f3b2120 Tree-SHA512: 7ec2a501a30bfe2ce72601077cd675cf5e5ac2f0f93f97fc7e83cb7401606b69ae909b35bfc0ace8bd1ea771ca4fba70e2ad9ac9ba26f2b6e371494cf694c0a8

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] - rust-bitcoin/rust-bitcoin#1092

a7b7bed Create configuration conditional bench (Tobin C. Harding) 5f6c7ad Remove unneeded line of whitespace (Tobin C. Harding) Pull request description: 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] - rust-bitcoin/rust-bitcoin#1092 ACKs for top commit: apoelstra: ACK a7b7bed Tree-SHA512: a451e970a543597bd3714e7e9a57cb6325700ff9c3f5e68347a40a37263f95ea16aeaf8beeca0463a27a5f63430d48c2db5b86f7e21f9ee196cf49a9a38c1730

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). Please note, this patch maintains the current behaviour of turning on the recovery and rand-std features when benching although I was unable to ascertain why this is needed. Maintains the use of `--features=compiler` in `test.sh` and in the suggested incantation in `README.md`. [0] - rust-bitcoin/rust-bitcoin#1092

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] - rust-bitcoin/rust-bitcoin#1092

…al "bench" 79ccdc4 Create configuration conditional bench (Tobin C. Harding) 5335135 Remove unneeded line of whitespace (Tobin C. Harding) Pull request description: 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] - rust-bitcoin/rust-bitcoin#1092 ACKs for top commit: apoelstra: ACK 79ccdc4 Tree-SHA512: a451e970a543597bd3714e7e9a57cb6325700ff9c3f5e68347a40a37263f95ea16aeaf8beeca0463a27a5f63430d48c2db5b86f7e21f9ee196cf49a9a38c1730

tcharding mentioned this pull request Jul 10, 2022

Add cfg nightly #1080

Closed

tcharding force-pushed the 07-11-cfg-nightly branch from 64aae19 to 9d14723 Compare July 11, 2022 01:50

tcharding force-pushed the 07-11-cfg-nightly branch 2 times, most recently from af36b06 to 5605011 Compare July 12, 2022 00:09

tcharding changed the title ~~WIP: Attempt to use --cfg=bench~~ Create configuration conditional "bench" Jul 12, 2022

tcharding marked this pull request as ready for review July 12, 2022 00:28

apoelstra previously approved these changes Jul 12, 2022

View reviewed changes

Kixunil reviewed Jul 12, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 7174117 July 13, 2022 00:01

tcharding force-pushed the 07-11-cfg-nightly branch from 5605011 to 7174117 Compare July 13, 2022 00:01

Put triple ticks on their own line

8000 34d5a31

In markdown triple ticks go on a line of their own. This change does not effect the rendering in GitHub which is managing to parse this section correctly already.

tcharding force-pushed the 07-11-cfg-nightly branch from 7174117 to 58334cf Compare July 13, 2022 00:10

tcharding mentioned this pull request Jul 13, 2022

Create configuration conditional "bench" rust-bitcoin/rust-secp256k1#473

Merged

tcharding force-pushed the 07-11-cfg-nightly branch from 58334cf to 64d50b9 Compare July 13, 2022 04:50

tcharding mentioned this pull request Jul 13, 2022

Create configuration conditional "bench" rust-bitcoin/bitcoin_hashes#166

Merged

tcharding mentioned this pull request Jul 13, 2022

Create configuration conditional bench rust-bitcoin/rust-miniscript#447

Closed

RCasatta mentioned this pull request Jul 13, 2022

Create conditional configuration "bench" RC 9E81 asatta/blocks_iterator#58

Closed

apoelstra previously approved these changes Jul 13, 2022

View reviewed changes

tcharding added 3 commits July 14, 2022 09:47

Add cargo/rustc sanity calls

c6d5a12

Add trivial calls to `cargo` and `rustc` as sanity checks before starting the script proper.

tcharding dismissed apoelstra’s stale review via f3b2120 July 14, 2022 00:21

tcharding force-pushed the 07-11-cfg-nightly branch from 64d50b9 to f3b2120 Compare July 14, 2022 00:21

Kixunil approved these changes Jul 14, 2022

View reviewed changes

apoelstra approved these changes Jul 15, 2022

View reviewed changes

apoelstra merged commit bd8d9af into rust-bitcoin:master Jul 17, 2022

tcharding deleted the 07-11-cfg-nightly branch July 18, 2022 05:39

Kixunil added the Tests label Aug 1, 2022

Kixunil added this to the 0.29.0 milestone Aug 1, 2022

apoelstra mentioned this pull request Aug 23, 2024

Having list of "broken" commits #3219

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Create configuration conditional "bench" #1092

Create configuration conditional "bench" #1092

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Create configuration conditional "bench" #1092

Create configuration conditional "bench" #1092

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!