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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Oct 18, 2023
Merged

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

merged 1 commit into from
Oct 18, 2023

Conversation

epilys
Copy link
Member

@epilys epilys commented Oct 17, 2023 •
edited
Loading

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:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@epilys
Don't install rustdoc along with default Rust toolchain
a275f30 
Downloading and installing rustdoc every the image is built takes
considerable time that is not necessary, so specify only the rustup
components we need.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
@epilys epilys mentioned this pull request Oct 17, 2023
Closed
4 tasks
roypat
roypat reviewed Oct 18, 2023
View reviewed changes
Copy link
Collaborator

@roypat roypat left a comment

Choose a reason for hiding this comment

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

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

@epilys
Copy link
Member Author

epilys commented Oct 18, 2023

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
Copy link
Collaborator

roypat commented Oct 18, 2023

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!

@roypat roypat merged commit ff56109 into rust-vmm:main Oct 18, 2023
2 checks passed
@epilys
Copy link
Member Author

epilys commented Oct 18, 2023

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

Absolutely no problem :) Enjoy your coffee!

@epilys epilys deleted the fix-remove-rustdoc-install branch October 18, 2023 08:48
@stefano-garzarella
Copy link
Member

stefano-garzarella commented Oct 19, 2023 •
edited
Loading

@epilys I'm updating rust-vmm-ci to use the new container (v26 -> v28): rust-vmm/rust-vmm-ci#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 (rust-vmm/rust-vmm-ci#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 stefano-garzarella mentioned this pull request Oct 19, 2023
Merged
4 tasks
@stefano-garzarella
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

epilys commented Oct 19, 2023

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stsquad stsquad stsquad approved these changes

@roypat roypat roypat approved these changes

@andreeaflorescu andreeaflorescu Awaiting requested review from andreeaflorescu andreeaflorescu is a code owner

@lauralt lauralt Awaiting requested review from lauralt lauralt is a code owner

@stefano-garzarella stefano-garzarella Awaiting requested review from stefano-garzarella stefano-garzarella is a code owner

@JonathanWoollett-Light JonathanWoollett-Light Awaiting requested review from JonathanWoollett-Light

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@epilys @roypat @stefano-garzarella @stsquad