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

Include rustfmt and clippy components by default #37

Closed
tyranron opened this issue Dec 7, 2018 · 12 comments
Closed

Include rustfmt and clippy components by default #37

tyranron opened this issue Dec 7, 2018 · 12 comments

Comments

@tyranron
Copy link
Contributor

tyranron commented Dec 7, 2018

I know this issue has been raised already a couple of times (#16, #36, even by me). But with latest stable 1.31 release and 2018 edition landing, the clippy and rustfmt components are part of stable toolchain (so no versions mess). Maybe they worth to be included into rust image by default as are widely used in comparing to other rustup components?

The current situation with CI is relatively OK:

fmt:rust:
  stage: test
  dependencies: []
  image: rust:1.31
  services: []
  before_script:
    - rustup component add rustfmt
  script:
    - cargo fmt --check

But is trickier when it comes to CLI Docker usage:

docker run --rm -v "$(pwd)":/app -w /app rust:1.31 cargo fmt
error: 'cargo-fmt' is not installed for the toolchain '1.31.0-x86_64-unknown-linux-gnu'
To install, run `rustup component add rustfmt`

Having them baked into the image already will make its usage smoother for almost any project.

@sfackler
Copy link
Member

sfackler commented Dec 7, 2018

This seems like a decision that should be made in rustup rather than the docker image.

@Stargateur
Copy link

Stargateur commented Feb 18, 2020

@sfackler rustup seem to say that it's not their business, rust-lang/rustup#1569 (comment) is their any reason now to not include it now ?

Could we have a rust:tools or something ?

@mleonhard
Copy link

Can we please re-open and reconsider this?

The instrumentisto/rust images have clippy and rustfmt.

Related: "Include rustfmt and clippy" rust-lang/docker-rust-nightly#25

@strct
Copy link

strct commented Jan 11, 2022

There is an unofficial docker image for latest rust including cargo fmt and cargo clippy: https://hub.docker.com/r/rustdocker/rust (It's not mine, but may be valuable for others searching a solution). Gitlab repository here: https://gitlab.com/imp/rustdocker/blob/master/rust/Dockerfile

@abonander
Copy link

rustdocker/rust unfortunately looks like it hasn't been kept up to date.

I would really suggest at least providing another tag with Clippy and rustfmt installed, because when using the image in CI you're gonna end up installing them anyway.

@strct
Copy link

strct commented Feb 1, 2022

I do agree that it would be really great if rustfmt and clippy would be added to the official docker image, or a separate official docker image with clippy and rustfmt would be added (at least for stable rust in my use case).

rustdocker/rust doesn't add newer tags to docker.io, but it is on the latest rust version:

podman run --rm -it docker.io/rustdocker/rust:stable
info: syncing channel updates for '1.58.1-x86_64-unknown-linux-gnu'

  1.58.1-x86_64-unknown-linux-gnu unchanged - rustc 1.58.1 (db9d1b20b 2022-01-20)

info: checking for self-updates
rustup 1.24.3 (ce5817a94 2021-05-31)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.58.1 (db9d1b20b 2022-01-20)`
rustc 1.58.1 (db9d1b20b 2022-01-20)
cargo 1.58.0 (f01b232bc 2022-01-19)
root@7a666a78d328:/# rustc --version
rustc 1.58.1 (db9d1b20b 2022-01-20)
root@7a666a78d328:/# rustfmt --version
rustfmt 1.4.38-stable (db9d1b2 2022-01-20)

not sure why rustdocker/rust is trying to rustup update when running the container though.

@Stargateur
Copy link

Stargateur commented Feb 2, 2022

I personally stop waiting and use the image of our beloved master shep, https://hub.docker.com/u/shepmaster (contains nice image and updated daily). Since shep is a trusted and old Rust contributor it's enough secure for me.

Thus I still push for an official one.

@strct
Copy link

strct commented Feb 2, 2022

Thanks for the link. I didn't know about this docker image. I'll start using them too. But I would still appreciate it if rustfmt and clippy were included in the official rust docker image.

@mleonhard
Copy link

I still need this. Please re-open?

@vaelund
Copy link

vaelund commented Feb 23, 2022

Please re-open?

I second this. Please reconsider.

Is there any good argument against this, apart from increasing the image size?

@workingjubilee
Copy link
Member

workingjubilee commented Mar 10, 2023

Hello from Rustup-land. I come bearing the current apparent consensus from Rustup.

We're thinking about weakening the recommendation for using the "minimal" profile in CI and explaining why we recommended it in the first place in our docs, but also explaining why we don't think the default profile is appropriate for CI.

Namely, the "default" profile includes, according to du... apparently over 600 megabytes of documentation? Damn. You most definitely do not need that in a Docker image! The machine is not going to reference the documentation while running tests.

At the same time: the minimal profile really is intended to be, well, minimal, and it's really, really hard to say what should go in a hypothetical "ci" profile. The recommendation of "use minimal for CI" was written at a time that rustfmt and clippy did not have the same level of adoption they have acquired, and the most common usecase for the toolchain in CI was simply building code and running tests, and it made sense.

We think that if there's a desire to have a more useful Docker image with selected additional tools, that it should simply be done. It does not need Rustup's blessing, nor do we want to be responsible for deciding what the most maximally useful "Docker image blessed by Rust" is. Rustup is a tool that is designed to allow you to add and remove other parts of the toolchain as you see fit, by design. Please use that feature! We are more likely to have a better idea of what should go into a hypothetical "ci" or perhaps "tools" profile if other parts of the Rust project do feel free to add on to what the existing minimal and default profiles come with.

Don't use the complete profile, though. It's bad!

@vaelund
Copy link

vaelund commented Mar 14, 2023

It appears the following PR would solve this issue: #122
Can I motivate anyone to review that?

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

No branches or pull requests

8 participants