Cabal and Cabal-syntax API checking

[geekosaur]

Not checking cabal-install or cabal-install-solver currently because they don't have public APIs. This may be incorrect for cabal-install-solver, so that may be added later.

Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

• Patches conform to the coding conventions.
• Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

There should be no need for manual QA, because it's already been tested by multiple rebases on master which changed API (for example, #10270).

[ulysses4ever]

Wow, this is awesome! 🤩

[geekosaur]

Do you happen to know whether cabal-install-solver needs to be included here, since it's a library? My assumption is that it's internal and as such has no public API to be checked.

[geekosaur]

Also, this is very much WIP as I sort out what's needed (and deal with weirdshit like the fact that I somehow got a byte order mark the first time, and subsequent runs don't like it much).

[ulysses4ever]

Cabal and Cabal-syntax are mission critical. The solver package is not (e.g. reverse dependencies count from Hackage: 259 for Cabal vs 1 for solver). So it's fine to start with those.

[geekosaur]

Just made the current API dumps into artifacts, since not everyone will have an Ubuntu box to run print-api on and sufficiently large changes would be a PITA working from diff output.

[Kleidukos]

@geekosaur why not use GHC 9.10.1? :)

[geekosaur]

Originally I was thinking 9.4.8 for consistency with GHC_FOR_RELEASE, then discovered that wasn't supported so I bumped it. I could do it again with 9.10.1 if that's preferred.

[Kleidukos]

Yes, let's use the highest supported GHC now and bump it once it falls out of the support window

[mpickering]

Please can we provide a means to easily update these files with a single command locally?

Ideally it should be possible to run all cabal tests locally with a single command and update the output within the same framework. This is already an issue with the structured hash test, which is difficult to update and dependent on GHC version, and in practice, just more annoying busywork than informative for contributors.

Otherwise, I don't think this job should be required by contributors as it introduces a significant amount of friction. It could be run as part of CI to track how the API has changed over time rather than requiring a golden file to update or perhaps this test could just be run on release branches (where the API is not expected to change).

[sheaf]

I think having such an API test is going to be very valuable, as it allows us to confidently assess the API impact of PRs under review. This will give us much more confidence and allow us to write more accurate changelogs, and avoid accidental changes in minor Cabal version etc. I'm grateful that the Cabal project is taking such a step.

However, I share Matthew's concern about the workflow for accepting the test. At the moment one has to provision a docker container to run with the appropriate environment for the test. Either that, or one ends up with a situation like the structured hash test where one has to wait for CI to run, let it fail because of the test, then change the stdout locally, then push again. If one needs one more CI run than one did before then I think it is too onerous; I would much rather that the structured hash test and this test be separate tests which don't cause the whole CI pipeline to fail if they fail, so that they can be updated in parallel with working on the PR without cancelling the CI which might otherwise have valuable information.

I also thing the surface area we are testing is a bit too large (I don't think the Cabal project has any guarantees about the entire set of exposed modules; that seems too much). As a result, I'm worried that we will get unreadable diffs as a result whenever there is a significant refactor. But I suppose we can wait and see how things pan out in that regard.

[Kleidukos]

I think having such an API test is going to be very valuable, as it allows us to confidently assess the API impact of PRs under review. This will give us much more confidence and allow us to write more accurate changelogs, and avoid accidental changes in minor Cabal version etc. I'm grateful that the Cabal project is taking such a step.

👍

However, I share Matthew's concern about the workflow for accepting the test

We all do, I don't think anyone has objected publicly or privately.

I also thing the surface area we are testing is a bit too large (I don't think the Cabal project has any guarantees about the entire set of exposed modules; that seems too much). As a result, I'm worried that we will get unreadable diffs as a result whenever there is a significant refactor. But I suppose we can wait and see how things pan out in that regard.

Yep, configuring the tool with an "ignore" list is in the plans. Cabal is just an early adopter and the needs of adopters will help determining how print-api & diff-package-api can provide the most value. Example: @geekosaur had to bypass the pre-made Workflow in order to test multiple packages at once.

[geekosaur]

However, I share Matthew's concern about the workflow for accepting the test. At the moment one has to provision a docker container to run with the appropriate environment for the test.

Actually no: as a first cut, the new API files are artifacts which can be downloaded and copied over the existing ones, specifically for this case. If there's some friendly way to make it completely automatic, I'm all ears, but I did specifically address that "contributors need to replicate the CI environment" issue. (And will shortly be using it myself, since I'll be redoing the golden API files for 9.10.1.)

[geekosaur]

Otherwise, I don't think this job should be required by contributors as it introduces a significant amount of friction. It could be run as part of CI to track how the API has changed over time rather than requiring a golden file to update or perhaps this test could just be run on release branches (where the API is not expected to change).

As semi-official release manager for the 3.12 branch, I would like this to run on every incoming PR. I do need to tune that though, as I don't want API changes to prevent merges; I want them to inform me whether a given PR is a backport candidate or not. As I'm still learning the details of workflows, I should probably put this back in draft even though I'm soliciting reviews.

[geekosaur]

Uh, let me correct that: the test should always pass, this is accomplished by updating the golden tests by copying the new API files over the old ones as described above. Then the presence of such updates tells me as release manager that the PR shouldn't be backported.

I think my ideal for both this and the structured hashes is that they'd be checkboxes in the PR that the tests could inspect in order to determine whether to report a mismatch or do an update. But

1. AFAICT there's no way to inspect PR checkboxes from workflows;
2. the multiple templates would make this difficult anyway.

[geekosaur]

Turns out I lost the last batch of changes (forgot to unstash them after rebasing other PRs) so I ended up redoing them, then discovered that the Makefile rules wouldn't work as written because make doesn't know the dependencies, cabal does.

There is still one issue with local API generation, which is that print-api assumes your default ghc is the one it's linked against.

[Kleidukos]

@Kleidukos, is there a way to tell print-api to use a different ghc? It turns out I can't run it on my system without switching my default ghc to 9.10.1 (it's normally 9.6.6). Everything else I can pass -w ghc-9.10.1 to and it works as expected.

Yes, we are still figuring out the ergonomics with @mmhat. He found out that using ghcup run --ghc 9.10.1 -- <your command> works. Does that work for you as a temporary solution?

[geekosaur]

It does seem to work, although it also looks like it's unnecessarily rebuilding something.

[geekosaur]

@Kleidukos, @ffaf1, not sure if this is something in print-api or something we missed in #10309, but I just got the following:

print-api: <no location info>: error: [GHC-87110]
    Could not load module `Distribution.Backpack'.
    It is a member of the hidden package `Cabal-syntax-3.13.0.0'.
HasCallStack backtrace:
  finally, called at compiler/GHC/Utils/Panic.hs:303:7 in ghc-9.10.1-69c3:GHC.Utils.Panic

Since I just rebased on master, this should want 3.15.0.0, no?

(ETA: never mind, had to remove dist-newstyle-apigen-9.10.1.)

[geekosaur]

(despite the labels, this is parked until post-ghc-9.12.1)

If/when this does go in, "Check API Post Job" will need to be added to the required jobs.