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

actions: Improve the CI caching and simplify the cargo CLI options #131

Merged
merged 3 commits into from
Jan 4, 2023

Conversation

primeos-work
Copy link
Member

@primeos-work primeos-work commented Jan 3, 2023

According to my tests this fixes/improves the GitHub Actions caching, I simplified and unified the cargo CLI options, and disabled the caching for cargo-deny as that likely doesn't work anyway. The details are explained in the individual commit messages.

Edit: Oops, forgot to use --signoff - fixed with git rebase --signoff :)

The `Swatinem/rust-cache@v2` action uses a prefix as part of the cache
key. By default, this prefix is formed by combining the `prefix-key`
(defaults to "v0-rust") and the `shared-key` (defaults to the job name).
As a result, this breaks caching within the CI workflow as the prefix
differs for each job (e.g., `v0-rust-test` for the test job) which
prevents the cache from being restored (`No cache found.`).

Setting `shared-key` (default: empty) will prevent this by using that
string instead of the job name.
Therefore, caching within a ci.yml workflow will now work as expected.

Signed-off-by: Michael Weiss <michael.weiss@atos.net>
The `--all-targets` option is equivalent to specifying `--lib --bins
--tests --benches --examples`. We seem to only need `--tests` (as the
binaries are built as unittests by default) but we might as well set
`--all-targets` for future-proofing (in case we ever add any examples,
benchmarks, or a library).
By running `cargo check` with `--all-targets` we ensure that all of the
code is already compiled for subsequent commands (e.g., useful for the
caching of the CI workflow).

The `--all` option is a deprecated alias for `--workspace`. We only have
one member (butido) in the workspace or rather don't define a workspace
in Cargo.toml (it would have a `workspace` table instead of the
`package` table). Therefore, we may drop that option for simplicity.

The same applies to the `--all-features` option: We don't define any
butido features in Cargo.toml (no `features` table/section).

Last but not least, I updated CONTRIBUTING.md to (almost) match our CI
workflow.

Signed-off-by: Michael Weiss <michael.weiss@atos.net>
We probably don't benefit of the caching here. The cargo-deny job
doesn't use `dtolnay/rust-toolchain` as
`EmbarkStudios/cargo-deny-action` uses a Docker container (currently
based on `rust:1.65.0-alpine3.16`). This can/will result in a different
cache key (currently due to a different compiler version and the lack of
the `CARGO_TERM_COLOR` environment variable).
Plus cargo-deny doesn't need to compile any packages. It needs to
download a few things (in /home/runner/.cargo - which doesn't seem to
get bind mounted into the Docker container anyway) but
`swatinem/rust-cache` also needs to download the cache.

This change should make things faster as the cargo-deny job doesn't need
to wait for the check job anymore and the caching likely doesn't work at
all (it likely won't matter though as the cargo-deny job shouldn't be
the slowest job of the CI workflow but let's drop the caching here for
simplicity and less confusion).

Signed-off-by: Michael Weiss <michael.weiss@atos.net>
@primeos-work
Copy link
Member Author

primeos-work commented Jan 4, 2023

Looking at the CI results here, this didn't work as well as I expected (not sure if the caching within a workflow works or if it would e.g. require additional delays) but I think the idea/concept is still fine/correct so let's give it a try and see how it'll work out (and make further changes, if necessary).
Edit: Nvm, it probably works as expected, I just forgot that the first step is a cargo check and not a cargo build so it shouldn't be surprising that the latter commands still need to compile some packages - I wonder if it might be better to do a cargo build first (instead of the cargo check) since we likely need to compile all of the packages for cargo test anyway (and the additional checking without the final code generation step would be wasted effort if that doesn't get cached).

bors merge
Edit: Attempt 2

@primeos-work
Copy link
Member Author

bors merge
(Note: bors got stuck (waiting to run - maybe it would've eventually started after some time but I didn't find anything it could be waiting for) and didn't consider the other two PRs (not even queued via another batch) so I cancelled everything. Let's hope it'll work better this time. Sorry for the noise.)

@bors
Copy link
Contributor

bors bot commented Jan 4, 2023

Build succeeded:

  • CI

@bors bors bot merged commit f01f90b into science-computing:master Jan 4, 2023
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.

None yet

1 participant