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

update README.md on how to install toolchain and components #6294

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

goetz-markgraf
Copy link

Hello community. This is my first PR, so please bear with me if I do something wrong. I will happily adjust the PR if needed.

I have tried to build rustfmt. Since I did not have the required toolchain and components installed, the build crashed. Thanks to help on discord, I was able to get running.

In this PR, I have added the needed commands to install the toolchain and the components to README.md. This will help newcomers like me, I think.

@ytmimi
Copy link
Contributor

ytmimi commented Aug 26, 2024

Thanks for working on the docs! I'm guessing cargo didn't automatically install the toolchain and components listed in the rust-toolchain file when you tried to build? Whenever we bump the toolchain, cargo automatically installs everything the next time I run cargo build. I wonder why it didn't do that for you 🤔

@goetz-markgraf
Copy link
Author

I don't know. I have not found any setting. But for those, this addition to the readme could help.
If someone knows the settings or config, I'll be very happy to learn.

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

I've left a few suggestions on the docs you wrote. I think linking out to the docs on toolchains and components could be useful for users who might not be too familiar with those terms. Also, I'd prefer not to list a specific nightly version in the docs and would much rather link out to the rust-toolchain file.

README.md Outdated
Comment on lines 161 to 165
```
[toolchain]
channel = "nightly-2024-08-17"
components = ["llvm-tools", "rustc-dev"]
```
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer not to list the content of the rust-toolchain file inline. Instead, I'd rather linked out to the file [`rust-toolchain`](https://github.com/rust-lang/rustfmt/blob/master/rust-toolchain), and let users know that they need to install the exact nightly version of the compiler that rustfmt has pinned in its rust-toolchain file.

README.md Outdated
Comment on lines 154 to 155
All command should be run in the project's root directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct. You should be able to install the nightly toolchain and components from anywhere.

Suggested change
All command should be run in the project's root directory.

Copy link
Author

Choose a reason for hiding this comment

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

I have completly re-written the section.

README.md Outdated
Comment on lines 158 to 159
Make sure to have installed the correct toolchain and all components.
The needed parts can be found in the file `rust-toolchain`. For example:
Copy link
Contributor

@ytmimi ytmimi Aug 28, 2024

Choose a reason for hiding this comment

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

Suggested change
Make sure to have installed the correct toolchain and all components.
The needed parts can be found in the file `rust-toolchain`. For example:
rustfmt depends on compiler internals and therefore pins a specific version of the nightly compiler.
Make sure you've installed the correct nightly [toolchain](https://rust-lang.github.io/rustup/concepts/toolchains.html) and all [components](https://rust-lang.github.io/rustup/concepts/components.html) listed in rustfmt's [`rust-toolchain` file](https://github.com/rust-lang/rustfmt/blob/master/rust-toolchain).

It's probably worth mentioning this information before telling users to build rustfmt with cargo build or mention that they should follow these steps if they're having issues building.

README.md Outdated
Comment on lines 169 to 171
```
rustup install nightly-2024-08-17
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets point users to the rust-toolchain file to get the exact date:

Suggested change
```
rustup install nightly-2024-08-17
```
```
rustup install nightly-YYYY-MM-DD
```

@goetz-markgraf
Copy link
Author

Great Idea, I will update my PR, probably tomorrow. THX for your ideas

@ytmimi
Copy link
Contributor

ytmimi commented Aug 31, 2024

@goetz-markgraf Thanks for making some changes. I actually preferred your first draft better than what you changed it to. Would you be able to take another go at this and apply some of my suggestions to your first draft?

Also, we strongly discourage merge commits. You should be able to rebase your branch to bring it up to date.

@goetz-markgraf
Copy link
Author

The first draft show a way to install toolchain and components, but for rustfmt this way is wrong, as I have learnd.
Since there is a rust-toolchain-file, one should use it and not fiddle with the installation.

My problem was, that I had started nvim directly after cloning the repo. nvim in turn started the LSP rust-analyzer that tried to install the toolchain. That was only party successful. And then I was stuck with a corrupt toolchain.

I have documented what you should do, when you get a corrupt toolchain. And that, to me, is better than trying to patch up the broken installation.

As for you comments, I tried my best to apply then. I have not used any literal name of toolchain but rather your suggestion. And I have added a link to the documentation of rustup that was key to learn about the correct way.

So, I'd rather stick with the second approach.

@ytmimi
Copy link
Contributor

ytmimi commented Aug 31, 2024

The first draft show a way to install toolchain and components, but for rustfmt this way is wrong, as I have learnd.
Since there is a rust-toolchain-file, one should use it and not fiddle with the installation.

What's wrong with installing components by hand? Also, where did you learn this was wrong?

@goetz-markgraf
Copy link
Author

goetz-markgraf commented Aug 31, 2024

Well. All my previous efforts did not resolve the root cause, the corrupt toolchain. In an attempt to solve the compile error I had overridden the toolchain with rustup override set nightly and so the rust-toolchain file was completely ignored.

The new approach leads to the toolchain correctly recognizing the rust-toolchain file as intended by the authors of the repo. So I think, setting up the configuration such that rustup and cargo can do their thing instead of fiddeling manually with the toolchain is the right approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants