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

Improve CI/CD #1122

Merged
merged 3 commits into from
Sep 29, 2023
Merged

Improve CI/CD #1122

merged 3 commits into from
Sep 29, 2023

Conversation

nepalez
Copy link
Contributor

@nepalez nepalez commented Sep 27, 2023

In this PR I tried to improve the CI in several aspects. Below is the description of commits in this PR:

1. Turn off incremental compilation

The incremental build is a way to fasten future compilations for the cost of extra dependency tracking and bigger /target.
This approach is not necessary in the CI because every compilation is done from scratch.

As a result of turning it off the size of the target reduces from 660MB to 543MB.

2. Turn off debug info

This change reduces the size of the /target directory from 543MB to 424MB without (or from 660MB to 483MB with) the incremental compilation.

3. Improved artifacts caching

3.1. Use Swatinem/rust-cache@v2 to make caching smarter.

The primary difference is that this GHA caches not the whole content of the ./target but only dependencies. Along with points 1 and 2 this makes caches 3x smaller (230-450MB instead of 0.8-1.3GB) for the same build time.

3.2. Split build and test stages

This enables to store the cache if the build succeeded even though the test can later fail. It also useful to immediately see the stage where a failure occur.

3.2. Use the shared_key in caches instead of the job-based key

This enables some caches (primarily ubuntu/stable) to be reused between jobs. It affects fast jobs only (like checking examples, running clippy etc., which are now reuse the caches built by test matrix), but anyway this approach consumes less resources.

3.3. Use v4 (instead of v3) of the actions/setup-go

This version provides caching by default (104MB x 3 = 312MB)

3.4. Drop unnecessary Go/nats-server installation for checking msrv and examples

These steps weren't necessary because we only run rust check without even preparing a full build, not to mention running tests.

3.5. Add the cache for the cargo-spellcheck

It saves ~4 min at the corresponding job for the cost of the additional 54MB cache.

3.6. Add the --no-deps option to cargo doc

I thought it's up to the dependencies' maintainers to check their docs


What I would also add is hashing prepared Cargo.lock instead of the **/Cargo.toml.

While this can reduce the lifetime of caches, we could see possible problems with the latest versions of dependencies.

This change would makes the msrv check slower (building lockfile on 1.67.0 takes ~2-3 min), but it wouldn't be critical (this job is 2-3 times faster than running the test suite).

I made this change in a separate commit, but would like to discuss it before adding the commit to this PR.

@nepalez nepalez force-pushed the improve-ci branch 3 times, most recently from 08b4993 to 1061e3f Compare September 27, 2023 15:04
The incremental build is a way to fasten future compilations
for the cost of extra dependency tracking and bigger /target.

This <default> approach is not necessary in the CI
because every compilation is done from scratch.

As a result of turning it off the size of the target
reduces from 660MB to 543MB.
This change reduces the size of the /target directory
from 543MB to 424MB without (or from 660MB to 483MB with)
the incremental compilation.
@levb levb self-requested a review September 28, 2023 12:35
Copy link

@levb levb left a comment

Choose a reason for hiding this comment

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

LGTM, but I am not really a Rust guy, don't know enough about the specific actions used here; Also @Jarema, I don't really understand the Rust caching, so please check that the cache key makes sense to you.

- name: Install nats-server
run: go install github.com/nats-io/nats-server/v2@latest
run: go install github.com/nats-io/nats-server/v2@main
Copy link

Choose a reason for hiding this comment

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

1/5 we should probably use the latest server release rather than main for benchmarking

Copy link

Choose a reason for hiding this comment

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

curl -s https://api.github.com/repos/nats-io/nats-server/releases/latest | jq -r '.tag_name'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After I added the caching here, this would require getting the current version of the server to use it in the cache key (for now I only build the key from the os and the go version)

@nepalez
Copy link
Contributor Author

nepalez commented Sep 28, 2023

I also updated the last commit with caching the nats-server binary as well. This saves ~3min on windows (and only ~1 min on linux/macos). The additional size of 3 caches is 3x9MB = 18MB, so it looks like a good deal.

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for your contribution!

@Jarema Jarema merged commit 4176419 into nats-io:main Sep 29, 2023
10 checks passed
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.

3 participants