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

devguide: recommend using rustup to install rust - v1 #9612

Closed
wants to merge 1 commit into from

Conversation

jufajardini
Copy link
Contributor

During Outreachy onboarding, we noticed that we weren't referring to rust installation via rustup, in our devguide.

Describe changes:

  • remove indication to install rust and cargo via apt
  • add instructions to install rust via rustup, and reference to rust install docs
  • tiny style reword

We were still instructing that rust and cargo were installed via apt.

Update the instructions to follow what is recommended by Rust community
themselves.
@jufajardini jufajardini added the typo/doc update No code change : only doc or typo fixes label Oct 13, 2023
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Merging #9612 (ac29aea) into master (1a132f4) will decrease coverage by 0.03%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9612      +/-   ##
==========================================
- Coverage   82.28%   82.26%   -0.03%     
==========================================
  Files         968      968              
  Lines      274275   274275              
==========================================
- Hits       225700   225640      -60     
- Misses      48575    48635      +60     
Flag Coverage Δ
fuzzcorpus 64.38% <ø> (+0.04%) ⬆️
suricata-verify 60.90% <ø> (-0.03%) ⬇️
unittests 62.87% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@victorjulien
Copy link
Member

I disagree with this change. We should use the distro packages unless there is a reason they are insufficient.

@jufajardini
Copy link
Contributor Author

I disagree with this change. We should use the distro packages unless there is a reason they are insufficient.

Invoking @jasonish in case he has strong opinions on this '-'

@jasonish
Copy link
Member

I disagree with this change. We should use the distro packages unless there is a reason they are insufficient.

Target audience. For end users installing from git, we should use the packages.

For developers installing from git, rustup is probably the best. Its the only way to install/test different versions of Rust, which could be a reason for CI failure, etc. This is in the developers guide, not the user install guide.

@victorjulien
Copy link
Member

I disagree with this change. We should use the distro packages unless there is a reason they are insufficient.

Target audience. For end users installing from git, we should use the packages.

For developers installing from git, rustup is probably the best. Its the only way to install/test different versions of Rust, which could be a reason for CI failure, etc. This is in the developers guide, not the user install guide.

I disagree. We also don't recommend installing your own C compiler or specific versions of libc. Rust packaging has come along far enough that we can use it in development. I think it's fine to have a section to explain benefits of using a more recent version, but not claim it's needed or recommended.

@jasonish
Copy link
Member

I disagree with this change. We should use the distro packages unless there is a reason they are insufficient.

Target audience. For end users installing from git, we should use the packages.
For developers installing from git, rustup is probably the best. Its the only way to install/test different versions of Rust, which could be a reason for CI failure, etc. This is in the developers guide, not the user install guide.

I disagree. We also don't recommend installing your own C compiler or specific versions of libc. Rust packaging has come along far enough that we can use it in development. I think it's fine to have a section to explain benefits of using a more recent version, but not claim it's needed or recommended.

Its not there yet. Want clippy, rustup component add clippy, same with rust-analyzer: rustup component add rust-analyzer.

Likewise, we're likely to request changes on a commit that doesn't pass the lints in CI, which might require a user to change to a specific version of rust. Trivial with rustup. None of this is about using a more recent version.

@jufajardini jufajardini added the decision-required Waiting on deliberation from the team label Nov 30, 2023
@jufajardini
Copy link
Contributor Author

Closing as we haven't agreed upon the best approach, here.

@jufajardini jufajardini closed this Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decision-required Waiting on deliberation from the team typo/doc update No code change : only doc or typo fixes
Development

Successfully merging this pull request may close these issues.

None yet

3 participants