ci: move the Rust CodeQL analysis off the pull-request path - #1151
Closed
kvinwang wants to merge 5 commits into
Closed
ci: move the Rust CodeQL analysis off the pull-request path#1151kvinwang wants to merge 5 commits into
kvinwang wants to merge 5 commits into
Conversation
`rust-checks` is the largest job in the repository at ~12 minutes, it is a required status check, and it was compiling the workspace from an empty target directory on every run. Every other Rust job in the repository is in the same state except two: the gateway suites, and `simulator-release.yml`, which has been using `Swatinem/rust-cache` all along. This uses the same action the same way. `workspaces: dstack` because the manifest is not at the repository root, and the action keys the cache on the lockfile it finds there.
…moved Two changes to the same job, both about work it did not need to do. The cache is the same one `rust-checks` just got; `sdk/rust` is a separate workspace from `dstack` and gets its own entry. The filter is a step, not a `paths:` on the workflow. `sdk-tests` is a required status check: a workflow skipped by `paths:` reports nothing at all, so the check never arrives and the pull request waits on it forever. Gating the expensive steps inside a job that always runs keeps the report and drops the work -- a documentation-only pull request costs a checkout and a diff. The pattern includes `dstack/` because `sdk/run-tests.sh` starts the simulator, which is built from that workspace: these suites exercise the agent's wire surface, not only the client libraries. The whole directory rather than the simulator's dependency closure, which is a dozen crates deep and would go stale the first time one of them moved.
Neither is a required status check, so a workflow-level `paths:` filter is safe here -- a pull request that skips them is not left waiting on a check that never arrives. Docker Build Check verifies that the three builder images still build and that their pinned package lists still match, which cannot change unless something under `dstack/` does. It is three jobs of roughly eight minutes each and it was running on every pull request, including documentation-only ones. The images also get a layer cache, scoped per image. It covers the pinned-package install and the toolchain setup and stops there: the cargo build cannot be cached, because the source arrives through `git clone` at DSTACK_REV inside the build rather than from the build context, so BuildKit has nothing to key it on. That is deliberate -- the revision is what the image records in /etc/.GIT_REV -- so the ceiling is the layers above the clone. The VMM UI build is a minute and only ever concerns `dstack/vmm/ui`.
Both matrix legs call `simulator_start`, which builds `dstack-guest-agent-simulator` out of the current tree. Same binary, same commit, compiled twice in parallel. The legs still build it themselves -- nothing here replaces that call or hands them a binary from elsewhere, so what they test is still what the commit produces. A `simulator` job runs first and populates the cargo cache under a key both legs restore, so the build they run finds its work already done. A skip switch in `simulator_build` would have been shorter and would have created a path where the binary under test did not come from the checkout, which is not worth a few minutes. The workflow also gains a `paths:` filter. `dstack/**` rather than the guest-agent directories alone: the closure reaches a dozen crates, and naming the obvious three would leave the rest silently uncovered.
DO NOT MERGE THIS COMMIT WHILE CODE SCANNING DEFAULT SETUP IS ENABLED. GitHub refuses uploads from an advanced configuration while the default one is active, so the setting has to be turned off first -- Settings -> Code security -> Code scanning -> CodeQL analysis -> Disable. The file says so at the top as well. It is the last commit in the series so it can be dropped on its own. `Analyze (rust)` is 17.4 minutes, the single largest job in the repository, larger than the full Rust test suite, because CodeQL builds the whole workspace under its tracer. The other five languages together come to about five minutes. Rust moves to pushes on `next` and to the weekly schedule. Every finding it would have made is still made, on the merge commit rather than on the pull request that produced it -- reported one merge later, not missed. The cheap languages keep running per pull request. `c-cpp` is dropped: the repository contains one C file, `tools/sca/examples/hello-c/src/server.c`, which is example material for a supply-chain-analysis demo and ships in nothing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The cost
Analyze (rust)is 17.4 minutes on every pull request — the single largestjob in the repository, larger than the full Rust test suite — because CodeQL
builds the whole workspace under its tracer. Measured across the six analyses on
one push:
96% of the CodeQL time is one language, and default setup offers no way to
schedule it differently — the only lever is an advanced configuration.
What changes
Rust moves to pushes on
nextand to the weekly schedule. Everything it wouldhave found is still found, on the merge commit rather than on the pull request
that produced it: a regression is reported one merge later, not missed. The
five cheap languages keep running per pull request, where they cost little and
catch things early.
c-cppis dropped. The repository contains exactly one C file,tools/sca/examples/hello-c/src/server.c, which is example material for asupply-chain-analysis demo and ships in nothing.
Everything else — query suite, the weekly cadence, the remaining languages — is
what default setup was already doing.
To land it
or
then merge. The
Analyze (*)check names stay the same, so the two required-checkrulesets need no edit (neither lists a CodeQL check today).
If the trade is not wanted, close this and #1150 stands on its own.