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

doc: Recommend tracking Cargo.lock with rust-toolchain files #3054

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

MoSal
Copy link
Contributor

@MoSal MoSal commented Aug 2, 2022

Current use-case where not doing this causes issues:

cargo new ttt
cd ttt
sed -i '/edition/s;2021;2018;' Cargo.toml
echo 'criterion = "0.3"' >> Cargo.toml
echo '1.55' > rust-toolchain
cargo check

Issue triggered by an indirect dependency requiring a newer edition
than what the pinned toolchain supports.

@@ -111,7 +111,8 @@ that uses the new TOML encoding in the `rust-toolchain` file. You need to upgrad
`rustup` to 1.23.0+.

The `rust-toolchain.toml`/`rust-toolchain` files are suitable to check in to
source control.
source control. If that's done, `Cargo.lock` should probably be tracked too to
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it?

Arguments for/against Cargo.lock for libraries apply also to rust-toolchain, no?

The purpose of a Cargo.lock lockfile is to describe the state of the world at the time of a successful build. Cargo uses the lockfile to provide deterministic builds on different times and different systems, by ensuring that the exact same dependencies and versions are used as when the Cargo.lock file was originally generated.

And the toolchain used is a part of the state of the world. So this, in my mind, reinforces the tie between both files.

Users dependent on the library will not inspect the library’s Cargo.lock (even if it exists).

Same can be said about rust-toolchain, no?

In other words, libraries specify SemVer requirements for their dependencies but cannot see the full picture. Only end products like binaries have a full picture to decide what versions of dependencies should be used.

.. and what toolchain version should be used?


So, one can argue that, ideally, libraries shouldn't track either file. But, and irregardless of crate type, tracking one of them files signals a desire to be able to re-create the state of the world in the future easily. And to truly be able to re-create the state of the world, you need both files!

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that we should not recommend in this book about the way Cargo.lock is handled. It should follow the rules of the documentation I posted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its true that for users of a library crate, the toolchain files interactions with dependencies selected by cargo are irrelevant. But I also think its true that for the given scenario - a version fixed toolchain file, then developer of a library crate, doing a clean checkout will often break because dependencies have moved on but rust/cargo are pinned at an ancient version. So documenting that friction and giving people pragmatic advice - 'if you use a concrete version locked toolchain file then also versioning Cargo lock' seems wise.

@@ -111,7 +111,8 @@ that uses the new TOML encoding in the `rust-toolchain` file. You need to upgrad
`rustup` to 1.23.0+.

The `rust-toolchain.toml`/`rust-toolchain` files are suitable to check in to
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what we mean by 'suitable to check into source control' here.

We probably need to do a bit of clarification / nuance around rust-toolchain files, since when used without care a bunch of friction - like this PR talks about - turns up.

I've taken to recommending folk don't use rust-toolchain in fact because of the number of touch points - things like automatically installing a toolchain in a docker build which errors (because we don't fall back to copy, which is a choice I still support), or the fact that putting a symbolic toolchain like 'stable' in a rust-toolchain file doesn't provide any sort of state-of-the-world benefit. Cargo's rust-version header is IMO sufficient and better for most use cases.

That said, it is a feature we support, and it is true that a toolchain file that pins to a specific concrete version will cause friction for developers when dependencies have an MSRV above the rust-toolchains version.

I think documenting the friction is a decent starting point, though I do wonder if we should go further and have cargo use a fixed toolchain as a constraint in dependency updates (checking the msrv in the dependencies Cargo.toml).

Thats a separate conversation though.

doc/src/overrides.md Outdated Show resolved Hide resolved
 .. if pinning to a specific version.

 Current use-case where not doing this causes issues:

 ```
 cargo new ttt
 cd ttt
 sed -i '/edition/s;2021;2018;' Cargo.toml
 echo 'criterion = "0.3"' >> Cargo.toml
 echo '1.55' > rust-toolchain
 cargo check
 ```

 Issue triggered by an indirect dependency requiring a newer edition
 than what the pinned toolchain supports.

Signed-off-by: Mohammad AlSaleh <[email protected]>
Copy link
Contributor

@rbtcollins rbtcollins left a comment

Choose a reason for hiding this comment

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

We may end up with more questions from this, but lets find out.

@rbtcollins rbtcollins merged commit 060b8f9 into rust-lang:master Jan 17, 2023
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