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

Support ghc-9.10 #18

Closed
wants to merge 2 commits into from
Closed

Support ghc-9.10 #18

wants to merge 2 commits into from

Conversation

erikd
Copy link
Collaborator

@erikd erikd commented Jul 12, 2024

No description provided.

The `base` that ships with `ghc-9.10` (base version 4.20.*) exports the
function `foldl'` so we need to hide its to avoid a name collision.
@erikd erikd requested a review from infinity0 July 12, 2024 07:13
@@ -158,7 +158,7 @@ import Data.Strict.HashMap.Autogen.Internal.List (isPermutationBy, unorderedComp
import Data.Semigroup (Semigroup (..), stimesIdempotentMonoid)
import GHC.Exts (Int (..), Int#, TYPE, (==#))
import GHC.Stack (HasCallStack)
import Prelude hiding (filter, foldl, foldr, lookup, map,
import Prelude hiding (filter, foldl, foldl', foldr, lookup, map,
null, pred)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infinity0 Is this the best way to do this? I looked around at the scripts but could not find a better way.

Copy link
Member

@infinity0 infinity0 Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is not the best way. The best way is to

  1. ensure the upstream libraries already support GHC 9.10.
  2. update the versions inside strict-containers/regen.sh to the versions that support GHC 9.10, and then re-run it. ideally this Just Works and there's nothing else to do.
  3. if this fails, then adjust our patches in strict-containers/patches/, and retry (2)
  4. if (3) is too hard to do manually, then strict-containers/update-patches-from-git.sh can help automate part of this process, but you should understand exactly what it's doing as it may mess up your git repo if run incorrectly

I just checked however, and:

  • (1) already fails because the latest released versions of containers (0.7) and unordered-containers (0.2.20) don't yet support GHC 9.10, they don't import hiding foldl'.
  • containers master branch does support GHC 9.10, however unordered-containers doesn't.
  • edit: also should wait for vector to release, so we can do Drop Vector patches #16, which presumably will also include GHC 9.10 support.

I suggest you wait until all these libraries have released versions that support GHC 9.10 before proceeding with this PR. At the very least, your patch here should be applied to the unordered-containers git repo before we proceed with changes in this library.

After that (i.e. contributing to unordered-containers, so that all 3 upstream git repos have master branches that support GHC 9.10), you could optionally experiment with changing the versions in regen.sh to master, but this may result in duplicate unnecessary work if the upstream libraries make further changes that invalidate our new adjusted patches. So that's why I suggest to wait until all libraries have released (tagged) versions that support GHC 9.10.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i will not merge this PR as is, I will try the above.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(1) already fails because the latest released versions of containers (0.7) and unordered-containers (0.2.20) don't yet support GHC 9.10

Both packages do support GHC 9.10.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bodigrim I checked the source code and they don't hiding foldl' like this PR was proposing, how could they support GHC 9.10 without that?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infinity0 I'm not sure what you are looking at. I just checked, say, https://hackage.haskell.org/package/containers-0.7/docs/src/Data.IntMap.Strict.Internal.html - it does hide foldl'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right I was too hasty with a text search, used a regex that only looked on one line.

And unordered-containers has haskell-unordered-containers/unordered-containers@259dc9e which achieves the same without mentioning foldl' at all.

Ok in that case all that's needed is to wait for vector to release, the latest version 0.13.1.0 doesn't hide foldl' but its master branch does.

Then, some of the patches will have to be updated too.

vector @ 0.13.1.0 already invalidates the existing patches
diff --git a/strict-containers/regen.sh b/strict-containers/regen.sh
index a96bec1..25ed7d0 100755
--- a/strict-containers/regen.sh
+++ b/strict-containers/regen.sh
@@ -111,9 +111,9 @@ copy_test_and_rename() {
 if [ -z "$CLEAN" ]; then
        VERSIONS_CABAL=versions.cabal.in
        rm -f $VERSIONS_CABAL
-       ensure_checkout containers v0.6.6
-       ensure_checkout unordered-containers v0.2.19.1
-       ensure_checkout vector vector-0.13.0.0
+       ensure_checkout containers v0.7
+       ensure_checkout unordered-containers v0.2.20
+       ensure_checkout vector vector-0.13.1.0
        cat $VERSIONS_CABAL | fixup_cabal versions ""
        rm -f $VERSIONS_CABAL
 else
[..]
patching file src/Data/Strict/Vector/Autogen/Mutable.hs
Hunk #2 succeeded at 135 (offset 2 lines).
Hunk #3 succeeded at 144 (offset 2 lines).
patching file tests/Tests/Move.hs
patching file tests/Tests/Vector.hs
patching file tests/Tests/Vector/Boxed.hs
patching file tests/Tests/Vector/UnitTests.hs
patching file tests/Utilities.hs
Hunk #1 FAILED at 5.
containers @ master (> 0.7) invalidates the existing patches
diff --git a/strict-containers/regen.sh b/strict-containers/regen.sh
index a96bec1..25ed7d0 100755
--- a/strict-containers/regen.sh
+++ b/strict-containers/regen.sh
@@ -111,9 +111,9 @@ copy_test_and_rename() {
 if [ -z "$CLEAN" ]; then
        VERSIONS_CABAL=versions.cabal.in
        rm -f $VERSIONS_CABAL
-       ensure_checkout containers v0.6.6
-       ensure_checkout unordered-containers v0.2.19.1
-       ensure_checkout vector vector-0.13.0.0
+       ensure_checkout containers master
+       ensure_checkout unordered-containers master
+       ensure_checkout vector master
        cat $VERSIONS_CABAL | fixup_cabal versions ""
        rm -f $VERSIONS_CABAL
 else
[..]
patching file src/Data/Strict/Map/Autogen/Internal.hs
Hunk #1 succeeded at 467 (offset 9 lines).
patching file src/Data/Strict/IntMap/Autogen/Internal.hs
Hunk #1 FAILED at 359.
1 out of 1 hunk FAILED -- saving rejects to file src/Data/Strict/IntMap/Autogen/Internal.hs.rej

Copy link
Collaborator Author

@erikd erikd Jul 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hackage has a version that could be used, but the scripts rely on the version in git and the version is git is on v0.6.8.

I have raised an issue: haskell/containers#1012

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erikd
Copy link
Collaborator Author

erikd commented Jul 12, 2024

This is not yet ready.

@erikd erikd closed this Jul 12, 2024
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.

3 participants