darinmorrison commented on Issue #1314:
A few more comments about this:
First, just as a data point another project (tokio-rs/tracing) has been doing it this way also and you can see that it works well.
Secondly, why not just use
Cargo.lock
as the key and cache~/.cargo
. We could do that here since the lock file is in-repo, but I think there are two downsides:
There is no correlation between the contents of
Cargo.lock
and whencargo-audit
needs to be rebuilt, becausecargo-audit
isn't a dependency. We would still get some caching behavior simply becauseCargo.lock
changes over time (causing the cache to be re-generated) but on average that approach will be less efficient because the cache will become stale with respect tocargo-audit
releases from time to time.Caching
~/.cargo
will use a much bigger chunk of the overall cache budget. The cache budget is5GB
now, so maybe it's not a practical problem, but the difference between caching~/.cargo
versus just the binary will be on the order of hundreds of MB vs a couple of MB. However, another side to this is that restoring caches in GitHub actions is currently rather slow, e.g., restoring a~300MB
cache can take20s
to30s
just on its own, before the build even starts.I suppose someone could just ask the
cargo-audit
maintainers if they could make the project more CI friendly by either providing pre-built binaries (likewasm-pack
orsccache
does) or at least by configuring GitHub release metadata so the current version can be fetched with a one-liner viajq
.In any case, I could still modify this to use
Cargo.lock
if y'all want to try that but I thought it would be worth noting those two issues.
alexcrichton commented on Issue #1314:
My main hesitation here is I basically just don't understand the CI at all, it seems like a wad of goop that you have to hunt down a lot of documentation to understand. I think we'd benefit from having something much simpler here.
Could we perhaps specify a version of
cargo-audit
to install? Something likecargo install cargo-audit --vers 1.2.3
? That way we could cache based on the key "1.2.3" and we could update all that once a new version is released. Additionally this also protects us from accidentally breaking changes upstream so our CI should be more stable.
darinmorrison commented on Issue #1314:
Yes, I think that would work okay.
I might suggest adding a static file
.github/caching/cargo-audit.lock
and setting the contents of the file to the version, e.g.,1.2.3
.That way you could easily update it with a script (e.g.,
script/update-ci-tools.sh
or whatever) rather than having to go in and manually edit.github/workflow/main.yaml
.It would essentially be doing the same thing as that
run
script, but you'd trigger it manually.This is what it would look like:
cargo-audit: runs-on: ubuntu-latest steps: - uses: actions/checkout@master - name: Cache cargo-audit/bin id: cache-cargo-audit uses: actions/cache@v1 with: path: ${{ runner.tool_cache }}/cargo-audit/bin key: cargo-audit-bin-${{ hashFiles('.github/caching/cargo-audit.lock') }} - name: Install cargo-audit if: "steps.cache-cargo-audit.outputs.cache-hit != 'true'" run: cargo install --root ${{ runner.tool_cache }}/cargo-audit --force cargo-audit - run: echo "::add-path::${{ runner.tool_cache }}/cargo-audit/bin" - run: cargo auditAlternatively, you could set the version in the job environment. It's a bit of a hack, but it would improve visibility of the version number if you just want to edit
main.yaml
manually:cargo-audit: env: CARGO_AUDIT_VERSION: 1.2.3 runs-on: ubuntu-latest steps: - uses: actions/checkout@master - name: Cache cargo-audit/bin id: cache-cargo-audit uses: actions/cache@v1 with: path: ${{ runner.tool_cache }}/cargo-audit/bin key: ${{ env.CARGO_AUDIT_VERSION }} - name: Install cargo-audit if: "steps.cache-cargo-audit.outputs.cache-hit != 'true'" run: cargo install --root ${{ runner.tool_cache }}/cargo-audit --force cargo-audit - run: echo "::add-path::${{ runner.tool_cache }}/cargo-audit/bin" - run: cargo audit
darinmorrison edited a comment on Issue #1314:
Yes, I think that would work okay.
I might suggest adding a static file
.github/caching/cargo-audit.lock
and setting the contents of the file to the version, e.g.,1.2.3
.That way you could easily update it with a script (e.g.,
script/update-ci-tools.sh
or whatever) rather than having to go in and manually edit.github/workflow/main.yaml
.It would essentially be doing the same thing as that
run
script, but you'd trigger it manually.This is what it would look like:
cargo-audit: runs-on: ubuntu-latest steps: - uses: actions/checkout@master - name: Cache cargo-audit/bin id: cache-cargo-audit uses: actions/cache@v1 with: path: ${{ runner.tool_cache }}/cargo-audit/bin key: cargo-audit-bin-${{ hashFiles('.github/caching/cargo-audit.lock') }} - name: Install cargo-audit if: "steps.cache-cargo-audit.outputs.cache-hit != 'true'" run: cargo install --root ${{ runner.tool_cache }}/cargo-audit --force cargo-audit - run: echo "::add-path::${{ runner.tool_cache }}/cargo-audit/bin" - run: cargo auditAlternatively, you could set the version in the job environment. It's a bit of a hack, but it would improve visibility of the version number if you just want to edit
main.yaml
manually:cargo-audit: env: CARGO_AUDIT_VERSION: 1.2.3 runs-on: ubuntu-latest steps: - uses: actions/checkout@master - name: Cache cargo-audit/bin id: cache-cargo-audit uses: actions/cache@v1 with: path: ${{ runner.tool_cache }}/cargo-audit/bin key: cargo-audit-bin-${{ env.CARGO_AUDIT_VERSION }} - name: Install cargo-audit if: "steps.cache-cargo-audit.outputs.cache-hit != 'true'" run: cargo install --root ${{ runner.tool_cache }}/cargo-audit --force cargo-audit - run: echo "::add-path::${{ runner.tool_cache }}/cargo-audit/bin" - run: cargo audit
alexcrichton commented on Issue #1314:
Yes that's along the lines of what I'm thinking. I don't think we need the
if
statement or names for each step as well, and that should clean it up even more too.
darinmorrison commented on Issue #1314:
Okay, I made some changes based on this discussion.
I think we do still need the
if
statement though because the only directory that is being cached is${{ runner.tool_cache }}/cargo-audit/bin
.Without the
if
, won't all of the dependencies be fetched each time back into~/.cargo
and and be rebuilt each time the job is run?
darinmorrison edited a comment on Issue #1314:
Okay, I made some changes based on this discussion.
I think we do still need the
if
statement though because the only directory that is being cached is${{ runner.tool_cache }}/cargo-audit/bin
.Without the
if
, won't all of the dependencies be fetched and rebuilt in~/.cargo
each time the job is run?
alexcrichton commented on Issue #1314:
My point is that Cargo does its own caching. Executing
cargo install
when it's already installed means thatcargo install
will exit quickly.
darinmorrison commented on Issue #1314:
My point is that Cargo does its own caching. Executing
cargo install
when it's already installed means thatcargo install
will exit quickly.Sorry to keep returning to this point, but what wasn't clear to me were the rules under which Cargo will determine that a binary is considered "installed" and not do additional work.
In any case, I just ran some experiments and I think I understand now. This is what I tried:
$ cargo install --root /tmp/cargo-audit --force cargo-audit $ rm -rf ~/.cargo $ rustup $ cargo install --root /tmp/cargo-audit --force cargo-audit # results in rebuildDoing this results in
cargo-audit
being re-built, presumably because of--force
.Removing
--force
seems to do what you want:$ cargo install --root /tmp/cargo-audit cargo-audit $ rm -rf ~/.cargo $ rustup $ cargo install --root /tmp/cargo-outdated cargo-outdated Updating crates.io index Ignored package `cargo-outdated v0.9.7` is already installed, use --force to overrideHowever, the above only works correctly if we cache the entire custom root
/tmp/cargo-audit
:bin .crates.toml .crates2.jsonIf
.crates.toml
and.crates2.json
are missing (and we only keepbin
as now) it errors:$ cargo install --root /tmp/cargo-outdated cargo-outdated Updating crates.io index error: binary `cargo-outdated` already exists in destination Add --force to overwriteAnyway, sorry if this was obvious, I just want to make sure that the caching actually works as intended. I'll push an update soon.
darinmorrison edited a comment on Issue #1314:
My point is that Cargo does its own caching. Executing
cargo install
when it's already installed means thatcargo install
will exit quickly.Sorry to keep returning to this point, but what wasn't clear to me were the rules under which Cargo will determine that a binary is considered "installed" and not do additional work.
In any case, I just ran some experiments and I think I understand now. This is what I tried:
$ cargo install --root /tmp/cargo-audit --force cargo-audit $ rm -rf ~/.cargo $ rustup $ cargo install --root /tmp/cargo-audit --force cargo-audit # results in rebuildDoing this results in
cargo-audit
being re-built, presumably because of--force
.Removing
--force
seems to do what you want:$ cargo install --root /tmp/cargo-audit cargo-audit $ rm -rf ~/.cargo $ rustup $ cargo install --root /tmp/cargo-audit cargo-audit Updating crates.io index Ignored package `cargo-audit v0.11.2` is already installed, use --force to overrideHowever, the above only works correctly if we cache the entire custom root
/tmp/cargo-audit
:bin .crates.toml .crates2.jsonIf
.crates.toml
and.crates2.json
are missing (and we only keepbin
as now) it errors:$ cargo install --root /tmp/cargo-audit cargo-audit Updating crates.io index error: binary `cargo-audit` already exists in destination Add --force to overwriteAnyway, sorry if this was obvious, I just want to make sure that the caching actually works as intended. I'll push an update soon.
alexcrichton commented on Issue #1314:
Indeed yeah, without
--force
and caching the whole root, I think that should work?
darinmorrison commented on Issue #1314:
@alexcrichton looks like the caching works: https://github.com/bytecodealliance/wasmtime/pull/1314/checks?check_run_id=522651021
Should be good to go.
alexcrichton commented on Issue #1314:
Ok the CI bits look good here to me, thanks @darinmorrison!
@sunfishcode do you have thoughts on whether we want to add this to CI?
Last updated: Dec 23 2024 at 12:05 UTC