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.

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

Updated examples and README.md #678

Closed
wants to merge 0 commits into from
Closed

Updated examples and README.md #678

wants to merge 0 commits into from

Conversation

i5hi
Copy link

@i5hi i5hi commented Oct 12, 2021

  • Added examples/psbt.rs

  • Updated error message for examples/bip32.rs.

The previous error would print the entire path as follows:

not enough arguments. usage: /home/ishi/rust-bitcoin/target/debug/examples/bip32 <WIF>
  • Updated README.md with instructions to build and run examples.

sanket1729
sanket1729 reviewed Oct 13, 2021
View reviewed changes
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

Concept ACK. There are some MSRV errors

tcharding
tcharding reviewed Oct 13, 2021
View reviewed changes
examples/psbt.rs Outdated Show resolved Hide resolved
examples/psbt.rs Outdated Show resolved Hide resolved
examples/psbt.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated
@@ -50,10 +50,14 @@ secp256k1 = { version = "0.20.0", features = [ "recovery", "rand-std" ] }
bincode = "1.3.1"
# We need to pin ryu (transitive dep from serde_json) to stay compatible with Rust 1.22.0
ryu = "<1.0.5"
base64-compat = "1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this here? I don't see it used anywhere in the PR?

Copy link
Author

Choose a reason for hiding this comment

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

The examples/psbt.rs file needed it to use base64::decode.

Copy link
Member

Choose a reason for hiding this comment

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

Oh nice, I didn't click that base64-compat had the lib name set to base64. Thanks

tcharding
tcharding previously approved these changes Oct 13, 2021
View reviewed changes
Cargo.toml Outdated
@@ -50,10 +50,14 @@ secp256k1 = { version = "0.20.0", features = [ "recovery", "rand-std" ] }
bincode = "1.3.1"
# We need to pin ryu (transitive dep from serde_json) to stay compatible with Rust 1.22.0
ryu = "<1.0.5"
base64-compat = "1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Oh nice, I didn't click that base64-compat had the lib name set to base64. Thanks

@i5hi
Copy link
Author

i5hi commented Oct 22, 2021 •
edited

@sanket1729 are we still getting MSRV errors? The tests are passing for me locally.

@i5hi
Copy link
Author

i5hi commented Oct 22, 2021

damn. looking into it.

any idea what I need, to be able to catch this error when I run contrib/test.sh locally?

@i5hi
Copy link
Author

i5hi commented Oct 22, 2021

@tcharding I hadn't noticed base64 as a feature. Enabling it under [[example]] in Cargo.toml was enough without adding the dev-dependency.

@i5hi
Copy link
Author

i5hi commented Oct 26, 2021 •
edited

@sanket1729 Sorry this required so many CI runs.

After some digging I think adding extern crate base64 should resolve this issue with rustc 1.29.0

I need some help on downgrading rustc to 1.29.0 to be able to run these tests locally and ensure backwards compatability. Been at it for a while now and been getting an editions are unstable error and not able to get past it. Will have a go at it again tomorrow.

Addition:

I will also document this in the README once I figure it out so anyone making PR's in the future can easily test with v1.29.0 and prevent having to rely on CI for it. Its a little more complicated than it seems. The issue I'm having was reported here: rust-lang/cargo#6113; the solutions suggested do not work for us.

@i5hi i5hi requested a review from sanket1729 October 26, 2021 18:12
@mcroad
Copy link
Contributor

mcroad commented Nov 5, 2021

Having the same issue with rust-bitcoin/rust-miniscript#277.

To downgrade to 1.29 run rustup default 1.29.0 and rustc --version to confirm.

@i5hi
Copy link
Author

i5hi commented Nov 7, 2021

@mcroad I did that. But that leads to a bunch of other errors.
Does just that command alone work for you to be able to run contrib/test.sh?

@i5hi
Copy link
Author

i5hi commented Nov 7, 2021

@mcroad Just saw your issue on rust-miniscript. I get the same error here but with the cc crate. Will try out a few things tomorrow and get back.

@mcroad
Copy link
Contributor

mcroad commented Nov 8, 2021

@i5hi make sure to pin the cc dependency as described in the readme https://github.com/rust-bitcoin/rust-bitcoin#minimum-supported-rust-version-msrv

@i5hi
Copy link
Author

i5hi commented Nov 8, 2021

Aww man :/ Thats awkward :P Last time I attemtped this, I did run those pin commands and still got errors.

Noticed that if I ran rustup default 1.29.0 then the pin commands and then rustup default 1.29.0 again, it would error. I suppose rustup default must ONLY be run before pinning, running it both before and after (or just after) seems to reset the pin commands.

@i5hi
Copy link
Author

i5hi commented Nov 8, 2021

All good now. Cheers @mcroad!

@i5hi
Copy link
Author

i5hi commented Nov 10, 2021 •
edited

Final changes to this PR are complete.

  • psbt example works and is compatible with MSRV
  • updated README.md with a few more pointers on testing changes* with MSRV
  • added psbt example to contrib/test.sh

*edits

@i5hi i5hi force-pushed the master branch 2 times, most recently from 94aee4e to d4fe1fe Compare November 10, 2021 07:00
@i5hi
Copy link
Author

i5hi commented Nov 13, 2021

Added --features base64 to the new psbt example in contrib/test.sh which was failing the last CI run.

dr-orlovsky
dr-orlovsky suggested changes Nov 14, 2021
View reviewed changes
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Thank you and congrats with a first-time contribution!

I left few code nits and changes to README. Unfortunately you need to rebase on top of master and remove merge commit - we do not accept from-master merges in the history. Also, it is required that each of the commits compile successfully. Sorry for inconvenience, but this is a maintainer requirement applied to all PRs in this repo.

README.md Outdated
@@ -61,6 +61,10 @@ please join us in

This library should always compile with any combination of features on **Rust 1.29**.

```
rustup default 1.29.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure whether its good to ask user to change the default rust version - especially with the fact that it will compile with any version above that

Copy link
Author

Choose a reason for hiding this comment

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

I added this to help a contributor test with MSRV. Maybe add a note saying "To test with MSRV..." ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For testing I would prefer something like rustup install 1.29.0 && cargo +1.29.0 build, which will not switch the global rust environment

Copy link
Author

Choose a reason for hiding this comment

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

Right, just default subcommand also assumes 1.29.0 is already installed.

README.md Outdated
@@ -79,6 +83,8 @@ For the feature `base64` to work with 1.29.0 we also need to pin `byteorder`:
cargo update -p byteorder --precise "1.3.4"
```

*If you face a `Cargo.lock` parsing error, remove it before running the above commands.*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or do cargo generate-lockfile

examples/psbt.rs Outdated Show resolved Hide resolved
examples/psbt.rs Outdated Show resolved Hide resolved
@i5hi
Copy link
Author

i5hi commented Nov 14, 2021

Thanks for the support @dr-orlovsky

Will fix this up to meet all the mentioned requirements and also address the nits you've mentioned.

@tcharding
Copy link
Member

Hi @i5hi, are you working on this PR still mate?

@i5hi
Copy link
Author

i5hi commented Oct 30, 2022

Hey. Yes.

I will re-review, update and try to close it by the end of November.

@tcharding
Copy link
Member

Awesome, thanks man!

@i5hi i5hi closed this Nov 14, 2022
@i5hi
Copy link
Author

i5hi commented Nov 14, 2022

oh snap, i broke it while trying to sync up.

Will make a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dr-orlovsky dr-orlovsky dr-orlovsky requested changes

@tcharding tcharding tcharding left review comments

@sanket1729 sanket1729 Awaiting requested review from sanket1729

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@i5hi @mcroad @tcharding @dr-orlovsky @sanket1729