Add configurable base image to workflows - #56
Conversation
bf9d3ef to
9bc1156
Compare
a92acdc to
bbb0d52
Compare
|
For a successful Rust build using the dev container, see https://github.com/fermi-ad/alarms-actions-synchronizer/pull/21. I have not used this in a Flutter context yet, will be running some test integrations and updating this PR with my results! |
|
Will this speedup PRs, @jacob-curley-fnal? I fed our |
|
I haven't looked too closely at speed comparisons. This is mostly an effort to harden our reproducibility and consistency across the CI pipeline and dev environments. By running in the same container we do development with, the CI pipeline is never going to show random failures due to toolchain drift or Linux kernel differences. It also lets us do things like preinstall the Rust llvm-cov dependency or the Flutter toolchain so we're not reinstalling that on every run of the pipeline. We'd talked about the speed consideration a bit among Software Group B. I come from a place where the average pipeline took ~1 hour to complete, with heavy days taking upward of 2 hours. And that's not including the time spent waiting for a job runner to become available (we had ~15 developers sharing 4 pipeline VMs). So, for me, all of our projects building in under ~10 minutes is really fast 😆 If folks around here find the build times to be too long, I'm open to trying to optimize that as best we can! But my bias is to avoid any optimizations that create discrepancies between how the pipeline builds and runs as compared to how we build and run in development. |
|
One example: In the Rust Integration, I see that it builds the code coverage tool every time. That means we're building three Rust targets: the code coverage tool once and extapi twice (once for clippy, once for unit tests.) Gemini found I also see it builds everything every time -- even though we supposedly use a cache. Gemini claims we didn't specify the proper target so it's not seeing the cache properly. So we're paying for unpacking and repacking the cache and storage space, yet we're building everything. |
|
#57 shows the proposed changes. I don't suppose your changes will handle the issues mentioned in it. |
|
I did some test integrations of the container-based workflow for flutter in the enclosure status project. The test PR is here: https://github.com/fermi-ad/enclosure-status-app/pull/41. I was able to validate that the pipeline ran successfully both with and without a |
|
@rneswold — to clarify, the changes in this PR aren't addressing the caching concern on the Rust side; in fact, I've removed caching from that pipeline altogether. Here's my reasoning: The previous setup cached the Cargo registry files but not the compiled code, which is why it was rebuilding every time anyway. The "double build" you noticed between the linting and testing steps is expected behavior. The deeper issue with Rust build caching is that most approaches, including Excluding More broadly, I'd argue that CI's primary value is a clean, reproducible build that gives us genuine confidence in the state of the code. Carrying stale artifacts forward from run to run introduces variables that won't exist in production or a fresh checkout, which can mask real issues. I'd rather have a pipeline that's a couple of minutes slower but consistently trustworthy than one that's faster but occasionally misleading. That said, I do like the idea of the installer action for avoiding redundant tool builds! The tool I've swapped in here is |
|
FWIW, I realize clippy and unit tests require two builds. All the more reason to try to get the cache working. |
Actually, I thought recent versions of But thanks for taking the time to explain your motivations. They make sense. |
The blog gets into specifics:
So we'd be collecting old artifacts, including old build outputs, for at least a month before any get dropped. |
Make dev container the first-class build environment
This PR teaches our shared GitHub Actions workflows to use a repo's own
.devcontainer/devcontainer.jsonas the canonical build environment, falling back to the existing runner-based approach only when no dev container is present.Why
The existing workflows accumulated complexity over time: environment variables were computed mid-step, lint output was base64-encoded to survive step boundaries, coverage required a multi-step
grcovceremony, and the build environment on the runner was subtly different from what developers used locally. Each of these was a small lie — the CI environment was not the same as the development environment, and we papered over the gap with workarounds.A dev container makes the environment explicit and version-controlled. When a repo ships a
.devcontainer/devcontainer.json, CI should simply use it. This PR makes that the default path.What changed
All four workflows (
flutter-deploy,flutter-integration,rust-deployment,rust-integration):Check for dev containerstep that sets apresentoutput flag.devcontainers/ci@v0.3) or a Non-Container fallback.git config, reducing credential blast radius.Flutter workflows:
BUILD_PATH), PWA strategy (PWA_STRAT), and app metadata (APP_VER,APP_NAME) are now extracted early and stored in$GITHUB_ENV, making them available to all downstream steps without threading step outputs through every reference.touch .envsteps that created empty files with no purpose.Dockerfile(notflutter.Dockerfile), sodocker/build-push-actioncan find it without an explicitfile:argument — removing an implicit coupling between the copy step and the build step.Rust workflows:
grcovpipeline (manualRUSTFLAGS=-C instrument-coverage+LLVM_PROFILE_FILE+grcovpost-processing) with a singlecargo llvm-cov --lcov --output-path target/lcov.infocall. Fewer moving parts, same output.static-dependenciesinput is marked**DEPRECATED**— repos should declare their build dependencies in a dev container instead.CARGO_NET_GIT_FETCH_WITH_CLIis scoped to the dev container step rather than the entire job.actions/cache@v5Cargo cache step; testing indicated no significant difference in build times due tocargoneeding to recompile everything anywayBoth ecosystems:
lints.txtand uploaded as a workflow artifact viaactions/upload-artifact@v7, replacing the fragile base64-encode → store as step output → decode-on-failure pattern. Lint failures print the file directly and exit cleanly.fermi-ad/code-coverage-reporterbumped from@v2to@v3.The result
Repos with a dev container get a CI environment that matches their local setup. Repos without one continue to work exactly as before. The workflows are easier to read because each section is clearly labelled and the branching is explicit rather than hidden in inline conditionals. Each piece of complexity that was removed was there to compensate for a mismatch between environments — and that mismatch is now gone.