cabal icon indicating copy to clipboard operation
cabal copied to clipboard

cabal check should warn about missing upper bounds

Open jappeace opened this issue 3 years ago • 15 comments

According to the pvp we should specify upperbounds for all dependencies. Since hackage in the future intents to reject packages without tight upper bounds on dependencies, it's prudent that cabal check gives a warning for this.

jappeace avatar Jul 15 '22 15:07 jappeace

cabal check mimics Hackage's requirements; when Hackage will change its upload policy, cabal check will for sure warn about missing upper bound too.

ffaf1 avatar Jul 15 '22 16:07 ffaf1

Then tell them to stop sending emails about version bounds. They made my colleague upset. :pensive:

jappeace avatar Jul 15 '22 16:07 jappeace

Thanks for bringing this to our attention. Let me check how the whole process is set up, I will get back to you as soon as I have a proposal.

ffaf1 avatar Jul 15 '22 16:07 ffaf1

A way to introduce this could be by adding a PackageDistSuspiciousWarn:

https://github.com/haskell/cabal/blob/af258612f5ddbaf0cb5805f896db6b001c35ce19/Cabal/src/Distribution/PackageDescription/Check.hs#L108-L110

If there is some consensus, I could check how this warning would impact people using stack and eventually write a PR.

ffaf1 avatar Jul 15 '22 18:07 ffaf1

Yes, definitely let's discuss. I wonder what the warnings are used for, so far.

Mikolaj avatar Jul 15 '22 19:07 Mikolaj

On hackage-server, they are used here:

https://github.com/haskell/hackage-server/blob/ae4f14e9d6c66bc0bcd6cd274eea280e0ea5ae25/src/Distribution/Server/Packages/Unpack.hs#L289-L296

ffaf1 avatar Jul 18 '22 07:07 ffaf1

Also notice how we already check for upper bounds in setup-depends

https://github.com/haskell/cabal/blob/207d4ee08b929ae71ae2c746fe6da660bc129f05/Cabal/src/Distribution/PackageDescription/Check.hs#L2124-L2131

with PackageDistInexcusable.

ffaf1 avatar Jul 18 '22 08:07 ffaf1

Related: #5317 (opened in 2018).

ffaf1 avatar Jul 21 '22 05:07 ffaf1

A more sophisticated version of it is asked for in #1703

ulysses4ever avatar Jul 22 '22 18:07 ulysses4ever

I'd go with a simple add any upper bound to all dependencies, and point to cabal gen-bounds in the message. Apparently this is so hard they've not figured out how to do this since 2014, so better keep it simple, right?

jappeace avatar Jul 22 '22 18:07 jappeace

@jappeace: most of "them" were and are volunteers and most of "them" could figure out 20 such things a day, if that was their own single-user project and they could focus on exclusively it and didn't need to forge consensus.

Mikolaj avatar Jul 22 '22 19:07 Mikolaj

sorry it wasn't my intention to doubt the competence of the maintainers, I just meant, let's keep it simple so we have time to do it. I want to volunteer an implementation even, but I'm not going to deal with the imports like #1703 suggests, because that seems needlesly difficult for little gain.

jappeace avatar Jul 22 '22 20:07 jappeace

Oh, fair enough. thank you. No opinion on https://github.com/haskell/cabal/issues/1703, but I do believe in baby steps and getting things humbly done. What do you think @ffaf1 and @ulysses4ever?

Mikolaj avatar Jul 22 '22 20:07 Mikolaj

I'm a big fan of do the simple thing first. I'm not sure if this warning should be on by default or hidden behind a flag (e.g. cabal check --pedantic). But I'm fine with either.

ulysses4ever avatar Jul 22 '22 20:07 ulysses4ever

I agree with “simple things first”. With the new and improved™ structured errors, a third party tool (e.g. stack) can decide to ignore specific checks easily and reliably, so this should be uncontroversial.

I would personally not hide it behind a flag, for UX reasons. I would be pretty miffed to invoke cabal check, upload a package, get a prodding email from trustees (no matter how polite) only to discover I had to use an additional flag.

If @jappeace wants to take a stab at it, this is a good place where to start:

https://github.com/haskell/cabal/blob/23536274bb7baabe5c33140620471e897cf34cf2/Cabal/src/Distribution/PackageDescription/Check.hs#L1823-L1824

ffaf1 avatar Jul 22 '22 21:07 ffaf1