near / nearcore

Reference client for NEAR Protocol
https://near.org
GNU General Public License v3.0
2.31k stars 614 forks source link

Check that `rust-version` in Cargo.tomls is consistent with what we declare in `rust-toolchain.toml` #6124

Closed matklad closed 1 year ago

matklad commented 2 years ago

The check should go here:

https://github.com/near/nearcore/blob/6d86ccd4fee74c1567dc2d83d384b1952826e796/tools/themis/src/rules.rs#L28-L48

See https://github.com/near/nearcore/pull/5834/files#r787711401 for context.

matklad commented 2 years ago

(as an alternative, we can also just remove rust-version from Cargo.toml. I am still not sure if, on ballance, the field is worth it. #5834 shifted my opinion more in the "not worth it" direction, but that could have played differently if we did have the CI check)

matklad commented 2 years ago

Hm, I think what we actually should do here is

@miraclx WDYT?

miraclx commented 2 years ago

I'm not sure if this is an optimal solution. rust-version specifies MSRV for a crate. If we decide to keep a consistent version for all crates in the workspace, there would undoubtedly be crates that run just fine on lower versions, but this constraint would prohibit their use. Basically, I think, we should aim for maximum compatibility where we can and not create artificial and unnecessary constraints.

As currently defined, the rust-version spec and compliance checks we have allows each crate to have its own minimum rust version spec, which can be anything less than or equal to the version in rust-toolchain.

I'd advise we only bump the MSRV on a crate by crate basis, based on utility. Whatever runs just fine on a prior rust version doesn't need to be updated.

However, the rust-toolchain version can and should be whatever the latest rust version is.

matklad commented 2 years ago

There's no way for us to understand when we we need to upgrade rust-version. We only run CI with what's specified in rust-toolchain.toml, and only that version is guaranteed to work.

We can add MSRV CI check for every crate, but I think this is clearly unreasonable -- nearcore crates do not generally promise any kinds of stability or compatibility.

But year, select crates which do want to make extra promises, like near-account-id can manually specify lower rust-version in Cargo.toml, provided that there's a CI check for that as well.

pmnoxx commented 2 years ago

LGTM as-is.

See my comment in #6124

I'm not sure if this is an optimal solution. rust-version specifies MSRV for a crate. If we decide to keep a consistent version for all crates in the workspace, there would undoubtedly be crates that run just fine on lower versions, but this constraint would prohibit their use. As currently defined, the rust-version spec and compliance checks we have allows each crate to have its own minimum rust version spec, which can be anything less than or equal to the version in rust-toolchain. I'd advise we only bump the MSRV on a crate by crate basis, based on utility. Whatever runs just fine on a prior rust version doesn't need to be updated. However, the rust-toolchain version can and should be whatever the latest rust version is.

We currently, don't verify whenever a crate will work with rust-version, which is specified.

I see a few choices:

Questions:

miraclx commented 2 years ago

My vote is for option 2. A simple cargo +<msrv> check -p <crate> for each crate. cargo-msrv verify is well suited to help with this.

Even if we don't adopt this for all the crates, we should at least for the non-private ones.

pmnoxx commented 2 years ago

My vote is for option 2. A simple cargo +<msrv> check -p <crate> for each crate. cargo-msrv version is well suited to help with this.

Even if we don't adopt this for all the crates, we should at least for the non-private ones.

Option 2 adds extra maintenance cost. For, example, if I start using by accident new syntax in near-primitives. Then I may find out, half an hour later, after CI runs, that some crates need to have their minimum version bumped up.

The question is, what is the upside of it? I see that some external developed, may be able to use some of packages, with lower version of rust stable than the current version. However, someone we will bump the version up, so some external developer may be required to bump up the version.

I see this as a trade-off, this adds extra maintenance cost, to support a feature, that I don't see anyone actually requested.

If we want to improve work for external developers, while not adding maintenance cost. We could for example, decide, that we will use one version, but a version that's latest -1, or latest -2, to give people change to upgrade. I'm not sure if it's worth either.

pmnoxx commented 2 years ago

My vote is for option 3. a) use the latest version everywhere. b) lag behind, maybe 3-6 month, that is 1-2 version. I'm not sure if that's worth it, though.

This way, we don't add extra maintenance cost. Usually, the benefits of new Rust features, doesn't outweigh the extra added maintenance cost of having each crate with their own Rust version. I suggest to use a single version, whenever it's the latest one, or one that's 3-6 month behinds, is fine.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.