Replies: 2 comments
-
|
Hi, @abnobdoss thanks for the discussion. The consolidation seems reasonable to me, and I love the lint steps split from clippy. One more thing, maybe we could let the heavy steps like build and test to depend on light steps like lint and typo, if previous steps failed, we could just stop the whole workflow. |
Beta Was this translation helpful? Give feedback.
0 replies
-
|
Thank you for reviewing @blackmwk! The gating would be a great addition to streamline things and cut down on unnecessary CI job runs. If no concerns are raised on this proposal I'll be sure to include that gate! |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
-
I'd like to propose a couple of ways we could improve and streamline CI, though I completely understand if there isn't the appetite for structural changes right now. If there's interest, I'd bring them in as small, independently reviewable PRs.
The problem
ci.ymlruns about 12 jobs on every PR, six of them some flavor of "build", and a lot of them recompile the same dependency graph independently. It works, but it's slower than it needs to be, hard to read at a glance, and it's pushed us past GitHub's 10 GB per-repo cache cap (we're at ~10.7 GB and already evicting). That ceiling only gets tighter as things like the integration tests grow toward what the other Iceberg subprojects run. Both come back to the same dependencies being compiled several times over.The two proposed changes below each take a piece of that: one on the build jobs, one on the check/lint steps.
Consolidating the builds
For the builds, I'd like to propose folding each OS's separate jobs into one, so the dependencies compile once per OS. Today each OS runs
buildandbuild_with_no_default_features, and Ubuntu also runscheck_standaloneand a separatetestsjob. That would look like:build-and-test(Ubuntu)--no-default-features, per-crate standalone check, nextest, doc testsbuild,build_with_no_default_features,check_standalone,testson Ubuntubuild(macOS, Windows)--no-default-featuresbuild,build_with_no_default_featureson macOS/Windowsmsrv(Ubuntu)Everything that runs today would still run; the
--no-default-featuresbuild would become a step in the build job, so that coverage is kept.It would also be the main cache win. The Ubuntu
build,tests, andcheck_standalonecaches are each around 1.1 GB and are mostly the same dependency closure under different names. Folding them into one entry per OS would get a good chunk of that 10 GB back.Splitting up the check job
For the check job, I'd like to propose splitting its two kinds of work. It currently bundles quick checks that don't compile anything (formatting, license header, machete, and so on) with clippy, which compiles the whole dependency graph. Pulling the non-compiling checks into their own
lintjob:lint(Ubuntu)Cargo.lockcheck, cargo-machete, typoscheck's non-clippy steps, plus the separate Typos workflowWith nothing to compile,
lintwould fail a formatting nit or typo in seconds instead of behind a full build. We could also install taplo and cargo-machete as prebuilt binaries viataiki-e/install-action(already used for cargo-nextest) instead of building them from source each run, saving ~2.5 minutes, and pull in the typos check that currently sits in its own workflow.In this proposal, clippy would stay its own job, but run on Ubuntu only. Today it runs on both, and the macOS run is the single longest job on a green PR at around 10-12 minutes, versus ~6-7 on Ubuntu, almost all of it clippy. No code in the repo is gated on
target_os/unix/windows, so clippy's output is identical on both and the macOS run is just redundant lint work (the compile coverage there stays viabuild). Dropping it takes that 10-12 minute job off the critical path, and since it's the one change that removes a per-platform run, it's the piece I'd most want input on.Feedback welcome
Mainly looking for thoughts on:
Happy to adjust any of this.
Beta Was this translation helpful? Give feedback.
All reactions