Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Would it be possible to port to GHC 9.10? #14

Closed
Mikolaj opened this issue Sep 22, 2023 · 22 comments
Closed

Would it be possible to port to GHC 9.10? #14

Mikolaj opened this issue Sep 22, 2023 · 22 comments

Comments

@Mikolaj
Copy link

Mikolaj commented Sep 22, 2023

The changes would probably be similar to what's in

haskell/vector#462

See

haskell/core-libraries-committee#167

for context and the breakage got revealed in

https://gitlab.haskell.org/ghc/ghc/-/issues/23848#note_527774

Thank you!

@infinity0
Copy link
Member

GHC 9.10 is still in development right? So far I've only updated this package for releases, and I don't really want to make a habit of chasing upstream so closely.

Do you want to help co-maintain this package? It's a bit special and involves a bunch of hacky shell scripts to auto-apply patches to the upstream libraries. In the long term it might be better to persuade those packages to include our strict versions and take over the maintenance themselves.

It's a shame that Haskell doesn't have (convenient) strictness polymorphism for data...

@Mikolaj
Copy link
Author

Mikolaj commented Sep 27, 2023

I totally share your sentiment regarding our long term strict future.

Thank you for you kind offer of co-maintainership, but unfortunately, I'm overloaded with maintenance ATM (I maintain cabal among other things).

A pity, though, because I report a lot of GHC problems that pop up in my library horde-ad that has strict-containers as its dependencies. Fixing usually involves reproducing the problem with GHC HEAD and just now strict-containers stopped compiling with GHC HEAD. A solution would be to ask https://ghc.gitlab.haskell.org/head.hackage/ to include strict-containers so that it's kept compatible with HEAD continuously. Given that strict-containers contains modules that should be (IMHO) in the packages already in head.hackage, that makes sense to me. However, I can't judge how feasible this would be, e.g., considering the scripts you mention. Thoughs?

@Mikolaj
Copy link
Author

Mikolaj commented Sep 30, 2023

@RyanGlScott, do you think it makes sense to add this package to head.hackage? When I'm looking at transitive deps at https://hackage.haskell.org/package/strict-containers, it seems the following deps are not yet included in head.hackage:

indexed-traversable
strict
vector-binary-instances
assoc
bifunctor-classes-compat
these
transformers-compat

That's a surprisingly long list, but maybe I'm looking it up wrong.

@RyanGlScott
Copy link

do you think it makes sense to add this package to head.hackage?

Sure, head.hackage welcomes patches for libraries that don't build with HEAD. You don't even have to be the maintainer of the package to add it—you can just submit a MR (following the instructions here). I don't have time to get around to this myself right now, but if you submit a head.hackage MR, I'd be happy to be a reviewer.

it seems the following deps are not yet included in head.hackage:

You don't need head.hackage patches for all of a package's dependencies, just those that do not build with recent GHCs. All of the libraries you list currently build with everything up to GHC HEAD, so there's no need to explicitly add them to head.hackage.

@infinity0
Copy link
Member

I don't mind having this package in head.hackage as long as I'm not expected to maintain that patched version.

Directly applying the patch from vector to here isn't the normal maintenance workflow for this package. The usual workflow is to run the scripts to re-apply (i.e. rebase) our own patches on top of the latest vector. So any patches for this package in head.hackage would basically be thrown away when I get around to releasing a next version of this package (by rebasing the existing patches as just described). Just wanted to let you guys know this, so it doesn't come as a surprise.

The other dependencies @Mikolaj you mentioned above are due to providing extra instances in both this package and also strict, to avoid having an explosion of extra packages just for trivial instances.

@Mikolaj
Copy link
Author

Mikolaj commented Oct 1, 2023

@RyanGlScott: thank you.

@infinity0: is it plausible that people contributing to head.hackage could do just that (whenever it's the proper fix), that is, rebase the patches contained in strict-containers using the scripts? Is the process reasonably documented somewhere (I can't find a README)? Would you throw away such head.hackage patches? I guess you would, because verifying that the rebase is fine amounts to running the scripts and comparing the output, so it's just additional effort compared to running the scripts alone --- unless any non-trivial extra changes are exceptionally required.

I'm taking your time discussing this, @infinity0 and @RyanGlScott, not in order to convince anybody about anything, but honestly to figure out which course of action makes sense in this very special case.

@RyanGlScott
Copy link

I don't mind having this package in head.hackage as long as I'm not expected to maintain that patched version.

Patching a library in head.hackage doesn't imply any obligations on the part of the library maintainer to maintain the patch. It's an entirely separate effort. Library maintainers are welcome to use head.hackage if they wish, of course, but it's also fine not to do this.

I don't have a strong opinion on the coding conventions used in head.hackage, as it generally prioritizes making things compile as opposed to writing patches in the exact same style as the original authors.

@infinity0
Copy link
Member

@infinity0: is it plausible that people contributing to head.hackage could do just that (whenever it's the proper fix), that is, rebase the patches contained in strict-containers using the scripts? Is the process reasonably documented somewhere (I can't find a README)?

The "patches" not only include actual patch files but also involve hacky shell scripts to rename modules, and make ad-hoc version-specific adjustments such as copying utility files etc. Whether this works from upstream release to release depends on how much they've refactored in the interim.

The broad workflow is documented in https://github.com/haskellari/strict-containers/blob/master/strict-containers/update-patches-from-git.sh

For people that don't regularly look at this package, and just want to make it compile for GHC HEAD, applying a single upstream patch to this package (i.e. rather than the other way around) will probably be easier, though there's no hard guarantee about that.

@Mikolaj
Copy link
Author

Mikolaj commented Oct 17, 2023

I've had a look. Applying the strict-containers scripts seems the fastest way to fix this, because the changes are trivial, but numerous (unless a good sed invocation covers all). Unfortunately, the version of the vector package containing the workaround (haskell/vector#462) is not yet on Hackage, so I'd need to apply the scripts to a commit from github or alternatively to the patched code on head.hackage. This sadly means a PR would not be directly applicable to the original strict containers repo.

Huh, meta-programming (as in head-hackage and in strict-containers) is powerful, but really adds costs in random places.

If this can't be done maximally reusably, perhaps one could find a good sed invocation and apply it to the version of strict-containers from Hackage and add that patch to head.hackage. The patches should be analogues to what is on head.hackage for vector, etc., but the file names would be different.

Do I make sense?

@infinity0
Copy link
Member

This sadly means a PR would not be directly applicable to the original strict containers repo.

This is fine, no problem. I was only mentioning it above for informational purposes. Conceptually it's the same as git rebasing a persistent fork, rebasing a small part of it often can't be done reusably to help the overall operation anyways, don't worry too much about reuse. Just do whatever is easiest.

unless a good sed invocation covers all

Not for our changes unfortunately, the full logic is in this script https://github.com/haskellari/strict-containers/blob/master/strict-containers/regen.sh

@Mikolaj
Copy link
Author

Mikolaj commented Oct 24, 2023

I drafted a head.hackage patch: https://gitlab.haskell.org/ghc/head.hackage/-/merge_requests/333

@Mikolaj
Copy link
Author

Mikolaj commented Oct 28, 2023

The patch is merged, so strict-containers are now on head.hackage and so the HEAD compatibility part of this ticket is done. Once GHC 9.10 is out, it seems just regenerating the package (and dep bound bumps) should ensure GHC 9.10 compatibility and then this head.hackage patch can be removed.

Thank you again.

@Mikolaj Mikolaj changed the title Would it be possible to port to GHC 9.10 (and HEAD)? Would it be possible to port to GHC 9.10? Oct 28, 2023
@erikd
Copy link
Collaborator

erikd commented Jul 12, 2024

Working on this. Current issue of a name clash on foldl' being export from Prelude.

@erikd
Copy link
Collaborator

erikd commented Jul 12, 2024

Have a PR up #18.

This PR is no good, because I hand edited the files that should have been regenerated with a script.

@erikd
Copy link
Collaborator

erikd commented Jul 14, 2024

Updating strict-containers to work with ghc-9.10 is currently blocked on the following two issues:

  1. Version tags seem to be missing. haskell/vector#496
  2. Is this repo up to date? haskell/containers#1012

@erikd
Copy link
Collaborator

erikd commented Jul 16, 2024

WIP PR is #19 . Just waiting for containers to be updated.

@erikd
Copy link
Collaborator

erikd commented Jul 29, 2024

containers now has the required tag, but I am stuck on getting this compiling: #19 (comment)

@erikd
Copy link
Collaborator

erikd commented Aug 5, 2024

Git HEAD now compiles with ghc-9.10.

@erikd
Copy link
Collaborator

erikd commented Aug 5, 2024

strict-containers-0.2.1 has been uploaded to Hackage.

I am not able to upload strict-containers-lens or strict-containers-serialise but pretty sure they are not needed. The only changes in those parts of the tree are tested-with entries in the cabal files.

@erikd erikd closed this as completed Aug 5, 2024
@Mikolaj
Copy link
Author

Mikolaj commented Aug 5, 2024

Thank you! I confirm I no longer need to --allow-newer for strict-containers and GHC 9.10.1.

@infinity0
Copy link
Member

Awesome, thanks all!

@infinity0
Copy link
Member

I am not able to upload strict-containers-lens or strict-containers-serialise but pretty sure they are not needed. The only changes in those parts of the tree are tested-with entries in the cabal files.

I've also added you to those packages now

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

No branches or pull requests

4 participants