Reduce amount of CI round-tripping required by contributors

[mpickering]

There is currently quite a lot of back-and-forth which is required when contributing patches to Cabal which makes the developer experience sometimes cumbersome.

In my mind, the goal should be to perform 1 round-trip with CI. A developer submits a MR, sees ALL issues with the patch and can update the patch once in order to fix all the problems. Ideally all the issues are reproducible locally so that iteration can be performed locally on a specific test.

Off the top of my head here are some reasons why you might have to perform multiple CI rounds:

• The changelog.d template has a MR number field, which can only be filled in after a MR is created.
• Formatting often initially fails for contributors who are not aware of the formatting requirement.
• Tests such as the structured hash test require a developer to copy and paste a value from CI into their local tree. This is different on different GHC versions so it's difficult to update locally.
• If one CI job fails (for example a windows job), then all other jobs are cancelled, which can hide further failures on other platforms or versions.
• Tests are run on CI sequentially, some tests are not run if earlier tests fail (for example, unit tests run before the package tests). In a bad situation you may do multiple rounds if a unit test fails and then a package test for ./Setup and then a package test for cabal-install.

There is also the suggestion of adding a file which tracks the API of Cabal and Cabal-syntax (#10259) which would add another bullet point to this list.

In order to fix these issues, here are some possible suggestions:

• Remove tests such as structured hash (I'm not sure what the purpose of this test is as it is blindly updated)
• Do not cancel concurrent jobs if one CI job fails
• Always run all tests, don't bail out early if one testsuite fails.
• Remove MR field from changelog.d entry, require each commit to reference an issue. (At which point the issue will link to the MR automatically)

[ffaf1]

require each commit to reference an issue

We do but it is not enforced strictly.

[geekosaur]

The changlog.d template has a MR number field, which can only be filled in after a MR is created.

It's possible to predict these, actually: the PR number is shared with issue numbers, but otherwise your only problem is someone else getting an issue or PR in before you do. I haven't specifically dealt with this, but multiple times have predicted links to PRs I'm about to put up.

[geekosaur]

Remove tests such as structured hash (I'm not sure what the purpose of this test is as it is blindly updated)

These tell us when core structures change, which among other things is an indication that a changelog.d entry is needed and that it will invalidate people's existing dist or dist-newstyle directories.

[mpickering]

An example from my recent PR: https://github.com/haskell/cabal/pull/10287

The job is killed after lib-tests runs, so I have no idea if there are going to be more failures in subsequent testsuites.

Why not always run all the tests and report all the failures at once to a user?

[ulysses4ever]

@mpickering I think there's no strong opposition to what you propose — just a matter of cooking a PR flipping the right switch. If memory serves, @andreasabel worked a bit on orchestrating CI, maybe he has an opinion?

[fgaz]

iirc it was done to conserve resources. However since then we got more free ci time and parallelism from github, so changing it should be fine.

[andreasabel]

I agree. I generally use fail-fast: false because:

• I want to see all problems CI can uncover as soon as possible.
• Often jobs fail for random reasons (like some web resource not available, some ill-configured host machine) and then they drag other jobs into the abyss.

[ulysses4ever]

Thanks all! I submitted https://github.com/haskell/cabal/pull/10291 setting fail-fast: false on the main validate job. Please, review.

[AndreasPK]

I wonder how big the benefit of enforcing a formatter automatically is instead of requesting it for larger patches manually.

I've set out to put up the rather simple mr here: https://github.com/haskell/cabal/pull/10488 today and besides the old base branch issue my experience was:

• Push - ci fails - realize you need to use fourmolu to format your code.
• Install fourmolu - Ask about the guide listing 0.12 on matrix and get told it's the wrong version of the formatter because exactly 0.12 is required.
• Install the right version - format using vscode- ci still fails for some reason. (A cabal dev on the MR later pointed out this was because hls defaults to using the built in fourmolu which is 0.14).
• Get recommended to use make style, so I use this instead and it works.
• But the earlier run using the wrong version of fourmolu made some layout worse.
• Manually revert that specific change, finally all is well.

The benefits for regular contributors might well outweigh the cost of such friction. But if how a formatter is encouraged/recommended/required/enforced is reevaluated in the future I wanted to put this down here so it can be considered as an additional data point.

[geekosaur]

I think automatic code formatting is a lousy idea, but I don't run this project. :/

[mpickering]

I think automatic code formatting is a lousy idea, but I don't run this project. :/

As the most active contributor to the project, I would certainly hope your opinions about how things should be done were taken seriously. Thanks for all your work recently!

[geekosaur]

I just proposed on Matrix that, if we're going to do this at all, we should change CI from "complain" to "fix" like we did for the Whitespace job.

[ulysses4ever]

And I just opined on Matrix that having CI fix anything will run into endless merge conflicts on contributors' side.

[mpickering]

Thank you everyone for your help with this issue, it feels like the process is much improved now for contributing. 👍