Add upper bound check to all

[jappeace]

reacration of https://github.com/haskell/cabal/pull/8339

fixes https://github.com/haskell/cabal/issues/8291 presumably this will make it nag at anyone for forgetting to add upper bounds to their packages.

I tested this by running cabal check on yesod-auth-simple (which has no upper bounds) then added said upper bounds with cabal gen-bounds, and now it works.

I also added a test public-multilib-3.cabal and public-multilib-3.check which expects the output. Furthermore an upper bound was added to public-multilib-2.cabal to assert it's not emmiting a warning.

---

Please include the following checklist in your PR:

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

[mergify[bot]]

:warning: The sha of the head commit of this PR conflicts with #8339. Mergify cannot evaluate rules on this PR. :warning:

[Mikolaj]

@mergify refresh

[mergify[bot]]

refresh

✅ Pull request refreshed

[Mikolaj]

Perhaps mergify is ~not~ now mollified after the old PR has been closed. We'll see. :)

[Mikolaj]

CI is acting up, so let me rebase to include the our CI workaround.

[Mikolaj]

@mergify rebase

[mergify[bot]]

rebase

✅ Branch has been successfully rebased

[jappeace]

If nobody has anything else to review, please merge?

[Mikolaj]

@andreasabel: could you re-review?

[jappeace]

If nobody has anything else to review, please merge?

[ffaf1]

I have added a small comment, also there are some failures in CI, do you need help with those (doctest, etc)?

[mouse07410]

Just to mention that often I do not "forget" but deliberately omit upper bound. Enforcing it sometimes makes sense, and sometimes absolutely doesn't.

[jappeace]

@mouse07410 this should be implemented as just a warning. It shouldn't block (unless I did it wrong).

[jappeace]

@ffaf1 yes I've no idea why the doctests are failing, it doesn't seem to be related to my changes

[ffaf1]

Seems a connection timed out, not your fault. There are compilation erros in your patch tho, fix those and you should be fine.

[Mikolaj]

Actually, CI was corrupted at that time and it's now fixed, so let me rebase and see if CI passes now.

[Mikolaj]

@mergify rebase

[mergify[bot]]

rebase

✅ Branch has been successfully rebased

[Mikolaj]

CI passes! What now?

[jappeace]

Please merge if reviewers are happy with the changes :relaxed:

[Mikolaj]

@andreasabel seems to be busy, so let me dimiss his blockage. Anybody else objects to merging? @jappeace: anything else you'd lke to address or improve before the merge? If that's it, I'm going to set label squash+merge_me (unless you prefer no squash) RSN.

[Mikolaj]

@mergify rebase

[mergify[bot]]

rebase

✅ Branch has been successfully rebased

[Mikolaj]

@jappeace: I'm afraid a test fails after rebasing, probably because it was added independently and the captured output doesn't contain the bounds warnings. Would you amend it?

[ulysses4ever]

I don’t want to argue one way or another, but I feel like some people will get upset about this change: after all, PVP doesn’t have universal support (in this part, at least), and there are Hackage authors who don’t put upper bounds purposefully. Given that it’s a mere warning (is it, btw?), I guess, they will survive. Still, I think, it may be nicer to at least supply a flag that turns the check off. Future work, perhaps…

Also, I’d hope for a more coherent explanation of the change in the commit message. Preferably, an explanation from the end user’s perspective.

[mouse07410]

I am totally with those who do not put upper bounds in purposefully.

So, as long as this PR results only in warning in such cases, I agree to live with it, though I personally find the idea silly.

[ffaf1]

In light of this, I still believe a warning PackageDistSuspiciousWarn is a sensible thing to print. Third party tools (and cabal-install) can then filter out what they don’t fancy, but first thing first!

[Mikolaj]

If this is controversial, we don't have to release it in 3.10 yet, but let's merge so that there's something to argue about and test in the field. It's bad style to let a PR slip. Let it go up in flames if unavoidable.

@ulysses4ever: if we are squash-merging, then indeed a good single commit message would be required instead of the default list of squashed commits. Is the changelog entry good enough? If so, perhaps copy it? If not, amend it?

@ffaf1: could you open a ticket about PackageDistSuspiciousWarn and/or a flag? It's fine if it refers to a yet unmerged PR.

[ulysses4ever]

As I said, I'm not against merging it, so, by all means…

Squash is the next level: presently, even the first commit message (which I usually expect to have some explanation no matter what) is kinda dense. Copying the changelog to the first commit message is an option (if the changelog has a decent explanation).

By the way, Mergify allows several approaches to forming the commit message on squash. We use the default one when it concatenates all messages. Another option is to take the first commit's message. I was wondering if the latter would be a better default. But this is off-topic here, perhaps...

[ffaf1]

PackageDistSuspiciousWarn is what will happen with this patch, if tools fancy it, they can filter it. I will open a ticket for a possible flag. (done!)