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 Rust images to use default rustup profile #960

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sgdxbc
Copy link

@sgdxbc sgdxbc commented Feb 11, 2024

The default profile including tools for formatting and linting, which are essential to development.

Also update Dockerfile. Previously it was based on Rust's official docker image which is hardcoded to set up a minimal profile Rust toolchain. That causes the following rust devcontainer feature to operate incompletely. Replacing based image to Rust's official docker image's upstream image solved the problem.

Also format the devcontainer config file itself a little bit.

@sgdxbc sgdxbc requested a review from a team as a code owner February 11, 2024 06:33
@sgdxbc
Copy link
Author

sgdxbc commented Feb 11, 2024

@microsoft-github-policy-service agree

@sgdxbc
Copy link
Author

sgdxbc commented Feb 13, 2024

Firstly I admit that the description above exaggerated the problem for a bit. Currently with a freshly set up devcontainer, the linting and formatting tools are actually available. (Guess that's also the reason why I am the first person complaining this.) However the overall initial configuration is kind of confusing and error-prone when it evolves later (which I will explain later), which brings the idea of this PR.

The based upstream image rust-1 (as shown in https://github.com/rust-lang/docker-rust/blob/master/1.76.0/bookworm/Dockerfile) has been mainly designed for CI usage and:

  • Set up a pinned version toolchain (e.g. 1.72.1-x86_64-unknown-linux-gnu currently) instead of the latest stable that usually more desired for development
  • Set the profile to minimal

According to the discussions at rust-lang/rustup#2579 and rust-lang/docker-rust#97, the upstream docker image is intentionally opinionated, and the two points above are not (and probably will never be) configurable.

In the later stage of setting up devcontainer, the "rust" devcontainer feature kicks in, installs a few auxiliary libraries, skips download and set up rustup since it's already there (so the options passed into the feature is unused, which is what I referenced as "operate incompletely"), and forcefully add linting and formatting components to the minimal toolchain that upstream image set up, results in what we see in the fresh devcontainer: a minimal profile version-pinned toolchain with additional desired components.

In my opinion this is confusing. I noticed my toolchain is not updated when I do rustup update, then I realize it is a version-pinned toolchain and do rustup install stable, which installs the latest stable toolchain in minimal profile, which takes away linting and formatting tools. The PR here, which mimics my way to manually patch the devcontainer, will set up a latest version toolchain with default profile, which matches my expectation much better, and I believe that is also the common demand.

I am aware that there are at least two ways to fix this. One is the approach I take, to remove the rustup from based image, instead leverage the devcontainer feature to set up rustup so it will respect the option I added. The other approach is to update the feature script so that it would correct the profile for an existing rustup. Notice that the profile is only considered when installing toolchains, so the missing components still need to be added manually.

I have taken the first approach because I am not aware of the backward compatibility policy (can you post a link so I can check?), and it feels more nature than the alternative approach because only single party take care of the toolchain.

Thanks for reviewing and reading these. Looking forward to what you think on it.

@samruddhikhandale
Copy link
Member

Thank you so much @sgdxbc for taking the time in writing up a beautiful explanation.

The reasoning makes sense to me and I can see how it can be beneficial to switch the profile to minimal. As this is an enhancement to the image, happy to help you with it.

However, as the problem you mentioned on why we shouldn't base out of the official rust image, I understand your point. However, we'd like to ensure that https://github.com/rust-lang/docker-rust/blob/master/1.76.0/bookworm/Dockerfile#L3-L27 code remains intact with this newly built image.

Either make sure Rust Feature already follows the same behavior (eg. setting env variables, installing rust etc) or modify it (preferred), else add commands to the Dockerfile.

Another note: We need to make sure that bookworm, bullseye and buster are making same changes (as we don't want it to impact across distros)

"ghcr.io/devcontainers/features/rust:1": "latest",
"ghcr.io/devcontainers/features/git:1": {
"ghcr.io/devcontainers/features/rust:1": {
"version": "latest",
Copy link
Member

@samruddhikhandale samruddhikhandale Feb 16, 2024

Choose a reason for hiding this comment

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

We would need to pin this to v1.

Currently, rust is publishing major version 1, so we would need to pin it.

FROM rust:1-${VARIANT}

This ^ was previously ensuring it, else when v2 releases, the build tool will incorrectly tag it to mcr.microsoft.com/devcontainers/rust:1-1

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I don't quiet get it. Do you mean we need to pin feature dependency to v1, or we need to simulate the effect of originally pinning base image to rust:1, or we need to pin this devcontainer image itself to v1?

@samruddhikhandale
Copy link
Member

@bamurtaugh @craiglpeters Do we know of a Rust expert who can help us review the reasonings of this PR? Thanks!

@sgdxbc
Copy link
Author

sgdxbc commented Feb 16, 2024

I have just checked the upstream dockerfile and the feature script. I admit to not being the one most familiar of the both, but in my opinion they match each other enough.

The first envvars RUSTUP_HOME and CARGO_HOME in dockerfile are set by feature script to the same value. The PATH is updated by rustup init script to the same value on feature side. The RUST_VERSION envvar is missing on feature side, which is expected since it's not setting up a version-pinned toolchain.

The following part of dockerfile downloads rustup init script from https://static.rust-lang.org/rustup/archive/1.26.0/${rustArch}/rustup-init, exam it with hardcoded hash and execute it. The feature script downloads the script from https://static.rust-lang.org/rustup/dist/${download_architecture}-unknown-linux-gnu/rustup-init, exam the script against an also-downloaded hash and execute with same arguments to dockerfile. The rustup script URL differs, but I guess each of them is also good for pinned/rolling strategy.

Although they currently have quiet matching behaviors, I wonder whether we need to keep the similarity forever if we decide to opt out official upstream image. (And if so whether that is solely out of backward-compatibility reason.) If the rust devcontainer feature is self-contained, it seems not particular meaningful to provide a dedicated devcontainer image for users that only want their devcontainers to have Rust. For example, personally I have discarded the dockerfile and switch to set image as base-ubuntu in my devcontainer config, because Ubuntu is my more desired env. I am wondering how other developer would think.

@bamurtaugh
Copy link
Member

@bamurtaugh @craiglpeters Do we know of a Rust expert who can help us review the #960 (comment) of this PR? Thanks!

@connor4312 I'm wondering if you might have thoughts based on your Rust expertise?

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.

4 participants