Rebase of #5370 (fix for #4683)

[ulysses4ever]

#5370 got really close to fixing #4683 but got stuck on trying to solve the -any vs no bounds confusion. Since 3.4, -any disappeared and took this issue with it. So I thought it's a pity this work is still unmerged.

---

Please include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

[Mikolaj]

Yay, I didn't follow the original issue not PR, but great job rescuing this work. BTW, my commits are now merged to master so you can rebase and they should vanish from your PR. I will then start the CI.

Any idea who would be the best pair of reviewers?

[ulysses4ever]

@Mikolaj rebased. I have no idea about potential reviewers though :-( Is there anything like code owners? The patch has to do with cabal check, so whoever is knowledgeable about this land…

[Mikolaj]

Summoning @emilypi and @fgaz, who know people.

BTW, did CI fail?

[ulysses4ever]

@Mikolaj it seems to have failed, yes. Sorry! I'll look into it later today. (At least, it built for me before the latest rebase)

[ulysses4ever]

@Mikolaj this should fix it.

[ulysses4ever]

Oh no, test suite is still not OK. Will check it out later this week.

[Mikolaj]

@ulysses4ever: let me know if I can help.

[ulysses4ever]

@Mikolaj indeed, I got stuck a bit, thank you for suggesting.

I looked into the CI failures a bit. What fails seems to be quick-checked tests coming from cabal-install/tests/UnitTests/Distribution/Solver/Modular/QuickCheck.hs (although the failure comes from (same dir)/DSL.hs:485 which simply calls error). In a nutshell, the problem is that arbitrarily generated tests don't pass check anymore. I guess some change in the generation logic is needed (in QuickCheck.hs); alternatively, the way the tests are setup can be adjusted: DSL.hs (near the error-call) already has logic to filter out some of check-warnings.

My current problem, though, is that I don't see docs for how to manually run quick-checked tests. I used cabal-testsuite/README.md before but it doesn't seem to cover quick-checked tests.

[Mikolaj]

Let's try to unravel this together. I've run

/home/mikolaj/r/cabal/dist-newstyle/build/x86_64-linux/ghc-9.0.1/cabal-testsuite-3/build/cabal-tests/cabal-tests --with-cabal /home/mikolaj/.cabal/bin/cabal

on your PR branch and it passes fine. And indeed the failing bit of CI does not mention cabal-testsuite, so its README is irrelevant. The command at the top of the failing CI check https://github.com/haskell/cabal/pull/7409/checks?check_run_id=2680086338 is

sh validate.sh -j 2 -w ghc-8.8.3 -v --solver-benchmarks -s cli-tests

and indeed it fails for me, with GHC 9.0.1 (the one that I have ready) substituted for the older GHC and things it couldn't find built:

cabal build long-tests
ln -s dist-newstyle/ dist-newstyle-validate-ghc-9.0.1
sh validate.sh -j 2 -w ghc-9.0.1 -v --solver-benchmarks -s cli-tests

I guess I'd start by rebasing your branch on newest master (it did fail a couple of weeks ago). Back to you.

[ulysses4ever]

Okay, thanks a lot @Mikolaj for carifying this. I figured simply

cabal build long-tests && cabal run long-tests

is enough, and even better: to get to the actual failing QuickCheck tests faster skipping slow passing tests, the run command can be called as follows enjoying some of the Tasty magic:

cabal run long-tests -- -p Solver

Another question. Currently cabal check warns about no upper bound on base in any of the "regular" components (library, executable, etc.) -- I think. The PR I took tried extending this to the setup "component", but it extended it significantly: base and Cabal not having upper bounds cause errors, and any other dependency with no upper bound case a warning. I am not sure if such an extension of warning/error policies is justified. If, say, we only had errors (or warnings) for base and Cabal, that would be enough to make the QuickChecked tests happy. And it's easy to implement.

A more challenging task would be to preserve the extended policy and fix the tests. There are two ways to do that. Either change the arbitrary instances or use the post processing filter of checking results. I am no big fun of either: first approach looks messy because it propagates the difference between setup checks and component checks deep into test suite; the second one with post processing feels very ad hoc.

So, I'm curious what you think about this: would it be fine to shrink the original PR to have cabal only checking base and Cabal on setup dependencies or there's some good reason to have stricter policy there, and therefore hacking into the test suite is justified.

[Mikolaj]

... base and Cabal not having upper bounds cause errors ...

So, I'm curious what you think about this: would it be fine to shrink the original PR to have cabal only checking base and Cabal on setup dependencies or there's some good reason to have stricter policy there, and therefore hacking into the test suite is justified.

I can't see any discussion and agreement about turning warnings into errors (and covering all deps) in the original issue. @fgaz: are you aware of any?

Given that it's an invention of the PR we are fixing and that it complicates the implementation, I think it's a good idea to go back to the spec from the issue.

[ulysses4ever]

(Just a minor comment: warnings vs errors doesn’t matter for the current test suite failures. What matters is checking bounds on all dependencies instead of just base/Cabal.)

[fgaz]

I can't see any discussion and agreement about turning warnings into errors (and covering all deps) in the original issue. @fgaz: are you aware of any?

No. Personally, I'd be in favor of extending the warning (a non-fatal one though) to all dependencies, because of this, but I also think we need some proper discussion before such a change, so I agree on reducing the scope of this pr

[ulysses4ever]

@fgaz thanks a lot for chiming in! I agree that extending warnings would be worthwhile, but don't think it makes much sense to have different warning strategies for setup dependencies and component dependencies. How about you?

Meanwhile I pushed a boiled down version of checks (only base and Cabal). Locally it passed the tests that used to fail on CI.

[ulysses4ever]

Thanks a lot for the comments @Mikolaj! I hope I addressed them now.

[ulysses4ever]

Just a heads-up: the commit message should be updated during squashing. Or I can do this and force-push (I prefer to have separate commits during code review but after it's done they're no longer relevant).

Fixed

[Mikolaj]

@ulysses4ever: is this ready from your POV?
@fgaz: I guess you are too busy to review right now?

[ulysses4ever]

@Mikolaj yes, that's my feeling.

[Mikolaj]

@emilypi: would you review?

[emilypi]

Why the TODO?

[ulysses4ever]

Oh, great point! In fact there is already such a function there. Fixed.

[ulysses4ever]

@emilypi maybe you could take a look and say if it's good enough now. 🙏

[Mikolaj]

@emilypi: would it be possible for you to re-review so that we can move this forward? Thank you!

[emilypi]

@Mikolaj will do

[Mikolaj]

AppVeyor died in a way I don't understand: https://ci.appveyor.com/project/emilypi/cabal/builds/40523206

Let me update the branch again (master moved since).