cabal check should warn about missing upper bounds

[jappeace]

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.

[ffaf1]

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

[jappeace]

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

[ffaf1]

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]

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

cabal/Cabal/src/Distribution/PackageDescription/Check.hs

Lines 108 to 110 in af25861

	-- | Like PackageDistSuspicious but will only display warnings
	-- rather than causing abnormal exit when you run 'cabal check'.
	| PackageDistSuspiciousWarn { explanation :: String }

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

[Mikolaj]

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

[ffaf1]

On hackage-server, they are used here:

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

[ffaf1]

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

cabal/Cabal/src/Distribution/PackageDescription/Check.hs

Lines 2124 to 2131 in 207d4ee

	emitError nm =
	PackageDistInexcusable $
	"The dependency 'setup-depends: '"++nm++"' does not specify an "
	++ "upper bound on the version number. Each major release of the "
	++ "'"++nm++"' package changes the API in various ways and most "
	++ "packages will need some changes to compile with it. If you are "
	++ "not sure what upper bound to use then use the next major "
	++ "version."

with PackageDistInexcusable.

[ffaf1]

Related: #5317 (opened in 2018).

[ulysses4ever]

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

[jappeace]

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?

[Mikolaj]

@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.

[jappeace]

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.

[Mikolaj]

Oh, fair enough. thank you. No opinion on #1703, but I do believe in baby steps and getting things humbly done. What do you think @ffaf1 and @ulysses4ever?

[ulysses4ever]

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.

[ffaf1]

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:

cabal/Cabal/src/Distribution/PackageDescription/Check.hs

Lines 1823 to 1824 in 2353627

	check (not (hasUpperBound baseDependency)) $
	PackageDistInexcusable BaseNoUpperBounds