print out dependency build failure during cabal build / repl

[ulysses4ever]

fix #5974 #7727

This is a cheap fix via straightforward extension of #6102

---

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.

Please also shortly describe how you tested your change. Bonus points for added tests!

[ulysses4ever]

That’s better: we pass CI now. Next, need a test showing actual profit from it as the current tests that I had to update are not very convincing unfortunately.

[ulysses4ever]

Can't figure out how to make a test because in practice the difference shows up if you build with -j1 vs -j2 (or any number greater than 1), but the test runner seem to not showing that difference. I can see that the example from #5974 with FPretty does get fixed with this change, e.g. I see the full build log with -j2.

[Mikolaj]

Does the runner run with -j2? Does it apply any special segregation of the outputs?

[jneira]

just in case it could help, the test runner uses 2 jobs:

cabal/.github/workflows/validate.yml

Line 30 in 1209433

COMMON_FLAGS: '-j 2 -v'

[ulysses4ever]

@jneira ooo... I knew there should be something like this! One thing, though: I don't see an effect of -j in test scripts even when running them manually, without validate.sh. Do you think this line explains that?

(I should perhaps upload the test I'm trying, so that you see it even if it doesn't work at this point.)

[ulysses4ever]

@Mikolaj I want to petition for this to be merged without a test:

1. It does fix new-build returns exit code 1 (but doesn't indicate that it fails) when run in a cabal project #5974 cabal-install appears to hide stderr output on failure #7727 locally for me.

2. Update in the existing tests (see Files changed) shows that it makes output for -j1 more instructive.

3. (Most importantly) The test suite insists on -j1 (judging by the commit message, in the interest of determinism in test outputs):

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

   Lines 305 to 308 in 410f871

   	| "v2-" `isPrefixOf` cmd
   	= [ "--builddir", testDistDir env
   	, "--project-file", testCabalProjectFile env
   	, "-j1" ]

   Therefore, any change that concerns -j2 (or more than 2), which this one is an instance of, can't be tested.

[Mikolaj]

I see. So the runner runs with -j2, but inside it, (most of) out tests run with -j1?

I trust your judgement on that. Let's skip adding more tests.

[Mikolaj]

LGTM.

[Mikolaj]

Good catch. Must be a left-over from when there was no build, but only install`.

[Mikolaj]

And also a good catch.

[ulysses4ever]

I see. So the runner runs with -j2, but inside it, (most of) out tests run with -j1?

In a nutshell, yes. That j1 is what is passed to the cabal being tested, whereas validate’s j2 is what gets the cabal that executes the test suite.

[ulysses4ever]

@jneira by any chance, could you take another look here?

[jneira]

lgtm, not only the ux is improved but the types are more precise so i hope future improvements will be easier
small note: iiuc there is no test over cabal repl and no previous one had to be updated, have you checked them?

[ulysses4ever]

@jneira thank you for the review. Could you, perhaps, expand a little bit on what you think could be checked about repl? I just checked that cabal from this PR gives the same error for cabal repl as it does for cabal build under conditions described in #5974. Which makes sense to me.

I think important thing to understand about this PR is that it changes the behavior observable only in the case of an unrecoverable error during the build. So, hopefully, it's hard to screw up things under this scenario: the user will only see any difference if they are not getting anything useful out of a build anyway. So, hopefully, it's a low-stake change.

[jneira]

I observed repl command was included for show the new message and I did not see a repl test changed to take the new msg in account, but maybe it should not?

[ulysses4ever]

@jneira indeed, and thanks for noticing (I completely forgot about Repl by now). The reason that no repl tests have to be updated is simply because there seems to be no fails tests for repl. Do you want me to add one?

[jneira]

well not as requirement to merge If you have tested it manually but the fails test could help in ongoing PRS updating it, the same way the previous ones helped this pr

[ulysses4ever]

I was playing with cabal repl in the test suite for a bit, and I don't see how to go about it. It doesn't seem like cabal repl actually hits dieIfNotHaddockFailure (changed in this patch). At least, not for the kind of errors I tried.

I tried erroring local package, and erroring in a dependency supplied by withRepo. The latter is closer to what this patch was trying to address, so here's even more detail. The test:

main = cabalTest . withRepo "repo" . void $ do
    cabalWithStdin "v2-repl" ["--build-depends", "example"] ":q"

(example being buggy.)

No matter what I tried, this patch doesn't seem to affect cabal repl output; or, more precisely, the bits recorded by the test suite doesn't seem to be affected.

In particular, for v2-build the failing output (that I did have to update with this patch) is:

# cabal v2-build
Resolving dependencies...
Build profile: -w ghc-<GHCVER> -O1
In order, the following will be built:
 - example-1.0 (lib) (first run)
Configuring library for example-1.0..
Preprocessing library for example-1.0..
Building library for example-1.0..
Error: cabal: Failed to build example-1.0-inplace.

The last line was added by this PR. But v2-repl shows:

# cabal v2-repl
Resolving dependencies...
Build profile: -w ghc-<GHCVER> -O1
In order, the following will be built:
 - example-1.0 (lib) (first run)
Configuring library for example-1.0..
Preprocessing library for example-1.0..

And that's it... (again, this is cabal.out file that the test suite creates).

---

I really don't want to spend much more time on this. Please, scream if you have ideas how to improve repl testing. Alternatively, I could erase mentions of ReplCommand in this patch (it will still address the issues I reference). I think it'd a downturn because I think no matter the tests it's better to track commands we're running rather then not doing that. But I can see how a non-testable thing feels repelling.