search

rust-vmm / rust-vmm-container

Container with all dependencies required for running rust-vmm crates integration tests.
Apache License 2.0
65 stars 32 forks source link

Don't install rustdoc along with default Rust toolchain #89

Closed epilys closed 11 months ago

epilys commented 11 months ago

Summary of the PR

Downloading and installing rustdoc every time the image is built takes considerable time that is not necessary, so specify only the rustup components we need.

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

epilys commented 11 months ago

I think the minimal toolchain also excludes clippy and rustfmt, no? I can't see those getting installed in the GitHub runner log

I see them. Maybe you looked at the nightly installation?

They are added with a --components flag argument:

--profile minimal --component clippy,rustfmt

 #12 [linux/amd64 3/3] RUN /opt/src/scripts/build_container.sh
#12 36.18 info: profile set to 'minimal'
#12 36.18 info: default host triple is x86_64-unknown-linux-gnu
#12 36.18 info: syncing channel updates for '1.72.0-x86_64-unknown-linux-gnu'
#12 36.32 info: latest update on 2023-08-24, rust version 1.72.0 (5680fa18f 2023-08-23)
#12 36.32 info: downloading component 'cargo'
#12 36.41 info: downloading component 'clippy'
#12 36.45 info: downloading component 'rust-std'
#12 36.73 info: downloading component 'rustc'
#12 37.34 info: downloading component 'rustfmt'
#12 37.37 info: installing component 'cargo'
#12 38.14 info: installing component 'clippy'
#12 39.17 info: installing component 'rust-std'
#12 41.82 info: installing component 'rustc'
roypat commented 11 months ago

I think the minimal toolchain also excludes clippy and rustfmt, no? I can't see those getting installed in the GitHub runner log

I see them. Maybe you looked at the nightly installation?

They are added with a --components flag argument:

--profile minimal --component clippy,rustfmt

 #12 [linux/amd64 3/3] RUN /opt/src/scripts/build_container.sh
#12 36.18 info: profile set to 'minimal'
#12 36.18 info: default host triple is x86_64-unknown-linux-gnu
#12 36.18 info: syncing channel updates for '1.72.0-x86_64-unknown-linux-gnu'
#12 36.32 info: latest update on 2023-08-24, rust version 1.72.0 (5680fa18f 2023-08-23)
#12 36.32 info: downloading component 'cargo'
#12 36.41 info: downloading component 'clippy'
#12 36.45 info: downloading component 'rust-std'
#12 36.73 info: downloading component 'rustc'
#12 37.34 info: downloading component 'rustfmt'
#12 37.37 info: installing component 'cargo'
#12 38.14 info: installing component 'clippy'
#12 39.17 info: installing component 'rust-std'
#12 41.82 info: installing component 'rustc'

Ahhh, my bad, should not be reviewing PRs before my coffee. Sorry!

epilys commented 11 months ago

Ahhh, my bad, should not be reviewing PRs before my coffee. Sorry!

Absolutely no problem :) Enjoy your coffee!

stefano-garzarella commented 10 months ago

@epilys I'm updating rust-vmm-ci to use the new container (v26 -> v28): https://github.com/rust-vmm/rust-vmm-ci/pull/138

The CI is failing when calling cargo audit. I replicated locally, and it started to fail with v27 that should be related to this PR.

I run the test in the rust-vmm-ci folder (https://github.com/rust-vmm/rust-vmm-ci/pull/138 branch) and I did a git clean -xfd to be sure having a clean repo (e.g. Cargo.lock removed):

v27 and v28 blocks indefinitely:

podman run -it --rm --security-opt=seccomp=unconfined --volume $(pwd):/workdir --workdir /workdir rustvmm/dev:v28 /bin/sh -e -c cargo\ audit\ --deny\ warnings
    Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
      Loaded 575 security advisories (from /root/.cargo/advisory-db)
    Updating crates.io index
    Blocking waiting for file lock on package cache

v26 worked well:

podman run -it --rm --security-opt=seccomp=unconfined --volume $(pwd):/workdir --workdir /workdir rustvmm/dev:v27 /bin/sh -e -c cargo\ audit\ --deny\ warnings
    Fetching advisory database from `https://github.com/RustSec/advisory-db.git`
      Loaded 575 security advisories (from /root/.cargo/advisory-db)
    Updating crates.io index
    Scanning Cargo.lock for vulnerabilities (1 crate dependencies)

Do you have any idea?

stefano-garzarella commented 10 months ago

Running cargo build just before calling cargo audit generates the Cargo.lock and fixes the issue, but I have no idea what is changed from v26. I'm also not sure this PR is related, since I tried to revert this commit and generate a new image locally, and I have the same behavior.

stefano-garzarella commented 10 months ago

Looking at https://crates.io/crates/cargo-audit they call cargo generate-lockfile before cargo audit so it looks like it is a prerequisite. I think this PR is fine, maybe we should fix rust-vmm-ci.

epilys commented 10 months ago

@stefano-garzarella was just gonna post that I found this too. We don't install cargo-audit at a pinned version so the images probably have a different one.

[1]+ ~/.rustup/toolchains/1.72.0-x86_64-unknown-linux-gnu/bin/cargo audit &    
root@031791dfe6de:/workdir# fuser ~/.cargo/.package-cache                                                                                                                                                                                                                                                                      
/root/.cargo/.package-cache:   567   598
root@031791dfe6de:/workdir# ps aux
USER         PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root           1  0.0  0.0   1008     4 pts/0    Ss   15:07   0:00 /sbin/docker-init -- /bin/bash
root           7  0.0  0.0   4624  3920 pts/0    S    15:07   0:00 /bin/bash
root         567  0.1  0.0 1700436 19688 pts/0   S    15:20   0:00 /root/.cargo/bin/cargo-audit audit
root         598  0.0  0.0  27088 13872 pts/0    S    15:20   0:00 /root/.rustup/toolchains/1.72.0-x86_64-unknown-linux-gnu/bin/cargo generate-lockfile
root         604  0.0  0.0   7060  1564 pts/0    R+   15:21   0:00 ps aux