Skip to content

Use RUSTFLAGS instead of feature to gate benching #1882

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Use RUSTFLAGS instead of feature to gate benching #1882

wants to merge 1 commit into from

Conversation

tcharding
Copy link
Contributor

@tcharding tcharding commented Nov 29, 2022 •
edited
Loading

Currently we use the "_bench_unstable" feature to gate benching code. This results in features not being additive for stable toolchains i.e., one cannot currently run cargo +stable check --all-features (error below). Although this method of gating bench code is widely proposed as a good solution online there is another solution that keeps all features additive.

Guard the bench code with #[cfg(bench)] and when running benches use RUSTFLAGS='--cfg=bench' to turn on bench.

cargo check --all-features
...
error[E0554]: `#![feature]` may not be used on the stable release channel
  --> lightning/src/lib.rs:57:91
   |
57 | #![cfg_attr(all(any(test, feature = "_test_utils"), feature = "_bench_unstable"), feature(test))]

@TheBlueMatt
Copy link
Collaborator

CI is mad but generally looks good, thanks.

@tcharding
Copy link
Contributor Author

Looks like I've botched some of the attributes up, I'll go back over the changes later with fresh eyes.

@dunxen
Copy link
Contributor

dunxen commented Nov 30, 2022

Concept ACK

@tcharding tcharding force-pushed the 11-30-cfg-bench branch 2 times, most recently from 1a8ff43 to 2c6c15b Compare December 1, 2022 03:05
tcharding
tcharding commented Dec 1, 2022
View reviewed changes
lightning/src/routing/router.rs
@@ -5505,7 +5505,7 @@ pub(crate) mod bench_utils {
}
}

#[cfg(all(test, feature = "_bench_unstable", not(feature = "no-std")))]
#[cfg(all(bench, not(features = "no-std")))]
mod benches {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original is incorrect is it not? These benches will never be run because we run benches with "std" enabled since it is enabled by default (RUSTC_BOOTSTRAP=1 RUSTFLAGS='--cfg=bench' cargo bench).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I wrote this comment on an old version of the PR, now it uses #[cfg(bench)].

@tcharding
Copy link
Contributor Author

Drats this can't go in until secp256k1 v0.25.0 is out and used in bitcoin because feature gating was broken for the bench modules in v0.24.0

I'll put this on ice until then.

@tcharding tcharding marked this pull request as draft December 1, 2022 05:24
@TheBlueMatt TheBlueMatt added the blocked on rust-bitcoin Blocked until we update rust-bitcoin/etc label Dec 1, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 15, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (d7f6e34) 90.90% compared to head (2db80b6) 90.93%.

❗ Current head 2db80b6 differs from pull request most recent head e2156c0. Consider uploading reports for the commit e2156c0 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1882      +/-   ##
==========================================
+ Coverage   90.90%   90.93%   +0.02%     
==========================================
  Files         104       91      -13     
  Lines       52838    50519    -2319     
  Branches    52838    50519    -2319     
==========================================
- Hits        48032    45937    -2095     
+ Misses       4806     4582     -224
Impacted Files Coverage Δ
lightning-persister/src/lib.rs 93.45% <ø> (+4.20%) ⬆️
lightning-rapid-gossip-sync/src/lib.rs 91.35% <ø> (+6.22%) ⬆️
lightning/src/lib.rs 100.00% <ø> (+32.25%) ⬆️
lightning/src/ln/channelmanager.rs 86.27% <ø> (-0.83%) ⬇️
lightning/src/ln/functional_test_utils.rs 93.73% <ø> (+1.08%) ⬆️
lightning/src/routing/gossip.rs 92.13% <ø> (+2.35%) ⬆️
lightning/src/routing/router.rs 92.28% <100.00%> (-2.16%) ⬇️

... and 101 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tcharding
Use RUSTFLAGS instead of feature to gate benching
e2156c0 
Currently we use the `"_bench_unstable"` feature to gate benching code.
This results in features not being additive for stable toolchains i.e.,
one cannot currently run `cargo +stable check --all-features`. Although
this method of gating bench code is widely proposed as a good solution
online there is another solution that keeps all features additive.

Guard the bench code with `#[cfg(bench)]` and when running benches use
`RUSTFLAGS='--cfg=bench'` to turn on `bench`.
@tcharding
Copy link
Contributor Author

Rebased and added docs in CONTRIBUTING.md.

@TheBlueMatt
Copy link
Collaborator

Oops I forgot about this PR. Is probably superseded by #2235.

@tcharding
Copy link
Contributor Author

Cool, maybe we should be doing that in rust-bitcoin land as well.

@tcharding tcharding closed this May 15, 2023
@tcharding tcharding deleted the 11-30-cfg-bench branch November 1, 2023 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
blocked on rust-bitcoin Blocked until we update rust-bitcoin/etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tcharding @TheBlueMatt @dunxen @codecov-commenter