Fix #6290: gen-bounds: do not report empty set of generated bounds

[andreasabel]

Fix #6290:

• Restructured the existing code so that an empty set of generated bounds is considered success (no bounds missing).
• Added a test (almost literally the one from gen-bounds incorrectly reports missing bound when depend on internal library #6290).
• Prevent quadratic complexity by using new util hasElem.
• Some polishing in the Utils module I couldn't abstain from...

I didn't manage to run my test locally, how does this work again?

[Mikolaj]

I didn't manage to run my test locally, how does this work again?

Trouble building or running? https://github.com/haskell/cabal/tree/master/cabal-testsuite

[andreasabel]

Well, the problem is that the test is skipped with message TestCodeSkip "no cabal-install"

cabal-testsuite/PackageTests/GenBounds/Issue6290[issue-6290]$ cabal-tests cabal.test.hs 
/usr/local/bin/runghc-9.2.4 --. .. cabal.test.hs --builddir /Users/abel/bin/src/cabal/dist-newstyle/build/x86_64-osx/ghc-9.2.4/cabal-testsuite-3 cabal.test.hs
SKIP no cabal-install
cabal.test.hs: TestCodeSkip "no cabal-install"
cabal-tests: callProcess: /usr/local/bin/runghc-9.2.4 "--" ... "cabal.test.hs" "--builddir" "/Users/abel/bin/src/cabal/dist-newstyle/build/x86_64-osx/ghc-9.2.4/cabal-testsuite-3" "cabal.test.hs" (exit 1): failed

How do I get it to not skip the test?

[andreasabel]

(Digression: Do we really want to error out on unused imports? CI has -Werror=unused-imports.

src/Distribution/Client/Utils.hs:77:1: error: [-Wunused-imports, -Werror=unused-imports]
Error:     The import of ‘Data.Set’ is redundant
      except perhaps to import instances from ‘Data.Set’
    To import instances alone, use: import Data.Set()
   |
77 | import Data.Set ( Set )
   | ^^^^^^^^^^^^^^^^^^^^^^^

https://github.com/haskell/cabal/runs/7885759782?check_suite_focus=true#step:11:1300.)

[Mikolaj]

How do I get it to not skip the test?

--with-cabal PATH?

Do we really want to error out on unused imports? CI has -Werror=unused-imports.

If so, we probably do? I hope that's mentioned in CONTRIBUTING,

[andreasabel]

Thanks! Passing --with-cabal=cabal helped further, but it seems to automatically pass a --builddir argument that cabal gen-bounds cannot digest:

$ cabal-tests --with-cabal=cabal cabal.test.hs 
/usr/local/bin/runghc-9.2.4 -- ...  cabal.test.hs --builddir /Users/abel/bin/src/cabal/dist-newstyle/build/x86_64-osx/ghc-9.2.4/cabal-testsuite-3 cabal.test.hs --with-cabal cabal
# cabal gen-bounds
+ /Users/abel/.cabal/bin/cabal gen-bounds '-vverbose +markoutput +nowrap' --builddir /Users/abel/bin/src/cabal/cabal-testsuite/PackageTests/GenBounds/Issue6290/cabal.dist/work/./dist
...
Error: cabal: unrecognized 'gen-bounds' option `--builddir'

Any more tricks in the sleve to get around this?

P.S.: I didn't consider --with-cabal here because such flags usually change the default command, rather than switching it on. (Confer --with-ghc for cabal.) Wouldn't having --with-cabal=cabal as default be the better choice, and to skip use --with-cabal=-?

[Mikolaj]

--builddir argument that cabal gen-bounds cannot digest:

Oh, so that may be why all cabal commands accept all possible nonsensical arguments. Should we tweak cabal gen-bounds to accept and ignore --builddir?

Wouldn't having --with-cabal=cabal as default be the better choice, and to skip use --with-cabal=-?

I guess, a good default would be cabal from your local build tree, because that's what people usually want to test, not the cabal they got from ghcup. So perhaps the result of cabal list-bin cabal?

[ulysses4ever]

In other words, --with-cabal here is an essential input: it's the cabal binary that you want to test (usually, freshly built, in-tree, as Mikolaj says).

I wonder if we should make the test suite more user friendly and more insistently nudging the user in the right direction in its messaging. I certainly fall for this at times.

[andreasabel]

@Mikolaj wrote:

Oh, so that may be why all cabal commands accept all possible nonsensical arguments. Should we tweak cabal gen-bounds to accept and ignore --builddir?

No, I wouldn't go in this direction, rather the opposite: always at least warn about non-sensical arguments. So, impov, the current behavior is correct. I am just surprised that my test fails, while a similar test for cabal check after which I modelled my test works fine. cabal check also does not tolerate --builddir.
There is some advanced magic that in the testsuite that puts puts --builddir on a gen-bounds spell but not on a check.
I think I found it here:

cabal/cabal-testsuite/src/Test/Cabal/Prelude.hs

Lines 283 to 294 in e1f5d8b

	| cmd `elem`
	[ "v1-update"
	, "outdated"
	, "user-config"
	, "man"
	, "v1-freeze"
	, "check"
	, "get", "unpack"
	, "info"
	, "init"
	]
	= [ ]

Casing on strings? Why isn't the "command"-argument for cabal invokation drawn from an abstract data type? This makes me age quickly. I got someone to blame for my grey hairs... And blame even says that I just edited this part in October, with no recollection of it now...

Thanks to your help, friends, I succeeded with the test locally now! Shipping to CI...

[andreasabel]

@Mikolaj wrote:

I guess, a good default would be cabal from your local build tree, because that's what people usually want to test, not the cabal they got from ghcup. So perhaps the result of cabal list-bin cabal?

That would be terrific, yes!

@ulysses4ever wrote:

I wonder if we should make the test suite more user friendly and more insistently nudging the user in the right direction in its messaging. I certainly fall for this at times.

Would be greatly appreciated, could be done on demand.
The current case would be solved by a sensible default for --with-cabal, though.
What could be done now is a more detailed --help text for cabal-tests, explaining the non-obvious arguments. E.g., I also wondered what exactly to put for FILE.
And --with-XXX arguments should have the usual semantics we know from configure and cabal itself: replace defaults.

[andreasabel]

One more problem: My current test is apparently pointless, because cabal gen-bounds writes to stdout which apparently isn't captured by the cabalTest wrapper.
I'd like a simple golden value test (subject to --accept), so I need to direct stdout into the cabal.out generated by cabal-tests.
After some investigation, I think the problem is that cabal gen-bounds does not mark its relevant outputs, so they are not picked up by the test runner. Maybe some technical dept here; apparently, gen-bounds didn't have a single test...

$ cabal gen-bounds '-vnormal +markoutput +nowrap'
Warning: this is a debug build of cabal-install with assertions enabled.
-----BEGIN CABAL OUTPUT-----
Resolving dependencies...
-----END CABAL OUTPUT-----
Congratulations, all your dependencies have upper bounds!

So, should the regular output be marked here? Or is it even better if the "Congratulations" message goes to stderr?

What confuses me is that cabal list-bin also does not mark its regular output:

$ cabal list-bin cabal '-vnormal +markoutput +nowrap'
Warning: this is a debug build of cabal-install with assertions enabled.
-----BEGIN CABAL OUTPUT-----
Up to date
-----END CABAL OUTPUT-----
/Users/abel/bin/src/cabal/dist-newstyle/build/x86_64-osx/ghc-9.2.4/cabal-install-3.9.0.0/x/cabal/build/cabal/cabal

To wrap up my musings:
What is the best path to get a golden value test for cabal gen-bounds?

[Mikolaj]

No idea, but let's please open the 3 or more issues mentioned above. They seem to make a lot of sense.

[andreasabel]

Further research: cabal sdist does wrap its regular output:

$ cabal sdist '-vnormal +markoutput +nowrap'
Warning: this is a debug build of cabal-install with assertions enabled.
-----BEGIN CABAL OUTPUT-----
Wrote tarball sdist to ....tar.gz
-----END CABAL OUTPUT-----

So, I suppose it is intended to wrap the output, only it isn't implemented for all commands.

[andreasabel]

What is the best path to get a golden value test for cabal gen-bounds?

To answer my own question: use notice verbosity rather than a naive putStrLn when implementing cabal-install commands.

[andreasabel]

CI is green, the test is meaningful, please review!

[andreasabel]

@Mikolaj wrote:

No idea, but let's please open the 3 or more issues mentioned above. They seem to make a lot of sense.

Done, or did I miss one?

[Mikolaj]

This bit maybe? As vague as it sounds?

I wonder if we should make the test suite more user friendly and more insistently nudging the user in the right direction in its messaging. I certainly fall for this at times.

Would be greatly appreciated, could be done on demand.
The current case would be solved by a sensible default for --with-cabal, though.
What could be done now is a more detailed --help text for cabal-tests, explaining the non-obvious arguments. E.g., I also wondered what exactly to put for FILE.
And --with-XXX arguments should have the usual semantics we know from configure and cabal itself: replace defaults.

[andreasabel]

I'd be happy if this merged soon, because I want to continue and fix the cabal gen-bounds --with-compiler issue.

[Mikolaj]

LGTM

[ulysses4ever]

LGTM, thanks! Minor comments are left to your discretion.

[ulysses4ever]

Suggested change

	"Congratulations, all your dependencies have upper bounds!"
	"Congratulations, all your external dependencies have upper bounds!"

[andreasabel]

I didn't want to change the good behaviors of cabal gen-bounds in this PR.
Hypothetically, someone might have a script using cabal gen-bounds and match the "Congratulations" message exactly.
But let's keep it in mind for an overhaul of cabal gen-bounds.