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

3.14 pre-flight checks: bump dependencies #10244

Merged
merged 3 commits into from
Aug 7, 2024
Merged

3.14 pre-flight checks: bump dependencies #10244

merged 3 commits into from
Aug 7, 2024

Conversation

ulysses4ever
Copy link
Collaborator

Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • N/A Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

I don't recall why I closed the original PR to bump hashable, aside from concentrating on why I couldn't do local validates which led to #10205.

@@ -19,7 +19,7 @@ library
, QuickCheck
, rere >=0.1 && <0.3
, tasty <1.6
, tasty-quickcheck <0.11
, tasty-quickcheck <0.12
Copy link
Collaborator

Choose a reason for hiding this comment

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

The API changed, according to CI. Do we want to bump and conditionalize, or leave it at the old version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True... In particular, it looks like it's only one file?

tests/UnitTests/Distribution/Solver/Modular/QuickCheck/Utils.hs:35:24: error: [GHC-83865]
Error:     • Couldn't match expected type: (Test.QuickCheck.Random.QCGen, Int)
                  with actual type: Maybe a0
    • In the pattern: Nothing
      In the pattern: QuickCheckReplay Nothing
      In a case alternative:
          QuickCheckReplay Nothing -> getStdRandom random
   |
35 |       QuickCheckReplay Nothing -> getStdRandom random
   |                        ^^^^^^^

(and two more errors like that). In tasty-quickcheck-0.11 this constructor has changed. There's another constructor that appeared in 0.11, QuickCheckReplayLegacy that is close to the one we use (sans Maybe).

How could we conditionalize btw? Leaving <0.11 would be fine, perhaps, but then every time we do cabal outdated (as per the wiki) it will make some noise -- I'm not a fan of that. I'd rather we had clean cabal outdated. In particular, if it's a matter of adding CPP in one tiny file in the test suite, I'd be fine with that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeh, by "conditionalize" I meant CPP on MIN_VERSION_tasty_quickcheck or whatever it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Kleidukos @Mikolaj opinions? Options with supporting tasty-quickcheck starting from 0.11 are:

  1. add CPP
  2. don't support it, stick to <0.11
  3. support versions starting from 0.11. It looks like it's only a test dependency (but in the cabal-install package), so it's probably not too bad regarding the support window.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like UnkindPartition/tasty#425 may provide the same functionality as that utils file, so we may be able to upgrade tasty and delete the whole file eventually.

Copy link
Member

Choose a reason for hiding this comment

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

Yes obviously my pick is conditioned by "Does the CI pass" ;-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think that we should start with (1) or (3), depending on what is required to satisfy the build, and then move towards (3), since tasty-quickcheck is only a test dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey all. I put ^>=0.11 for tasty-quickcheck, and CI seems to be fine with it. Check it out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great! I verified that the QuickCheck seeds are being displayed correctly with the new API: https://github.com/haskell/cabal/actions/runs/10241795977/job/28330597202#step:19:17

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, that's great, because I wasn't sure.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Looks great!

@ulysses4ever ulysses4ever added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Aug 5, 2024
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 7, 2024
@mergify mergify bot merged commit 47bbd54 into master Aug 7, 2024
50 checks passed
@mergify mergify bot deleted the outdated-3.14 branch August 7, 2024 12:38
mpickering pushed a commit to mpickering/cabal that referenced this pull request Aug 12, 2024
* allow hashable-1.5

* allow tasty-quickcheck 0.11, and in case of cabal-install:test move to ^>=0.11

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants