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:

• [x] Patches conform to the coding conventions.
• [x] 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, https://github.com/haskell/cabal/pull/10270).

[ulysses4ever]

Wow, this is awesome! :star_struck:

[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

[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.

:+1:

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]

Okay, I've added documentation about the check API job and how to update the API in a PR. The actual API check also outputs a note about where to find the new API artifacts.

[geekosaur]

Also, I switched out diff for cmp, because with Cabal's 1MB+ API footprint nobody really cares about the details, just that it changed. If someone really wants it, they can download the new API files and run diff locally.

[geekosaur]

Ideally it should be possible to run all cabal tests locally with a single command and update the output within the same framework.

The problem with this is that GHC and base internals (in particular, basic types and typeclasses) are part of the API, which means the API depends on the GHC version and to some extent the platform print-api is run on. As such, contributors (unless they happen to be on Ubuntu 22.04 on x86_64) would need a Docker container, virtualbox, or similar to reproduce the exact API. (It can be argued that that's not strictly part of Cabal's API, but then we run into print-api not having a way to suppress such internals as yet.) They also might need to run it in a copy anyway or at least do a cleanup pass afterward, because print-api requires GHC environment files, which are known to cause confusing problems in many circumstances.

This is why I only check the API on a single platform and GHC version, and have tried to make it as easy as possible to update the golden API record when needed.

[geekosaur]

@ulysses4ever, I'm not currently finding your question about API vs. platform.

1. Different Linux distributions on the same architecture shouldn't have different APIs, but might require using a different print-api release package because of glibc or other shared library incompatibilities (the print-api release only includes one shared version for Linux, and the example usage on text-display strongly suggests it's for Ubuntu 22.04).
2. MacOS and Windows on the same architecture probably differ, because they have different ABIs and this can potentially show up in some of ghc's/ghc-prim's/ghc-base's types. Ideally print-api would exclude (or use placeholders for) these types, but I suspect that's firmly in ghc-api territory and may make print-api as difficult to support across GHC versions as HLS is (that is, sometimes breaking across minor versions that aren't supposed to affect normal programmer-visible API/ABI). (This depends on how deep print-api drills; it may not expand types down to the internals level, I haven't checked.)
3. Different architectures almost certainly will differ, for the same reason as (2). CInt on AArch64 vs. x86_64 is an example here: on x86_64 the decision was made to keep C's int 32 bit for backward compatibility, but IIRC AArch64 went with 64 bits, so any API using CInt (not that I expect Cabal to have such, hopefully!) would differ because the type alias will expand to Int32 vs. Int64.

[geekosaur]

Some things to consider for the future: (/cc: @Kleidukos)

• I have trouble believing Cabal's API is that large. How much of it should move to an internal library? (Having print-api ignore things defined in Cabal would be a workaround.)
  • But beware of things that people depend on because they're considered part of the API currently (see for example #9856)
• Dependencies can have Internal modules, which are excluded from the public API but still visible. print-api would need an option to ignore these, or possibly an annotation similar to the ones in Haddock (hide, not-home) — which might even be reusable here. Example: the text-display example includes an Internal module in its API record, which is almost certainly incorrect; it should be accounted to the public module that re-exports it.

ETA: actually, I'm not sure source annotations in a dependency can be used, because the source isn't examined? Haddock, on the other hand, needs to examine the source.

[mpickering]

I am against merging anything which requires downloading files from CI in order to make tests pass. This workflow is much too cumbersome for developers. The structured hash check is already bad enough when it comes to PRs and having to do multiple rounds.

If the goal is to stop API changes happening on stable branches, then it seems that running this job only on release branches is the way to go. At which point the test should never/rarely fail and the burden is placed on the release manager (who desires this check) to update the files rather than the developer whose goal is to fix a bug or write a feature.

[geekosaur]

I am against merging anything which requires downloading files from CI in order to make tests pass

Are you prepared to confirm that ghc doesn't force this on us by producing different types on different platforms?

Let me be more clear about this. print-api, and probably any other API reporter, needs to resolve types to their home modules because of things like strict vs. lazy ByteString, so they don't get confused. (And let's not even get started on Henning Thielmann's preferred style for types and typeclasses, which is guaranteed to be a nightmare here.) My worry is that there is no way to prevent this from also happening for types that come from the compiler/RTS/ghc-internals/ghc-base/etc. (Possibly ghc-bignum as well: does Integer's API footprint and/or home module change depending on your ghc build?) ghc-api can probably be used to do something about this, but that raises another difficulty: at least twice, I've been unable to rebuild HLS for a new ghc minor release because of a ghc-api change. Will this also affect anything that can report an API?

The two most problematic issues I expect are (a) platform-specific implementations of basic types, and (b) Integer as mentioned above. Other possibilities seem unlikely, as I said above.

[Bodigrim]

1. Different Linux distributions on the same architecture shouldn't have different APIs, but might require using a different print-api release package because of glibc or other shared library incompatibilities

It seems to me that print-api is small enough to be built from sources every time instead of relying on pre-built binaries.

3. Different architectures almost certainly will differ, for the same reason as (2). CInt on AArch64 vs. x86_64 is an example here

I don't quite follow this example: CInt is CInt, it's not a type synonym. Does print-api unwrap it for some reason?

[Kleidukos]

@Bodigrim I can give an answer for point N°2: CInt ought not to change for consumers of the type. Only its place of definition will produce different results on different platforms.

See the reason written by Ben Gamari here: https://github.com/Kleidukos/print-api/blob/main/src/PrintApi/IgnoredDeclarations.hs#L37

See an example of "redacted" definition: https://gitlab.haskell.org/ghc/ghc/-/blob/master/testsuite/tests/interface-stability/base-exports.stdout-javascript-unknown-ghcjs#L2141-2142

[Bodigrim]

CInt ought not to change for consumers of the type. Only its place of definition will produce different results on different platforms.

I still don't quite get it: as much as I can tell CInt is defined in the same place, independently of a platform. The RHS of the definition is platform-dependent indeed, but unless you run print-api on base itself, this should not be a concern, if I understand it correctly.

[Kleidukos]

@Bodigrim That is absolutely what I meant, indeed. :)

[geekosaur]

It seems to me that print-api is small enough to be built from sources every time instead of relying on pre-built binaries.

If the objective is to minimize extra work needed by contributors, I'm not sure that's the way to go. Ideally they'd be able to cabal install print-api.

[geekosaur]

The current version of this has Makefile rules to generate the API records locally, individually or together. (You need to install print-api yourself.) If the GHC ecosystem behaves itself then this should be acceptable. Next update will also include an update-api rule and a check-api rule.

[mpickering]

Perhaps print-api could be packaged as an external command, so that cabal print-api would deal with building and installing the executable, building the project appropiate and so on.

(For example, cabal-doctest, https://github.com/sol/doctest/blob/main/src/Cabal.hs)

[mpickering]

Thanks for the Makefile target, that's an improvement. I don't know if the output will be invariant across platforms. I had a brief look but I think that the scope of the checking needs to be dialed down a bit.. for example Distribution.Compat.Binary is clearly supposed to be an "internal" module and just reexports some definitions for convenience within the library. Another one with a lot of reexports is Distribution.Compat.Prelude.

[Kleidukos]

@mpickering This is exactly what the next release of the print-api will address, by enabling end-users to maintain an "ignore list" of modules. Another future version will even be enabled to work with Haddock module attributes. This PR will use the next version before getting merged, although discussion about ergonomics is important and informs the development of print-api.

[geekosaur]

I also have started a discussion somewhere (maybe above) about reviewing Cabal APIs that belong in private libraries instead of the public one, but I suspect that's a long term goal. (Oh, it's the first item here.)