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

build: add rust-toolchain.toml #4613

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arxanas
Copy link
Collaborator

@arxanas arxanas commented Oct 9, 2024

Follow-up from discussion at https://discord.com/channels/968932220549103686/1288926971719323762

I don't think we achieved consensus in that thread. We could use stable or nightly here instead if we prefer. Note that user rustup overrides are still respected in the presence of a rust-toolchain.toml.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

Follow-up from discussion at https://discord.com/channels/968932220549103686/1288926971719323762

I don't think we achieved consensus in that thread. We could use `stable` or `nightly` here instead if we prefer. Note that user `rustup` overrides are still respected in the presence of a `rust-toolchain.toml`.
@yuja
Copy link
Collaborator

yuja commented Oct 9, 2024

For reference, there's a previous attempt: #1913

(I personally don't like rust-toolchain.toml because it forces rustup to install the specified toolchain if not exist.)

@arxanas
Copy link
Collaborator Author

arxanas commented Oct 9, 2024

One reason in favor of rust-toolchain.toml (failed to mention it in the commit message) is that otherwise you might have to reconfigure the version of Rust to use for every new worktree.

Besides worktrees, I often use different machines, including ephemeral cloud machines that don't have my previous overrides configured. It's annoying to have to manually configure the Rust version every time I freshly check out the repository. I'm open to other solutions.

(Of course, if you specifically do want to use the default Rust toolchain for every worktree, then this gets in the way of that. Maybe setting stable as the version helps to avoid forcing a download unnecessarily?)

@ilyagr
Copy link
Collaborator

ilyagr commented Oct 10, 2024

If we go with this, could you add an explanation somewhere of how to override it? Update: More generally, I think a few updates to contributing.md would have to go with it, e.g. https://martinvonz.github.io/jj/prerelease/contributing/#summary.

Is there a way to make cargo fmt to default to +nightly? (That would be good independently of what happens to the rest of this PR, but is not blocking, as this is already a problem with the status quo)

It's annoying to have to manually configure the Rust version every time I freshly check out the repository. I'm open to other solutions.

I'd just do cargo +nightly (or stable, but you still need nightly for cargo fmt), cargo +1.76, rustup default nightly, or rustup default 1.76.

I wish there was a way to define a +ci version for the project in rust-version.toml instead of making it the default (so that people don't have to look up the 1.76). I'm guessing this is not possible?

@arxanas
Copy link
Collaborator Author

arxanas commented Oct 10, 2024

If we go with this, could you add an explanation somewhere of how to override it? Update: More generally, I think a few updates to contributing.md would have to go with it, e.g. https://martinvonz.github.io/jj/prerelease/contributing/#summary.

👍

Looking at the docs now, I think adding rust-toolchain.toml simplifies the general instructions slightly.

I wish there was a way to define a +ci version for the project in rust-version.toml instead of making it the default (so that people don't have to look up the 1.76). I'm guessing this is not possible?

I think this would be ideal but I couldn't find a way to do it.

I'd just do cargo +nightly (or stable, but you still need nightly for cargo fmt), cargo +1.76, rustup default nightly, or rustup default 1.76.

Hmm, but then I'd either download new nightly toolchains as they come out, or I'd have to update all invocations of Rust commands with the new version.

  • [assessment: annoying] Having to update all the possible call-sites seems particularly annoying/difficult (as I'm sure I have many aliases, plus any IDE support needs to be updated).
  • [assessment: less annoying] Downloading the new toolchain whenever it's released is maybe less bad, but I'd still prefer to download it only once when I start working on the project on a given machine.
    • [assessment: not a serious concern] If we wanted to support Codespaces/etc., it might be easier in practice to use a static Rust toolchain version to boostrap the image, rather than dynamically provision the image with nightly or defer the download to development-time.

@ilyagr
Copy link
Collaborator

ilyagr commented Oct 10, 2024

I wish there was a way to define a +ci version for the project in rust-version.toml instead of making it the default (so that people don't have to look up the 1.76). I'm guessing this is not possible?

I think this would be ideal but I couldn't find a way to do it.

I think you are right, this feature doesn't seem to exist.

A couple of links to relevant issues. It doesn't look like either of them is solved or about to be solved, which is a pity, it would have helped a lot.

Toolchain aliases & supporting multiple toolchains in rust-toolchain.toml: rust-lang/rustup#3546 (comment)

Supporting MSRV specifically: rust-lang/rustup#1484


Reference link to the current docs (I imagine many of us already were looking at it, but I keep losing the link): https://rust-lang.github.io/rustup/overrides.html#the-toolchain-file

@arxanas arxanas marked this pull request as draft October 14, 2024 16:55
@arxanas
Copy link
Collaborator Author

arxanas commented Oct 14, 2024

Need to update docs

@yuja yuja mentioned this pull request Nov 1, 2024
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