Skip to content

Separate out Package utilities & enforce better typing discipline#1502

Open
isovector wants to merge 12 commits into
haskell:masterfrom
tweag:package-utils
Open

Separate out Package utilities & enforce better typing discipline#1502
isovector wants to merge 12 commits into
haskell:masterfrom
tweag:package-utils

Conversation

@isovector
Copy link
Copy Markdown
Contributor

@isovector isovector commented May 4, 2026

This PR moves the Utilities section out of Distribution.Server.Packages.Types and into Distribution.Server.Packages.Utils. It then adds:

+data MetadataRevision = MetadataRevision
+  { metaRevCabalFile :: CabalFileText
+  , metaRevInfo :: UploadInfo
+  }
+  deriving stock Show

-type UploadInfo = (UTCTime, UserId)
+data UploadInfo = UploadInfo
+  { uploadInfoTime :: UTCTime
+  , uploadInfoUser :: UserId
+  }
+  deriving stock Show

These new datatypes used to be anonymous tuples, and their new existence gives an easy-to-target API for the work in #1486. This PR is careful to not use the new MetadataRevision constructor anywhere, so that it can be trivially swapped out for a structure that can be pulled out of the database.

In addition, we keep OldUploadInfo around, since swapping it inside of the acid-state would be a breaking serialization change. Furthermore, all of the utility implementations are copied verbatim into Packages.Utils.Acid, since they're still required for *.State maintenance, and will diverge more dramatically in upcoming PRs.

This PR also changes the type of pkgDesc:

-pkgDesc :: PkgInfo -> GenericPackageDescription
+pkgDesc :: MetadataRevision -> GenericPackageDescription

which makes it clear that it is a function of a specific revision, rather than of the package-as-a-whole. This in turn revealed some bugs in the feed code, which was using the wrong dates. I will send a separate PR fixing those issues.

Comment thread src/Distribution/Server/Features/PackageFeed.hs Outdated
Comment thread src/Distribution/Server/Features/PackageFeed.hs Outdated
Comment thread src/Distribution/Server/Features/PackageList.hs Outdated
Comment thread src/Distribution/Server/Features/PackageList.hs Outdated
Comment thread src/Distribution/Server/Pages/Recent.hs
Comment thread src/Distribution/Server/Pages/Recent.hs
Comment thread src/Distribution/Server/Pages/Recent.hs Outdated
@isovector isovector marked this pull request as draft May 4, 2026 19:30
@isovector isovector marked this pull request as ready for review May 4, 2026 22:35
@Ericson2314
Copy link
Copy Markdown

Any reason this wasn't merged?

@isovector
Copy link
Copy Markdown
Contributor Author

@Ericson2314 I don't have the necessary bits, and once I realized the refactoring effort isn't likely to be fruitful, it didn't seem worth pushing on.

@Ericson2314
Copy link
Copy Markdown

Well if you consider this done, than I suppose @gbaz or someone can merge it if he wants accordingly.

@ysangkok
Copy link
Copy Markdown
Member

@Ericson2314 This section of the tech proposal draft suggests that "incremental refactoring is not feasible". I merged previous PRs before the tech proposal was announced, and I still thought an iterative refactoring was the plan. Now that a more comprehensive plan has been announced, I think the people responsible for the rewrite should have full access if it is to be executed, and it doesn't make sense for me to review iterative work like this, given that I am not intimately familiar with the details of the rewrite, and I won't have time to properly review it. Since I do not have access to deployments, all my previous work has been based on local testing, and then resolving remaining issues based on feedback from Gershom. I don't think that model works with the "live migration" model announced in the tech proposal. Please note that this just explains my own inaction, nothing else. I have no particular opposition to the code of this PR, and I am very grateful that Tweag is contributing.

Regarding the processing of the proposal: The tech proposal repo dictates that it is HF leadership that has the final say on whether proposals are accepted or not. I don't know if HF leadership has started reviewing the proposal yet, but surely it wouldn't make sense to start executing the draft plan before it has been vetted?

@Ericson2314
Copy link
Copy Markdown

I can understand not bothering to finish this PR if a different approach is being gone with. However, it is both non-draft and so appears finished (?) and approved. So it may be just a matter of hitting the button.

I liked the look of it / liked the changes as described in the PR description, so I made my SQL refactor on top of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants