Separate types/features/acid-state/backups across all features

[isovector]

Overview

As part of #1486, for each feature X this PR clearly separates:

• Features.X for the core HackageFeatures
• Features.X.Types for any domain-specific types that should persist after ripping out acid-state
• Features.X.State for any acid-state code (which will eventually be removed as part of Migrating away from acid-state #1486)¹
• Features.X.Backup for any CSV backup/restore code

These conventions area already used haphazardly across the codebase; this PR applies them across the board.

In addition, this PR also imports the Features.X.State qualified as Acid, making it clear to all downstream callers where precisely the acid-state is occurring.

Note to Reviewers

This is chopped out of a long-running branch I'm working on to remove acid-state. It's the first chunk of PR-appropriate work, but I'm happy to split it into smaller PRs if you'd like. Furthermore, the qualified as Acid imports are very helpful on my end, but might not be worth upstreaming. If you have concerns about that, I'm happy to remove those from this PR.

Footnotes

1. For the new modules that got this code split out, I used Features.X.Acid instead of Features.X.State. Happy to make this consistent if it's a concern during review. The reason to not simply use *.State is that I noticed ReverseDeps.State is a counterexample to the state modules being for acid-state. ↩

[ysangkok]

I see this has gotten force pushed a bunch. Is it ready for review or merge yet? Or do you want us to wait?

[isovector]

@ysangkok Sorry for the churn. I noticed some minor cleanup issues and was force pushing because of that. But the PR is ready for review!

[ysangkok]

Looks like the tests are not compiling...

[isovector]

@ysangkok fixed, sorry! I'm continually caught off guard that stack build doesn't build tests :)

[ysangkok]

@isovector When starting a Hackage-Server with an older database from before this PR, I get an error about AddAdminLog not being present. Are you able to reproduce this issue?

[isovector]

@ysangkok ugh, good catch. Digging in, it turns out that makeAcidic generates Method instances, which are implemented via showQualifiedTypeRep, which in turn bakes the module name. 🤦

Egregious state of affairs.

By my reckoning, there are two solutions here:

1. add template-haskell as a dependency to hackage-server, and write some TH that will give us a hook into how Method instances are generated
2. inline some TH splices and manually patch the instances

It might be tempting to consider 3. don't move this code, but that seems like a non-starter to me; once acid-state is no longer on the critical path, we're going to want to move it out of the library but keep it around for migrations, which will require solving this problem anyway.

Of these bad solutions, (2) seems less bad to me. It's less terrible than it sounds, since the inlined splices haven't had any changes in:

• AdminLog - 11 years
• UserNotify - 4 years
• UserSignup - 11 years
• Vouch - 3 years

I've added a new module Data.Acid.Compat which gives a GHC-style Note explaining the situation, and re-exports enough of Data.Acid and Data.SafeCopy to be able to immediately inline their splices, plus machinery to patch the necessary methodTag method.

[gbaz]

I think this is a reasonable solution, as long as we're confident we're on a path to moving away from this stuff altogether. The comments and documentation also suffice so that if we need to pull back we can do an effective revert of these four modules too.

But I also think "don't move the code" could be the best path. I mean the code will be somewhere until it isn't, and having it in an unexpected module isn't the end of the world. And when we want to get rid of it, we can still do so, no matter where it was. So unless there's a stronger reason I'm missing, I'd urge at least a reconsideration of or further discussion of "3".

[gbaz]

As I understand it, even with "3", the bulk of the PR is the same, no? It only means that we don't have code motion for four specific instances...

[isovector]

I don't have a particularly strong opinion here; happy to split out the acid movement into its own branch and shelve it for the time being if you'd prefer!

[ysangkok]

I like option 2 (this current version of the PR) since I think it means that each PR will be smaller? It's gonna be a pain to review the next one if it effectively includes this PR...

[gbaz]

If both of you prefer option 2, fine by me

[isovector]

My plan of attack here is a PR where each commit renames one state module and implements equivalent database logic in its place---even if it's non idiomatic db code. Should be straightforward (if tedious) to review.

We can follow up with some patches to improve the idioms.