Skip to content

Add configurable base image to workflows - #56

Open
jacob-curley-fnal wants to merge 2 commits into
mainfrom
build_with_image
Open

Add configurable base image to workflows#56
jacob-curley-fnal wants to merge 2 commits into
mainfrom
build_with_image

Conversation

@jacob-curley-fnal

@jacob-curley-fnal jacob-curley-fnal commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Make dev container the first-class build environment

This PR teaches our shared GitHub Actions workflows to use a repo's own .devcontainer/devcontainer.json as 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 grcov ceremony, 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):

  • Added a Check for dev container step that sets a present output flag.
  • All subsequent build/test/lint steps are conditioned on that flag, branching into a Dev Container path (devcontainers/ci@v0.3) or a Non-Container fallback.
  • The dev container path passes secrets as scoped environment variables rather than via global git config, reducing credential blast radius.

Flutter workflows:

  • Build context (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.
  • Removed touch .env steps that created empty files with no purpose.
  • The Docker build now copies the shared Dockerfile as Dockerfile (not flutter.Dockerfile), so docker/build-push-action can find it without an explicit file: argument — removing an implicit coupling between the copy step and the build step.

Rust workflows:

  • Replaced the grcov pipeline (manual RUSTFLAGS=-C instrument-coverage + LLVM_PROFILE_FILE + grcov post-processing) with a single cargo llvm-cov --lcov --output-path target/lcov.info call. Fewer moving parts, same output.
  • The static-dependencies input is marked **DEPRECATED** — repos should declare their build dependencies in a dev container instead.
  • CARGO_NET_GIT_FETCH_WITH_CLI is scoped to the dev container step rather than the entire job.
  • Removed the actions/cache@v5 Cargo cache step; testing indicated no significant difference in build times due to cargo needing to recompile everything anyway

Both ecosystems:

  • Lint output is now redirected to lints.txt and uploaded as a workflow artifact via actions/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-reporter bumped from @v2 to @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.

Comment thread .github/workflows/flutter-integration.yaml Outdated
@jacob-curley-fnal

Copy link
Copy Markdown
Contributor Author

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!

@rneswold

rneswold commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Will this speedup PRs, @jacob-curley-fnal? I fed our rust-integration.yaml (after verifying it didn't expose any secrets) to Gemini Pro and it found ways to speed things up. I was going to push a branch to get comments. But if the container speeds things up, I'll just drop the effort.

@jacob-curley-fnal

Copy link
Copy Markdown
Contributor Author

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.

@rneswold

rneswold commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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 taiki-e/install-action@v2, which has code coverage prebuilt.

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.

@rneswold

Copy link
Copy Markdown
Contributor

#57 shows the proposed changes. I don't suppose your changes will handle the issues mentioned in it.

@jacob-curley-fnal

Copy link
Copy Markdown
Contributor Author

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 .devcontainer/devcontainer.json configured in the repository!

@jacob-curley-fnal

Copy link
Copy Markdown
Contributor Author

@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. clippy and test produce different artifacts, so Cargo will always compile twice when linting is in the pipeline.

The deeper issue with Rust build caching is that most approaches, including Swatinem/rust-cache, key the cache on a hash of Cargo.lock. Any change to that file (including bumping the local crate version) invalidates the cache and forces a full rebuild of all dependencies. So, the cache only helps on runs where nothing has changed, which tends to be the minority of meaningful CI runs.

Excluding target/ from cached artifacts was intentional. Cargo doesn't prune old compilations from target/ without cargo clean, so a cached target/ directory grows unboundedly over time. To avoid that, you'd need to invalidate the cache frequently enough to keep it lean, but frequent invalidation means frequent full rebuilds, which defeats the purpose. I ran some tests and found that caching the registry source files alone didn't meaningfully reduce build time.

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 llvm-cov (which I've also added to the Rust devcontainer image), but it doesn't appear to be supported by that action yet, unfortunately.

@rneswold

Copy link
Copy Markdown
Contributor

FWIW, I realize clippy and unit tests require two builds. All the more reason to try to get the cache working.

@rneswold

rneswold commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Cargo doesn't prune old compilations from target/ without cargo clean, so a cached target/ directory grows unboundedly over time.

Actually, I thought recent versions of cargo do prune their cache: https://blog.rust-lang.org/2025/06/26/Rust-1.88.0/

But thanks for taking the time to explain your motivations. They make sense.

@jacob-curley-fnal

Copy link
Copy Markdown
Contributor Author

Actually, I thought recent versions of cargo do prune their cache

The blog gets into specifics:

Cargo will remove files downloaded from the network if not accessed in 3 months, and files obtained from the local system if not accessed in 1 month

So we'd be collecting old artifacts, including old build outputs, for at least a month before any get dropped.

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.

5 participants