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

Add rust-version to Cargo configuration #5530

Merged
merged 3 commits into from
Dec 2, 2021
Merged

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Nov 29, 2021

Add package.rust-version to all Cargo.toml files so that users don’t
get mysterious compiler failures when trying to build the project with
unsupported compilers.

Issue: #5452

Add `package.rust-version` to all `Cargo.toml` files so that users don’t
get mysterious compiler failures when trying to build the project with
unsupported compilers.

Issue: near#5452
@mina86 mina86 changed the title Verify that rustc is at least the version from rust-toolchain.toml Add rust-version to Cargo configuration Nov 30, 2021
@mina86 mina86 requested review from matklad and nagisa November 30, 2021 12:17
@mina86 mina86 marked this pull request as ready for review November 30, 2021 12:17
@matklad
Copy link
Contributor

matklad commented Nov 30, 2021

Example error

λ make release
cargo build -p neard --release
error: package `near-chunks-primitives v0.0.0 (/home/matklad/projects/nearcore/chain/chunks-primitives)` cannot be built because it requires rustc 1.59.0 or newer, while the currently active rustc version is 1.56.0
make: *** [Makefile:22: release] Error 101

Not a huge fan of "remember to upgrade this config in ten different places" workflows, as most likely we won't be able to keep this consistent. But it's worth a try!

@matklad
Copy link
Contributor

matklad commented Nov 30, 2021

(one alternative is to pin this only for, eg nearcore/Cargo.toml, which should make, eg make release behave)

@nagisa
Copy link
Collaborator

nagisa commented Nov 30, 2021

I'm of a similar opinion as @matklad. Having rust-version specified for each crate would make most sense if we maintained and tested each crate separately. Since this is a monorepo-ish setup, specifying rust-version for just the most commonly depended on crate within the workspace is going to be easier to work with.

@miraclx
Copy link
Contributor

miraclx commented Nov 30, 2021

Of the most commonly depended on, I think the best candidate would be near-primitives

@matklad
Copy link
Contributor

matklad commented Nov 30, 2021

To clarify, given that we do publish things to crates IO (without much semver guarantees), I thing there might be some merit in just specifying this for everything.

@mina86
Copy link
Contributor Author

mina86 commented Dec 1, 2021

Looks like opinions are split regarding whether this should be just in near-primitives or everywhere. I’m gonna mark it automerge in the current form (with rust-version added to all Cargo.toml). If anyone has strong opinions remove the label and discuss it.

@pmnoxx
Copy link
Contributor

pmnoxx commented Dec 1, 2021

As with every decision, there are benefits and drawbacks:

Benefits:

  • external developers who use our packages, will know what we require new version, making it less confusing
  • next time we upgrade Rust version, every code owner will get informed, we will be able to check what new features the next version offers, and decide whenever we should use any of them
  • we already put version inside each crate, whenever we bump up version everyone already gets notified. So, as long as we do the version update, at the same time, as rust_version update, we will not be adding any more extra work, than we already are doing.

Drawback:

  • Every code owners needs to accept the change. However, we already do that with every version bump. So, we are not really making the situation worse than it already is.
    It's a good idea, to always bump the version when rust_version changes.

I think we should do it.

@bowenwang1996 bowenwang1996 merged commit e72782f into near:master Dec 2, 2021
@mina86 mina86 deleted the version branch December 8, 2021 00:26
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.

10 participants