First Cabal code contribution

introduction

A few weeks ago Shae Erisson and I took a look at Cabal to try to find a small contribution we could make. And we managed it! Here’s the story.

First, an introduction. Cabal is a system for building and packaging Haskell libraries and programs. A few years ago I started learning Haskell in earnest. After a little time spent with stack, a similar tool, I’ve more recently switched to using Cabal and have gotten a great deal of value from it.

I did contribute a trivial documentation improvement earlier this year but before the experience related here I hadn’t looked at the code.

Shae and I had a hunch that we might find some good beginner tasks in the Cabal issue tracker so we started there.

I had previously noticed that the Cabal project uses a label for supposedly “easy” issues. Shae also pointed out a group of labels related to “new comers”. We found what appeared to be a tiered system with issues group by the anticipated size of the change. For example, there is a label for “one line change” issues and another for “small feature” and a couple more in between. We looked at the “one line change” issues first but the correct one line change was not obvious to us. These issues have a lot of discussion on them and we didn’t want to try to figure out how to resolve the open questions. We decided to look at a more substantial issue and hope that there would be less uncertainty around them.

The candidate we found, Use ProjectFlags in CmdClean, was labeled with “one file change” and looked promising. It didn’t have a description (presumably the reporter felt the summary made the task sufficiently obvious) but there was a promising hint from another contributor and a link to a very similar change made in another part of the codebase.

dev env

While we were looking around, Shae was getting a development environment set up. The Cabal project has a nice contributor guideline linked from the end of their README that we found without too much trouble. Since Shae and I both use Nix, Shae took the Nix-oriented route and got a development environment using:

nix develop github:yvan-sraka/cabal.nix

(I’ll replicate the disclaimer from the contributor that this is “supported solely by the community”.)

This took around 30 minutes to download and build (and Shae has pretty fast hardware). A nice improvement here might be to set up a Nix build cache for this development shell for everyone who wants to do Nix-based development on Cabal to share. I expect this would take the first-run setup time from 30 minutes to just one or two at most (depending mostly on download bandwidth available).

By the time we felt like we had a handle on what code changes to make and had found some of the pieces in a checkout of the project the development environment was (almost) ready. The change itself was quite straightforward.

the task

Our objective was to remove some implementation duplication in support of the --project-dir and --project-file options. Several cabal commands accept these options and some of them re-implement them where they could instead be sharing code.

We took the existing data type representing the options for the cabal clean command:

data CleanFlags = CleanFlags
  { cleanSaveConfig :: Flag Bool
  , cleanVerbosity :: Flag Verbosity
  , cleanDistDir :: Flag FilePath
  , cleanProjectDir :: Flag FilePath
  , cleanProjectFile :: Flag FilePath
  }
  deriving (Eq)

And removed the final two fields which exactly overlap with the fields defined by ProjectFlags. Then we changed the type of cleanCommand. It was:

cleanCommand :: CommandUI CleanFlags

And now it is:

cleanCommand :: CommandUI (ProjectFlags, CleanFlags)

Finally, we updated nearby code to reflect the type change. liftOptionL made this straightforward since it let us lift the existing operations on CleanFlags or ProjectFlags into the tuple.

One minor wrinkle we encountered is that ProjectFlags isn’t a perfect fit for the clean command. Specifically, the former includes support for requesting that a cabal.project file be ignored but this option doesn’t make sense for the clean command. This was easy to handle because someone had also already noticed and written a helper to remove the extra option where it is not wanted. However, this might suggest that the factoring of the types could be further improved so that it’s easier to add only the appropriate options.

At this point we thought we were done. Cabal CI proved we were mistaken. The details of the failure were a little tricky to track down (as is often the case for many projects, alas). Eventually we dug the following error out of the failing job’s logs:

Error: cabal: option `--project-dir' is ambiguous; could be one of:
  --project-dir=DIR Set the path of the project directory
  --project-dir=DIR Set the path of the project directory

Then we dug the test command out of the GitHub Actions configuration to try to reproduce the failure locally. Unfortunately, when we ran the command we got far more test failures than CI had. We decided to build cabal and test the case manually. This went smoothly enough and we were quickly able to find the problem and manually confirm the fix (which was simply to remove the field for --project-dir in a place we missed). GitHub Actions rewarded us for this change with a green checkmark.

the rest

At this point we waited for a code review on the PR. We quickly received two and addressed the minor point made (we had used NamedFieldPuns, of which I am a fan, but the module already used RecordWildCards and it’s a bit silly to use both of these so close together).

Afterwards, the PR was approved and we added the “squash+merge me” label to trigger some automation to merge the changes (which it did after a short delay to give other contributors a chance to chime in if they wished).

In the end the change was +41 / -42 lines. For a refactoring that reduced duplication this isn’t quite as much of a reduction as I was hoping for. The explanation ends up being that importing the necessary types and helper functions almost exactly balanced out the deleted duplicate fields. Considering the very small scope of this refactoring this ends up not being very surprising.

Thanks to Shae for working on this with me, to the Cabal maintainers who reviewed our PR, to all the Cabal contributors who made such a great tool, and to the “Cabal and Hackage” room on matrix.org for answering random questions.