Fix the Ormolu auto-format GH workflow step #5641

sellout · 2025-04-02T20:47:42Z

Overview

The Ormolu check in CI was not working. Now it does.

Implementation notes

There were a few issues:

The previous step (which collects the names of changed files) was missing its id, so the Ormolu step couldn’t retrieve the list of changed files.
That same step was also escaping the newline separator cancelling it out.
The job was passing, even if there was unformatted code, if it couldn’t commit the changes.

Interesting/controversial decisions

Fixing the first two issues is pretty straightforward, but the third one isn’t.

The job can’t create a commit on either protected branches, or on a fork. So, third-party contributors & their reviewers get no feedback that their code isn’t formatted correctly. However, the job runs on both PRs and pushes. The push job actually runs on the fork itself, and thus can create the commit.

So, you push everything once, and make a PR? Bob’s your uncle – the autoformatting gets done on your push job and the PR job either silently fails to format or succeeds because the automated commit has already happened.

Except … if you push incrementally, or add additional commits after the initial push, the push job will only look at the last commit (not even the entire push), whereas the PR job always looks a the entire PR. So, the confusing situation I found myself in was that when I first put up the PR, the auto-commit happened (based on my push, not the PR), but later commits didn’t have the same effect, and the PR jobs (which are the only ones I realized were happening) just skipped the “push a commit” step.

So, the way I finally did it is to run Ormolu inplace and make the commit if possible, but to otherwise run Ormolu check and fail the job if formatting was incorrect. From that, I expect contributors to see one of these behaviors:

they push a single commit, the job runs and adds the formatting commit, then they open the PR, and it passes;
they push a single commit, open the PR before the job runs, and the PR fails its Ormolu check, but then the fork adds the formatting commit, triggering another PR run, which then passes; or
they push more than one commit, any formatting changes in files modified in the last commit get auto-fixed, but changes in other files don’t, and the PR’s run catches those discrepancies and stays failing.

It’s not perfect, but it at least seems better than the previous situation that would just always pass and never make any changes.

Test coverage

This PR introduces a mis-formatting to trigger the auto- formatting in order to test the workflow change. It doesn‘t actually work that way though (see the previous section), so there is a final commit that fixes the build based on the failing ormolu check in CI.

Loose ends

Hopefully there’s a better way to manage the auto-commit thing. I would think having “Allow edits and access to secrets by maintainers” on the PR would be enough to allow the auto-commit to happen on the PR in some way, and having this done on pushes is probably wrong (but doing the rest of CI on pushes is probably nice, and we don’t want to throw out the baby with the bathwater) – so maybe the ormolu job should just be in a separate workflow or something?

The previous step (which collects the names of changed files) was missing its `id`, so the Ormolu step couldn’t retrieve the list of changed files. This commit also introduces some mis-formattings in the files modified by unisonweb#5640 to test those changes and this re-enabling of the workflow.

These two are still necessary, but overlooked because CI wasn’t actually running Ormolu.

Previously, only the last file ended up getting formatted, because the filenames ended up joined on a single line.

The Ormolu job apparently doesn’t work on forks or protected branches[^1], so have it do `check` instead of `inplace` in those cases. [^1]: I’m a bit confused by this, because when I put up an earlier version of the PR, it _did_ create a commit (which only formatted one of the files), but then I couldn’t get it to do the same on subsequent pushes.

aryairani · 2025-04-07T21:52:42Z

Oh, thanks. I was subconsciously wondering about this.

.github/workflows/ci.yaml

aryairani · 2025-04-11T16:43:34Z

.github/workflows/ci.yaml

        if: |
-          always()


I can't remember why this was here, but we may discover a reason later. It probably belongs somewhere else if anywhere,

Yeah, I couldn’t figure out why it would be useful to run this step if an earlier one failed.

I have a theory on this. I think always() was being used as a formatting guide, like using

true && command1 && command2

rather than

command1 && command2

aryairani · 2025-04-11T16:46:49Z

Can we detect when a push has multiple commits, and then run a full ormolu instead of the currently filtered to the files changed in the commit? Or maybe filter to the files changed in the PR?

aryairani · 2025-04-11T16:48:34Z

2. they push a single commit, open the PR before the job runs, and the PR fails its Ormolu check, but then the fork adds the formatting commit, triggering another PR run, which then passes; or

I don't think automatic commits trigger another run unfortunately. Am I wrong? Although it's okay as long as we're careful to check which tests ran before merging a PR

sellout · 2025-04-16T05:23:43Z

Can we detect when a push has multiple commits, and then run a full ormolu instead of the currently filtered to the files changed in the commit? Or maybe filter to the files changed in the PR?

I don’t know what kind of introspection we have.

I don't think automatic commits trigger another run unfortunately. Am I wrong? Although it's okay as long as we're careful to check which tests ran before merging a PR

No, you’re right – I always forget that.

I feel like having this job run on push is maybe just the wrong thing. Doing either check or inplace on the PR seems good enough. If a reviewer sees that it’s the Ormolu check that’s failing, they can run ormolu and push to the PR. It’s not perfect, but I think it’s a lot clearer. Maybe scripts/check.sh can also run ormolu if it finds it on the path, to help contributors notice before pushing?

aryairani · 2025-04-22T13:03:51Z

Can we detect when a push has multiple commits, and then run a full ormolu instead of the currently filtered to the files changed in the commit? Or maybe filter to the files changed in the PR?

I don’t know what kind of introspection we have.

I don't think automatic commits trigger another run unfortunately. Am I wrong? Although it's okay as long as we're careful to check which tests ran before merging a PR

No, you’re right – I always forget that.

I feel like having this job run on push is maybe just the wrong thing. Doing either check or inplace on the PR seems good enough. If a reviewer sees that it’s the Ormolu check that’s failing, they can run ormolu and push to the PR. It’s not perfect, but I think it’s a lot clearer. Maybe scripts/check.sh can also run ormolu if it finds it on the path, to help contributors notice before pushing?

I'm good with trying any/all of that — is there anything I should do or just sit tight?

This allows auto-committed formatting to only run on pull-requests, not all pushes.

This is also run by ./scripts/check.sh.

This shows the shortcomings of the previous push-based workflow. In bee1f7a, the push-based run was triggered, but it only ran on the files that were updated by the single merge commit prior to the automatic run. It didn’t affect files that were changed by other commits in the PR (like Stack.hs, which is updated here and caused the pull-request based check to fail).

sellout

Ok, I did a few of the things

there is now ./scripts/check-formatting, which will run Ormolu over the entire repo (and can either check or format)
./scripts/check.sh now runs check-formatting at the end if everything else has passed
the CI job now only runs on pull-requests, avoiding the unhelpful single-commit behavior of on: push

./scripts/check-formatting tries to be well-behaved – it won’t run an Ormolu other than the version we expect, and it won’t fail if there’s no Ormolu to run. Also, if the user has Nix, it’ll check that the script & versions.nix agree on the Ormolu version, but won’t complain if the user doesn’t have Nix.

sellout · 2025-05-12T22:46:24Z

.github/workflows/ci.yaml

        if: |
-          always()


I have a theory on this. I think always() was being used as a formatting guide, like using

true && command1 && command2

rather than

command1 && command2

aryairani · 2025-05-13T23:11:49Z

What do we think about the separate workflow idea? Run ormolu 1x/day on the whole codebase? and beyond that forget about making external contributors conform on formatting?

sellout · 2025-05-14T13:50:47Z

What do we think about the separate workflow idea? Run ormolu 1x/day on the whole codebase? and beyond that forget about making external contributors conform on formatting?

I think it sounds like a good compromise.

We could still run the --mode inplace step on PRs that can be updated¹, but wouldn’t block external contributors on a failed check. Maybe we could have a “continue on failure” step² on the external case, so we’d at least know that the formatting is off, but not let it block merging?

One thing that can be annoying when making changes is when format-on-save triggers a bunch of formatting outside of the code you’ve changed, making for noisier commits. But, with once-a-day formatting, you’re still much less likely to run into it than without auto-formatting, especially with it still being done on internal PRs.

I don’t understand why we can’t have autoformatting work on external PRs, since “Allow edits and access to secrets by maintainers” is enabled on most PRs – it feels like this should be an “edit by maintainers” sort of thing. ↩
Unfortunately, GithHub has been dragging its feet on this behavior for years. ↩

sellout · 2025-05-14T13:51:42Z

Should we start with what’s here, and see how much the Ormolu job gets in the way of contributions? Or is that just not a good experience for contributors?

aryairani

Ok let's try it!

aryairani · 2025-05-15T05:37:12Z

Should we start with what’s here, and see how much the Ormolu job gets in the way of contributions? Or is that just not a good experience for contributors?

It's not a good experience for contributors if the Ormolu job gets in the way, but since you're a contributor you can test it out for us ;-)

Though you're also a committer, so that may complicate things

sellout added 2 commits April 2, 2025 15:06

Restore a couple ORMOLU_DISABLE

a0ab7fe

These two are still necessary, but overlooked because CI wasn’t actually running Ormolu.

sellout force-pushed the fix-ormolu-ci branch 9 times, most recently from c0e26a2 to 1a5c9ed Compare April 2, 2025 21:33

Pass _all_ changed files to Ormolu

def095f

Previously, only the last file ended up getting formatted, because the filenames ended up joined on a single line.

sellout force-pushed the fix-ormolu-ci branch from 1a5c9ed to def095f Compare April 2, 2025 22:03

Fail Ormolu auto-format if can’t commit

3d33d92

sellout force-pushed the fix-ormolu-ci branch from e1873da to 3d33d92 Compare April 2, 2025 22:27

sellout force-pushed the fix-ormolu-ci branch 2 times, most recently from 943ddef to 23dc00e Compare April 2, 2025 23:28

sellout marked this pull request as ready for review April 2, 2025 23:35

sellout requested a review from a team as a code owner April 2, 2025 23:35

sellout mentioned this pull request Apr 2, 2025

Get rid of obsolete ORMOLU_DISABLE usage #5640

Merged

aryairani reviewed Apr 11, 2025

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

aryairani reviewed Apr 11, 2025

View reviewed changes

aryairani added 2 commits May 12, 2025 07:09

Merge branch 'trunk' into fix-ormolu-ci

2d3dc18

automatically run ormolu

bee1f7a

sellout added 4 commits May 12, 2025 16:53

Run Ormolu in a separate workflow

329d0db

This allows auto-committed formatting to only run on pull-requests, not all pushes.

Add a script to check & format with Ormolu

856a616

This is also run by ./scripts/check.sh.

Merge remote-tracking branch 'upstream/trunk' into fix-ormolu-ci

604f739

sellout commented May 12, 2025

View reviewed changes

aryairani approved these changes May 13, 2025

View reviewed changes

aryairani approved these changes May 14, 2025

View reviewed changes

sellout merged commit b6cc603 into unisonweb:trunk May 15, 2025
17 checks passed

sellout deleted the fix-ormolu-ci branch May 15, 2025 05:18

Fix the Ormolu auto-format GH workflow step #5641

Fix the Ormolu auto-format GH workflow step #5641

Uh oh!

Conversation

sellout commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Implementation notes

Interesting/controversial decisions

Test coverage

Loose ends

Uh oh!

aryairani commented Apr 7, 2025

Uh oh!

Uh oh!

aryairani Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

sellout Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

sellout May 12, 2025

Choose a reason for hiding this comment

Uh oh!

aryairani commented Apr 11, 2025

Uh oh!

aryairani commented Apr 11, 2025

Uh oh!

sellout commented Apr 16, 2025

Uh oh!

aryairani commented Apr 22, 2025

Uh oh!

sellout left a comment

Choose a reason for hiding this comment

Uh oh!

sellout May 12, 2025

Choose a reason for hiding this comment

Uh oh!

aryairani commented May 13, 2025

Uh oh!

sellout commented May 14, 2025

Footnotes

Uh oh!

sellout commented May 14, 2025

Uh oh!

aryairani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aryairani commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sellout commented Apr 2, 2025 •

edited

Loading

aryairani commented May 15, 2025 •

edited

Loading