Skip to content

Remove unstable feature #482

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
Merged
merged 1 commit into from
Jul 25, 2023
Merged

Remove unstable feature #482

merged 1 commit into from
Jul 25, 2023

Conversation

tcharding
Copy link
Member

Currently it is not possible to run the test suite with cargo test --all-features, this is because we use a feature called unstable to enable the unstable test crate that is used for benchmarking.

There is another way to conditionally enable the test crate and guard the benchmark code. We can use a custom configuration option bench and then set it using RUSTFLAGS='--cfg=bench.

@apoelstra
Copy link
Member

Not sure what to make of this CI failure. It might be a bug in base64 and might be a bug in cargo. Kicked it for now to see if it goes away.

@apoelstra
Copy link
Member

Could you also add a note to the README explaining how to run the benchmarks?

apoelstra
apoelstra previously approved these changes Oct 27, 2022
View reviewed changes
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 0c653bc

@tcharding
Copy link
Member Author

Not sure what to make of this CI failure.

My memory is super vague on this but I kind of remember now that we can't do this PR until we stop using base64-compat which is waiting on rust-bitocincorerpc update to pick up the rust-jsonrpc upgrade - I think.

rust-bitcoin/rust-bitcoincore-rpc#253

@apoelstra
Copy link
Member

Sounds like it's about time we just wrote our own base64 crate :/.

@tcharding
Copy link
Member Author

Sounds like it's about time we just wrote our own base64 crate :/.

Shall we fork https://github.com/marshallpierce/rust-base64 at the version we depend on (0.13) and hack it to suit our project? Or do you think it is worth investing the time to write from scratch?

@tcharding tcharding mentioned this pull request Dec 19, 2022
Closed
@apoelstra
Copy link
Member

Can you kick CI on this one to see if the failure has gone away?

Re forking base64, I think no, I think we should instead try to find a version that works and crev it. If it turns out there's no suitable version, then I guess we'll need to fork ... but let's get our hex story figured out first :)

@tcharding
Copy link
Member Author

The CI fail is because of rust-bitcoincore-rpc using an old version of rust-jsonrpc which depends on bas64-compat. That's why I tried prodding @stevenroose to release a new version :)

@tcharding
Copy link
Member Author

I think we should instead try to find a version that works and crev it.

It works now, IIRC it was because of edition 2018 and Rust 1.29 that it did not work. I'll put base64 at the top of my 'to crev list'.

@tcharding tcharding force-pushed the 10-27-add-bench branch 2 times, most recently from f2c25f3 to d12eb70 Compare May 15, 2023 06:36
@tcharding
Copy link
Member Author

tcharding commented May 15, 2023 •
edited
Loading

This has uncovered a bug in feature gating in test/bench code in rust-bitcoin, will have to fix that first.

@tcharding tcharding marked this pull request as draft May 15, 2023 07:04
@tcharding
Copy link
Member Author

Rebase on master.

@tcharding tcharding marked this pull request as ready for review July 13, 2023 04:10
@tcharding tcharding mentioned this pull request Jul 13, 2023
Closed
@tcharding
Copy link
Member Author

tcharding commented Jul 13, 2023 •
edited
Loading

Back on ice till rust-bitcoin/rust-bitcoin#1936 merges and is released.

EDIT: Fix is in rust-bitcoin v0.30.1

@apoelstra apoelstra mentioned this pull request Jul 16, 2023
Merged
apoelstra added a commit to rust-bitcoin/rust-bitcoin that referenced this pull request Jul 17, 2023
@apoelstra
Merge #1943: Backport #1941 to 0.30.x and release new patch version
4642021 
99ded54 release 0.30.1 (Andrew Poelstra)
b46b61a Use hex_lit::hex in benches (Tobin C. Harding)

Pull request description:

  Needed to unstick rust-bitcoin/rust-miniscript#482

ACKs for top commit:
  tcharding:
    ACK 99ded54
  sanket1729:
    utACK 99ded54

Tree-SHA512: d8eeb3203be8d5823a600f4e8066061f27a2eef576f7e979271bf435c4610ed1861b4416254eb52ed2306df250bebca35cec12e8ecaa3779ca12a7b8ddae3a3c
@tcharding tcharding force-pushed the 10-27-add-bench branch 3 times, most recently from fce9af3 to 8ccc4ae Compare July 25, 2023 07:46
@tcharding
Remove unstable feature
1a33e67 
Currently it is not possible to run the test suite with `cargo test
--all-features`, this is because we use a feature called `unstable` to
enable the unstable `test` crate that is used for benchmarking.

There is another way to conditionally enable the test crate and guard
the benchmark code. We can use a custom configuration option `bench`
and then set it using `RUSTFLAGS='--cfg=bench`.
apoelstra
apoelstra approved these changes Jul 25, 2023
View reviewed changes
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 1a33e67

@apoelstra apoelstra merged commit 1b2d437 into rust-bitcoin:master Jul 25, 2023
@tcharding tcharding deleted the 10-27-add-bench branch July 27, 2023 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tcharding @apoelstra