Skip to content

Conversation

@recursion-ninja
Copy link
Collaborator

Description

I added support for the new GHC-9.12 based on the current alpha release version. The only changes required were the bumping of version bounds and relaxing upper bound constrains on the base package in the project's direct and transitive dependencies.

Copy link
Collaborator

@jorisdral jorisdral left a comment

Choose a reason for hiding this comment

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

If we start supporting 9.12, then we should also add GHA jobs for that compiler version to our CI. It should be sufficient to add the version to the following line:

ghc: ["8.10.7", "9.2.8", "9.4.8", "9.6.4", "9.8.2", "9.10.1"]

@recursion-ninja
Copy link
Collaborator Author

If we start supporting 9.12, then we should also add GHA jobs for that compiler version to our CI.

That seems obvious. Slightly embarrassed I missed this.

@recursion-ninja recursion-ninja force-pushed the ghc-9.12 branch 9 times, most recently from 86b775a to 2063369 Compare November 12, 2024 18:25
@recursion-ninja
Copy link
Collaborator Author

recursion-ninja commented Nov 12, 2024

Four of the kmerge-test test cases involving sortConcat are failing in CI for GHC 9.12.1:

  1. kmerge/tests/count/eight/sortConcat
  2. kmerge/tests/count/five/sortConcat
  3. kmerge/tests/count/levelling-min/sortConcat
  4. kmerge/tests/count/levelling-max/sortConcat

@jorisdral
Copy link
Collaborator

Four of the kmerge-test test cases involving sortConcat are failing in CI for GHC 9.12.1:

1. `kmerge/tests/count/eight/sortConcat`

2. `kmerge/tests/count/five/sortConcat`

3. `kmerge/tests/count/levelling-min/sortConcat`

4. `kmerge/tests/count/levelling-max/sortConcat`

I've searched the GHC issue tracker a bit and it seems the implementation of sort has changed, which could be the reason for the changed count of comparisons. Ref: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/11870. It would be good to verify whether that change is the cause

@recursion-ninja
Copy link
Collaborator Author

It would be good to verify whether that change is the cause.

This is indeed the cause. I copied the sort and sortBy implementations from base-4.19.1 into the kmerge-test.hs test-suite as sortOriginal and sortOriginalBy, respectively. Subsequently, I replaced all calls to L.sort with the new sortOriginal function. When rerunning the test-suite using ghc-9.12.0, the test suite passes when invoking the old sorting routine rather than the new implementation in base4.21.

Now the question is how to proceed...

Perhaps, use CPP based on the base version? Would like your insight on this matter.

@wenkokke
Copy link
Collaborator

I'm not sure what the exact test is doing, but it might be good to relax it to rely less on the implementation details of sort, e.g., use an upper bound rather than an exact number of comparisons, or verify that the result is ordered rather than an exact order.

@recursion-ninja
Copy link
Collaborator Author

@wenkokke That is a good idea. I'm not really satisfied with the CPP solution I quickly slapped together, so I'll look into the intent behind the test case.

@jorisdral
Copy link
Collaborator

@wenkokke That is a good idea. I'm not really satisfied with the CPP solution I quickly slapped together, so I'll look into the intent behind the test case.

From what I recall, the intent here is to count comparisons for different heap-like implementations so that we can compare the implementations. I don't think we have to preserve the exact count for the sort test across GHC versions, as it's mainly a baseline. We can change the expected count based on the GHC version with a CPP pragma -- that seems to be the simplest solution.

@recursion-ninja recursion-ninja added this pull request to the merge queue Nov 25, 2024
Merged via the queue into main with commit d11c709 Nov 25, 2024
27 checks passed
@recursion-ninja recursion-ninja deleted the ghc-9.12 branch November 25, 2024 09:29
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