Suppress warnings for verbosity

[ptkato]

This PR will add the ability to suppress all warnings within a given command. For example, to suppress all warnings in cabal build, while keeping the output verbose: cabal build --verbose="verbose+nowarn".

This PR serves as a palliative measure for #7286, while also doubling up on adding a new feature.

---

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

[ptkato]

cc @ekmett

Considering that your issue is the subject of this PR, would it suffice for your troubles or a more specific approach would be needed? As of now, it basically suppresses everything that begins with "Warning:".

[phadej]

Sweeping problems under a carpet, not nice.

[Mikolaj]

@phadej: would you elaborate? what is the root problem here in your opinion?

[fgaz]

I'm guessing #5119 (spurious warning) and #7270 (bug on : syntax that keeps it from getting past the "experimental" stage), since those are the ones mentioned in #7286.

Being able to hide warnings is nice, but I agree fixing the source should have priority

[ptkato]

That is exactly the case, while fixing the root problems would be great, it still won't prevent other things coming up in the future and throwing warnings galore. An option to suppress warnings altogether is always nice, which is the objective of this PR.

[phadej]

would you elaborate? what is the root problem here in your opinion?

That flags hides all the warnings. #7286 asks to be able to hide specific warnings.

[ptkato]

Oh, you meant that. Well yes, it would be possible to hide specific warnings in a similar manner, like I stated in the issue, however I took another approach due to the amount of different warnings out there.

[emilypi]

@phadej do you have pointers for @ptkato to help with #7286? Embedding the warnings in the parsec instances was already a naughty approach, and @ptkato's double naughtiness doesn't really solve the problem that was introduced by that. We're here because it's unclear. If you could help him with decoupling the warnings from the parser and treating them in a principled manner that would be very appreciated, and we can unblock @ekmett at the same time.

[phadej]

Embedding the warnings in the parsec instances was already a naughty approach

What you mean by that?

All parser generated warnings come with an enum of their type: https://hackage.haskell.org/package/Cabal-3.4.0.0/docs/Distribution-Parsec-Warning.html#t:PWarnType so it's possible to granularly filter them. That's exactly why that enumeration is in place.

The

Warning: unlifted.cabal:55:27: colon specifier is experimental feature (issue
#5660)

is tagged with PWTExperimental so it's possible to filter just that. Yet, I still think that it's better to just make the feature non-experimental by implementing the outstanding issues. Because as long as feature is experimental, I'll do my best to prevent packages using such features to get into Hackage index, and that probably won't make Edward happy either.

---

Re

Warning: The package has an extraneous version range for a dependency on an
internal library: unlifted:{core, def, reps} ((>=0 && ==0) && ==0) && ==0,
unlifted:core ((>=0 && ==0) && ==0) && ==0. This version range includes the
current package but isn't needed as the current package's library will always
be used.

I admit it's a lost battle. To properly fix it some changes are needed to solver framework to distinguish which constraints are user written (i.e. are present in .cabal file) and which are auto-generated to make solver produce correct install plans. I don't remember (nor really ever understood) in which cases they are needed, TL;DR you need a solver-hero to fix that issue properly, so one "pragmatic" (but sad) approach is just comment out that warning generation and make an issue if someone ever gets to it.

Adding constraints on the package itself by hand is wrong, (e.g. having mylibrary ==0.0.1 in the test-suite), and I hope people don't do that anymore. Also it's something that .Check could possibly figure out, so solver won't need to care.

[Mikolaj]

BTW, the parens around the inequality are spurious

[Mikolaj]

Still spurious. :) [Edit: the parens]

[ekmett]

This patch seems to paper over two actual issues with a fix that doesn't in the end, actually get me to a state where a user can just cabal install a thing without getting an endless scrolling session full of errors without knowing a specialized incantation.

The two main issues I'm concerned with are a spurious complaint I get out of the bounds checker, where cabal complains about a >= 0 bound that it inserts itself and which nothing I can do can give a good user experience for, and one where you get yelled at for using a feature that was made mandatory. Both of these strike me as outright legitimate cabal design bugs.

There's no reason for either warning.

If I use multiple sub-libraries at all, which has been a big project we've been working on since the end of the 2.x era, I have to incur the useless auto-inserted bound, as there is no way to ask for cabal to not hallucinate it into place. Without this being fixed outright, why did we ever build common blocks and all those things in the first place? Because to a first approximation, we simply can't use them.

If I ask for cabal 3.4 as a file format, I then have to use the package:library syntax that I get warned about to use backpack at all. This was a bug fix for the fact that you couldn't have a sublibrary name that collided with a global package name. But as a result of that 'fix' I can't use sublibraries at all without spamming users.

If I have to make the user run some bizarre cabal command to explicitly opt out of these warnings, I'm not entirely clear how it is any better than asking the user to pipe their output to grep or something.

All packages that transitively depend on me then need to adopt this same bit of arcana, because when they invoke my build, they'll still get an endless stream of noise.

I confess, with the current state of this patch, I will probably (almost) never ship a package that incurs these warnings to hackage. This is a damn shame, because the alternative is for me to take like 30+ package names at a time that expose all the internal structure of my project, requiring 30x maintenance overhead, where I'd have to be constantly deprecate individually as that structure changes. Since that alternative is so unpalatable, it just leads to the current status quo of me simply not really uploading new projects to hackage at all and living off github.

[ekmett]

The current behavior for both of these seem actively hostile to active use.

It seems to me that the fix I'd actually want would be to find and suppress the warning about redundant version bounds when one of them is a >= 0 bound. Its origin seems to be almost always this sort of auto-insert, and as there's no way for a user to legitimately write a cabal file that doesn't incur it, its not an actionable warning if nothing the user does can ever prevent it from firing, and nothing the user did caused it. It's therefore a bug.

The warning on package:library similarly seems to be the kind of thing where warning about it on a version of cabal file format that makes it impossible to use without that notation is just blatantly saying this feature doesn't exist. My preference would be to simply unmark this feature as experimental.

My end goal is to be able to in n + 3 years time, for n = whenever these two issues are resolved to be able to do things like break up the internals of lens into lots of little public sublibraries, so that users can incur just the build times for the portions they want, and yet I wouldn't take up a ton of hackage namespace.

This patch doesn't get me any closer to setting n to now.

[Mikolaj]

@ekmett: don't worry. I think "This PR will close #7286." in description is a misleading left-over from an abandoned approach. We currently stand at "Being able to hide warnings is nice, but I agree fixing the source should have priority" from a later comment. So this PR is just a beginning. Do you have any constructive comments to this PR as such, without the assumption it's supposed to fix your #7286 problems? I already noted the remark that syntax of cabal build --verbosity="verbose+nowarnexperimental" is too cryptic. Any hints for an improvement? Anything else?

[fgaz]

Strong agree with both @phadej and @ekmett's first comment. I think #7270 really has to be fixed before allowing hackage uploads though. Unfortunately I don't know when I'll have enough time to sit down and do that, so it'd be great if someone helped investigate it.

One solution (only to the warning spam problem) could be to move that warning to cabal check (still keeping it fatal) or to hackage itself, so that it only gets printed when the user requests it.

[fgaz]

Honestly I'd remove nowarnexperimental: it's way too specific, and I think generalizing it would add a bit too much complexity for little gain since as per @ekmett's comment, there isn't a real need for it. If you do that it's a +1 from me.

Also remember to backport "Commenting out extraneous version warnings"! We don't want that warning to still be there in 3.6!

[ptkato]

@Mikolaj & @fgaz, if you could re-review (there were changes since your reviews), I'd be much obliged.

[Mikolaj]

Still fine. :)

[Mikolaj]

Still spurious. :) [Edit: the parens]

[fgaz]

Here I'd use expectBroken instead of flipping the condition, since the behavior we're testing for is not the one we want. Same for all the others

[ptkato]

Good catch, that way we needn't touch the expected outputs.

[gbaz]

It looks like there's a legit test failure here?

UNEXPECTED FAIL: cabal-testsuite\PackageTests\Regression\T5213ExeCoverage\cabal.test.hs cabal-testsuite\PackageTests\Regression\T5213\cabal.test.hs

[ptkato]

That's odd, they were passing before. I take it that that test was added afterwards? Well, no matter, I'll take a look into it.

[Mikolaj]

@ptkato: indeed, I'm the felon in this case, the tests are from my recent PR.

[Mikolaj]

Which probably means the tests are failing on 3.6 branch for a dual reason (I should have adjusted the expected results, not just backport them from master). Do we care if tests on 3.6 branch pass? I guess we do?

[Mikolaj]

I've checked, #7493 and #7510 are not yet backported, so 3.6 is not broken. One of the backports will need to change expected test results to match these from the final version of this PR.

[Mikolaj]

Now both above PRs are backported.