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

*: Migrate to Rust 2021 edition #2339

Merged
merged 8 commits into from
Nov 26, 2021
Merged

*: Migrate to Rust 2021 edition #2339

merged 8 commits into from
Nov 26, 2021

Conversation

nuke-web3
Copy link
Contributor

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

Confirmed to build locally on Ubuntu 20.04.3 LTS:

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

@thomaseizinger thomaseizinger changed the title move to rust 2021 *: Migrate to Rust 2021 edition Nov 14, 2021
@mxinden
Copy link
Member

mxinden commented Nov 15, 2021

Thanks @nukemandan for the upgrade work!

The most important rule for editions is that crates in one edition can interoperate seamlessly with crates compiled in other editions. This ensures that the decision to migrate to a newer edition is a "private one" that the crate can make without affecting others.

https://blog.rust-lang.org/2021/05/11/edition-2021.html

If I am not mistaken there is no downside for rust-libp2p to upgrade to edition 2021. Anyone objecting?

Not sure what the best practice is, but I would still consider an upgrade to 2021 to be a breaking change. Thus this pull request would need to add a changelog entry to each crate's CHANGELOG.md and bump the version if not already done (look for [unreleased] in CHANGELOG.md). See release docs for details.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Nov 17, 2021

If I am not mistaken there is no downside for rust-libp2p to upgrade to edition 2021. Anyone objecting?

AFAIK it will bump the MSRV to 1.56. Are we okay with that?

@nuke-web3
Copy link
Contributor Author

It is also strongly suggested that we included a minimal rust version to use, specifically 1.56.1

Cargo.toml should include:

rust-version = "1.56.1"

https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field

it cannot include semver operators or pre-release identifiers.
The rust-version must be equal to or newer than the version that first introduced the configured edition.
The rust-version may be ignored using the --ignore-rust-version option.

More discussion:

@mxinden
Copy link
Member

mxinden commented Nov 19, 2021

AFAIK it will bump the MSRV to 1.56. Are we okay with that?

I am fine with it. Anyone out there objecting?

It is also strongly suggested that we included a minimal rust version to use, specifically 1.56.1

What is the benefit of explicitly setting the version? Isn't it already implicitly set by moving to Rust 2021 @nukemandan?

@nuke-web3
Copy link
Contributor Author

nuke-web3 commented Nov 20, 2021

I have not played with behavior for different versions with and without this included, but I believe it enforces builds with no less than this version, and in this case the 1.56.1 (with the security patch) will be required to build, without specifying a flag to ignore it.
This will theoretically avoid issues that using older versions may introduce. But to be honest, it's not clear (I have not done a lot of research) if this is worth enforcing. My intuition is to add it.

Uniform [package] fields style
@nuke-web3
Copy link
Contributor Author

nuke-web3 commented Nov 21, 2021

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.

Should be good to go with a squash merge to make the commit log clean on master IMHO 👍

@mxinden
Copy link
Member

mxinden commented Nov 23, 2021

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.

Should be good to go with a squash merge to make the commit log clean on master IMHO +1

Thanks for testing. I understand that rust-version enforces usage of > VERSION in downstream dependencies, though I wonder whether we need to set it explicitly, or whether the implicit version restriction through edition = "2021" isn't enough?

@thomaseizinger
Copy link
Contributor

Thanks for testing. I understand that rust-version enforces usage of > VERSION in downstream dependencies, though I wonder whether we need to set it explicitly, or whether the implicit version restriction through edition = "2021" isn't enough?

I think it should give a much better error message? At least for version 1.54 on-wards (where rust-version stabilized). Not a huge benefit I guess?

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks @nukemandan for the help!

@mxinden mxinden merged commit a7ed1d6 into libp2p:master Nov 26, 2021
vnermolaev pushed a commit to vnermolaev/rust-libp2p that referenced this pull request Nov 30, 2021
Co-authored-by: Max Inden <mail@max-inden.de>
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.

3 participants