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.

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

move to rust 2021, macro tweak to compile #650

Closed
wants to merge 2 commits into from
Closed

Conversation

nuke-web3
Copy link

@nuke-web3 nuke-web3 commented Nov 14, 2021

Confirmed to build and pass tests locally on Ubuntu 20.04.3 LTS:

rustc 1.56.1 (59eed8a2a 2021-11-01)
rustc 1.58.0-nightly (e90c5fbbc 2021-11-12)

@nuke-web3 nuke-web3 changed the title move to rust 2021 move to rust 2021, macro tweak to compile Nov 14, 2021
@@ -78,7 +78,7 @@ pub fn generate_client_module(
#(#client_methods)*
}

impl#generics From<RpcChannel> for Client#generics
impl #generics From<RpcChannel> for Client #generics

Choose a reason for hiding this comment

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

What would happen if you did not make these whitespace changes?

Copy link
Member

Choose a reason for hiding this comment

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

rust-version = "1.56.1"
style for [package] field
@nuke-web3
Copy link
Author

rust-version = "1.56.1" introduced

For reference, I tried to get this to cargo build with 1.56.0 and it failed with

error: package `...` cannot be built because it requires rustc 1.56.1 or newer, while the currently active rustc version is 1.56.0

With --ignore-rust-version it does build with 1.56.0, as expected.

@nuke-web3 nuke-web3 requested review from KiChjang and bkchr and removed request for KiChjang November 21, 2021 05:05
@nuke-web3 nuke-web3 requested review from bkchr and tomusdrw and removed request for tomusdrw and bkchr November 21, 2021 06:49
@@ -1,14 +1,15 @@
[package]
name = "jsonrpc-client-transports"
version = "18.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably bump the major versions everywhere due to different compiler requirements.

Copy link
Author

Choose a reason for hiding this comment

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

do you mean minor or patch? 18.1.0 makes sense to me, as its compatible with 18.0.0
maybe even just 18.0.1 as this doesn't effect the behavior of the program

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually really meant major. The thing is that we now suddenly require everyone to use a specific Rust compiler version which IMHO is a breaking change. But I'm not a semver expert, maybe @dvdplm has some suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

MSRV changes are generally considered breaking. v19! :)

Copy link
Author

Choose a reason for hiding this comment

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

Oh well, I can do that here, sure! But I know that many do not do this (I put in a lot of PRs across parity for this, and it so far has not been)
Happy to update across the board and reference this thread to justify.

Do note that it's not universally agreed that MSVR bump = breaking change.

@nuke-web3 nuke-web3 closed this Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants