Stream: git-wasmtime

Topic: wasmtime / Issue #1314 CI: add cargo audit job

Wasmtime GitHub notifications bot (Mar 17 2020 at 19:51):

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:

1. There is no correlation between the contents of Cargo.lock and when cargo-audit needs to be rebuilt, because cargo-audit isn't a dependency. We would still get some caching behavior simply because Cargo.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 to cargo-audit releases from time to time.

2. Caching ~/.cargo will use a much bigger chunk of the overall cache budget. The cache budget is 5GB 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 take 20s to 30s 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 (like wasm-pack or sccache does) or at least by configuring GitHub release metadata so the current version can be fetched with a one-liner via jq.

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.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:08):

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 like cargo 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.

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:46):

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 audit

Alternatively, 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

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:48):

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 audit

Alternatively, 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

Wasmtime GitHub notifications bot (Mar 19 2020 at 21:55):

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.

Wasmtime GitHub notifications bot (Mar 19 2020 at 23:52):

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?

Wasmtime GitHub notifications bot (Mar 19 2020 at 23:54):

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?

Wasmtime GitHub notifications bot (Mar 20 2020 at 14:36):

alexcrichton commented on Issue #1314:

My point is that Cargo does its own caching. Executing cargo install when it's already installed means that cargo install will exit quickly.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:33):

darinmorrison commented on Issue #1314:

My point is that Cargo does its own caching. Executing cargo install when it's already installed means that cargo 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 rebuild

Doing 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 override

However, the above only works correctly if we cache the entire custom root /tmp/cargo-audit:

bin
.crates.toml
.crates2.json

If .crates.toml and .crates2.json are missing (and we only keep bin 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 overwrite

Anyway, sorry if this was obvious, I just want to make sure that the caching actually works as intended. I'll push an update soon.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:37):

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 that cargo 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 rebuild

Doing 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 override

However, the above only works correctly if we cache the entire custom root /tmp/cargo-audit:

bin
.crates.toml
.crates2.json

If .crates.toml and .crates2.json are missing (and we only keep bin 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 overwrite

Anyway, sorry if this was obvious, I just want to make sure that the caching actually works as intended. I'll push an update soon.

Wasmtime GitHub notifications bot (Mar 20 2020 at 16:47):

alexcrichton commented on Issue #1314:

Indeed yeah, without --force and caching the whole root, I think that should work?

Wasmtime GitHub notifications bot (Mar 20 2020 at 18:11):

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.

Wasmtime GitHub notifications bot (Mar 24 2020 at 14:23):

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: Oct 23 2024 at 20:03 UTC