Skip to content

Have CI check for diffs after integration tests #5798

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 8, 2025
Merged

Have CI check for diffs after integration tests #5798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2095bc9
Have CI check for diffs after integration tests
sellout Jul 8, 2025
70e0ce8
Add ci.yaml to the cache key for ci.yaml
sellout Jul 8, 2025
44f481f
Fix the failure revealed by the CI improvements
sellout Jul 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 31 additions & 31 deletions .github/workflows/ci.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,24 @@ This currently will re-save on success or failure, but only on a cache miss (sou

#### `ucm_local_bin`
A built `ucm` is cached in `ucm_local_bin` after a successful build and Haskell tests pass.
- The **cache key** includes the os, `stack.yaml`, any `package.yaml`, and any `.hs` file.
- The **cache key** includes the os, this workflow, `stack.yaml`, any `package.yaml`, and any `.hs` file.
- On an exact cache hit, these steps are skipped, otherwise they are run:
- restore `.stack`
- restore `.stack-work`
- install `stack`
- build `ucm` dependencies
- build `ucm`
- `unison-cli` tests
- `unison-core` tests
- `unison-parser-typechecker` tests
- `unison-sqlite` tests
- `unison-syntax` tests
- `unison-util-bytes` tests
- `unison-util-cache` tests
- `unison-util-relation` tests
- `cli-integration-tests`
- verification of `stack ghci` startup
- `interpreter-tests.md`
- restore `.stack`
- restore `.stack-work`
- install `stack`
- build `ucm` dependencies
- build `ucm`
- `unison-cli` tests
- `unison-core` tests
- `unison-parser-typechecker` tests
- `unison-sqlite` tests
- `unison-syntax` tests
- `unison-util-bytes` tests
- `unison-util-cache` tests
- `unison-util-relation` tests
- `cli-integration-tests`
- verification of `stack ghci` startup
- `interpreter-tests.md`

#### `unison_src_test_results`
A bit is cached in `unison_src_test_results` after non-Haskell tests in the `unison` repo pass.
Expand All @@ -65,24 +65,24 @@ No steps are skipped on a cache hit; however, a second `pull` will mostly be a n
JIT sources are cached in `jit_src_scheme` if the `generate-jit-source` job completes.
- The **cache key** includes the version of Racket, and the release version of `@unison/internal`.
- If the cache contains `{data-info, boot-generated, simple-wrappers, builtin-generated, compound-wrappers}.ss`, then these steps are skipped, otherwise they are run:
- "create transcript" to produce pull `@unison/internal` and run `generateSchemeBoot`.
- download `ucm artifact` saved in the previous step
- set `ucm` permissions
- checkout `unison` repo, which includes some static scheme and racket files.
- run the previously generated transcript
- "create transcript" to produce pull `@unison/internal` and run `generateSchemeBoot`.
- download `ucm artifact` saved in the previous step
- set `ucm` permissions
- checkout `unison` repo, which includes some static scheme and racket files.
- run the previously generated transcript
- If all steps succeed, the `jit_src_scheme` cache is saved.

#### `jit_dist`
JIT binaries are cached in `jit_dist` if the `build-jit-binary` job completes.
- The **cache key** includes the version of Racket, and the release version of `@unison/internal`.
- On an exact cache hit, these steps are skipped, otherwise they are run:
- Restore Racket dependencies
- setup Racket
- restore apt cache (Linux only)
- download jit source from previous job
- use `raco` to build jit binary
- download `ucm` artifact from previous job
- set `ucm` permissions
- restore `base` codebase saved in previous job
- jit integration test
- Restore Racket dependencies
- setup Racket
- restore apt cache (Linux only)
- download jit source from previous job
- use `raco` to build jit binary
- download `ucm` artifact from previous job
- set `ucm` permissions
- restore `base` codebase saved in previous job
- jit integration test
- If all of these steps succeed, the `jit_dist` cache is saved.
7 changes: 5 additions & 2 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ jobs:
uses: actions/cache@d4323d4df104b026a6aa633fdb11d772146be0bf # v4
with:
path: ${{env.ucm_local_bin}}
key: ucm-${{ matrix.os }}-${{ hashFiles('**/stack.yaml', '**/package.yaml', '**/*.hs', '**/unison-cli-integration/integrationtests/IntegrationTests/*')}}
key: ucm-${{ matrix.os }}-${{ hashFiles('**/ci.yaml', '**/stack.yaml', '**/package.yaml', '**/*.hs', '**/unison-cli-integration/integrationtests/IntegrationTests/*')}}
Copy link
Contributor

Choose a reason for hiding this comment

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

@sellout

Not sure where the best place to have this conversation is, but I'll put it here.

So I'm reluctant to include **/ci.yaml here, even though it's certainly more correct, because it causes any (possibly unrelated) change to the ci process — and there's often dozens of these when trying to add or modify any feature — take 45-60+ mins to evaluate.

Any ideas for how to best deal with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the commit log, this file changes like three times a month, but that overlooks how many commits may have been pushed (and possibly edited) but never made it into trunk. So yeah, I can see it being churny within any of those changes, as you mostly have to iterate on CI directly.

I think the first thing to consider is whether it can be broken into independent workflows. It looks like there’s a legit dependency graph there, so I guess it’s unlikely (but, as an example, here it is before I moved the ormolu job a couple months ago).

Extracting more complicated CI steps into scripts that can easily be run locally can help reduce the churn of CI files, since they can more easily be tested & iterated on locally. But then those scripts should also be part of the hash, or else it just moves the same problem. And I am a fan of small jobs, but combining sequences of jobs into one can make this easier, too. E.g., on this run, it looks like “generate jit source”, “build jit binary”, and “test jit” are a sequence. They’re not quite, because “generate jit source” is one job, and then “build jit binary” is a matrix that all depend on that. But “built jit binary” and “test jit” seem like they could reasonably be a single job.

It might just be best to just leave **/ci.yaml out, with a comment in case someone else (or me next month) comes back to add it again. That’s probably enough. If there’s some way to clear a particular cache entry1, that could also be mentioned in the comment. But I think mostly people won’t notice. I only did because I made a change that I expected to cause a CI failure (before pushing the fix), and it didn’t fail.

Of course, there’s also just performance improvement, which is nice because it generally helps developers and/or users, too, but that’s usually motivated the other way around – because users or developers are frustrated.

Footnotes

  1. Maybe a manually-triggerable workflow? But then that key value needs to be kept in sync between multiple workflows, which seems just likely to re-introduce the issue that Revert "Have CI check for diffs after integration tests" #5801, Revert "Revert "Have CI check for diffs after integration tests"" #5802, & don't use actions/cache to share artifacts between jobs #5804 are solving. Unless CI moves to some templating approach, which is a bigger lift. ↩

# added the integration test dependencies here as if they were source, for simplicity

- name: restore stack caches
Expand Down Expand Up @@ -161,7 +161,10 @@ jobs:

- name: cli-integration-tests
if: steps.cache-ucm-binaries.outputs.cache-hit != 'true'
run: stack exec cli-integration-tests
run: |
stack exec cli-integration-tests
# Fail if any transcripts cause git diffs.
git diff --ignore-cr-at-eol --exit-code unison-cli-integration/integration-tests

- name: verify stack ghci startup
if: runner.os == 'macOS' && steps.cache-ucm-binaries.outputs.cache-hit != 'true'
Expand Down
2 changes: 1 addition & 1 deletion unison-cli-integration/integration-tests/IntegrationTests/transcript.output.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ main = do
I found and typechecked these definitions in scratch.u. If you
do an `update`, here's how your codebase would change:

⍟ These new definitions are ok to `update`:
⍟ New definitions:

structural ability Break
type MyBool
Expand Down
Loading