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

ci: Enable caching #702

Merged
merged 1 commit into from
Mar 25, 2023
Merged

ci: Enable caching #702

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ jobs:
- name: Checkout sources
uses: actions/checkout@v3

- uses: Swatinem/rust-cache@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use an approach as in https://github.com/rust-osdev/multiboot2/blob/main/.github/workflows/_build-rust.yml#L52 instead.

The solution you picked for caching only caches dependencies and only works reliably with a Cargo.lock file, as the README says. https://github.com/Swatinem/rust-cache

Hence, I'm in favor of a solution as in the Multiboot2 crate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to use a prebuilt solution if possible (easier to read the CI config then, and hopefully simpler to maintain), so I'd like to carefully check that Swatinem/rust-cache really doesn't meet our needs.

That's a good point about Cargo.lock, but we could probably just check in Cargo.lock. The Cargo FAQ recommends against it, but I've often seen that recommendation debated. See rust-lang/cargo#8728 for example. Alternatively, we could skip checking in Cargo.lock and see how much difference it makes for us in practice. We have a nightly CI run, and I think we might find that does a decent job of initializing our caches so that PR runs are fast (which is probably when we will actually notice and care about CI time).

As to only caching dependencies, I think that's fine. rust-cache makes the claim that caching incremental builds of the crate itself is generally not effective, and I find that convincing. Caching the deps used by xtask in particular is what will really save a lot of CI time I think.

Regarding total space used by the cache:

gh api \
  -H "Accept: application/vnd.github+json" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  /repos/rust-osdev/uefi-rs/actions/cache/usage

{
  "full_name": "rust-osdev/uefi-rs",
  "active_caches_size_in_bytes": 5848673391,
  "active_caches_count": 19
}

So about 5.5GiB currently in use for this project. That seems fine right?

Lastly, I tried running the CI on this PR, then running it a second time. All the jobs with the cache enabled ran faster. That seems like pretty strong evidence to me that the cache is working as intended. On the left is the first run without an up-to-date cache, and on the right is the run with a good cache:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a few ideas I like to try out, ok? I think, we can have a neat and simpler solution for everything.. let's see.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a few ideas I like to try out, ok? I think, we can have a neat and simpler solution for everything.. let's see.

Hm, I tested something like in the multiboot2 repo: one single big workflow that covers all the building, style checks, unit testing and integration tests that has a lot of configure options.

The outcome is a big feature-creep, semi-working variant.


- name: Install qemu and OVMF
run: |
sudo apt-get update
Expand All @@ -36,6 +38,8 @@ jobs:
- name: Checkout sources
uses: actions/checkout@v3

- uses: Swatinem/rust-cache@v2
Copy link
Contributor

@phip1611 phip1611 Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

additionally: once we set up caching, we need one task per toolchain that runs first and fills the cache. Otherwise, caching is ineffective and produces many GBs of artifacts on GitHub but we only have a 10GB limit. I experienced that in the multiboot2 crate. The pipeline there looks like this:

image

Copy link
Contributor

@phip1611 phip1611 Mar 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Otherwise, caching is ineffective and produces many GBs of artifacts on GitHub but we only have a 10GB limit.

My concern here is only that the same cache is uploaded multiple times.. simultaneously by every job as for every new change to the dependencies, they all do perform the cargo install in parallel.


- name: Install qemu and OVMF
run: |
sudo apt-get update
Expand All @@ -52,6 +56,8 @@ jobs:
- name: Checkout sources
uses: actions/checkout@v3

- uses: Swatinem/rust-cache@v2

- name: Install qemu and OVMF
run: |
sudo apt-get update
Expand All @@ -68,6 +74,8 @@ jobs:
- name: Checkout sources
uses: actions/checkout@v3

- uses: Swatinem/rust-cache@v2

- name: Run cargo test (without unstable)
run: cargo xtask test

Expand All @@ -78,6 +86,8 @@ jobs:
- name: Checkout sources
uses: actions/checkout@v3

- uses: Swatinem/rust-cache@v2

- name: Run cargo fmt
run: |
rustup component add rustfmt
Expand Down Expand Up @@ -111,6 +121,8 @@ jobs:
- name: Set MSRV toolchain
run: cp .github/workflows/msrv_toolchain.toml rust-toolchain.toml

- uses: Swatinem/rust-cache@v2

- name: Build
run: cargo xtask test-latest-release

Expand All @@ -124,6 +136,8 @@ jobs:
- name: Checkout sources
uses: actions/checkout@v3

- uses: Swatinem/rust-cache@v2

- name: Run VM tests
run: cargo xtask run --target x86_64 --ci
timeout-minutes: 6
Expand All @@ -143,6 +157,8 @@ jobs:
- name: Set toolchain
run: cp .github/workflows/msrv_toolchain.toml rust-toolchain.toml

- uses: Swatinem/rust-cache@v2

- name: Build
run: cargo xtask build

Expand All @@ -158,6 +174,8 @@ jobs:
- name: Set nightly toolchain so that `unstable` can be included
run: cp .github/workflows/nightly_toolchain.toml rust-toolchain.toml

- uses: Swatinem/rust-cache@v2

- name: Build
run: cargo xtask build --feature-permutations

Expand All @@ -176,6 +194,8 @@ jobs:
- name: Enable nightly toolchain
run: cp .github/workflows/nightly_toolchain.toml rust-toolchain.toml

- uses: Swatinem/rust-cache@v2

- name: Run VM tests with the `unstable` feature
run: cargo xtask run --target x86_64 --headless --ci --unstable
timeout-minutes: 4
Expand Down