Expand and unify --keep-temp-files

[9999years]

This is blocking #9367.

Currently, cabal repl has a --keep-temp-files option, and cabal.project has a keep-temp-files option but it only affects Haddock builds.

This patch adds --keep-temp-files to CommonSetupFlags, making it available to all commands. The expanded --keep-temp-files flag is used for the cabal repl command and Haddock builds (retaining compatibility with the previous behavior). The flag will also be used for response files; see #9367.

Template Α: This PR modifies behaviour or interface

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.
• Manual QA notes have been included.
• Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

Depends-on: #10395
Depends-on: #10392
Depends-on: #10391
Depends-on: #10390
Depends-on: #10389
Depends-on: #10394
Depends-on: #10388
Depends-on: #10393

[sheaf]

One concern I have is that CommonSetupFlags is meant to be for flags common to all Setup invocations (it's a Cabal library concept). I don't think cabal-install should directly be re-using it unless it is for the purpose of invoking Setup.

I don't fully understand how the haddock-project command was implemented. I thought it was supposed to be a cabal-install-only concept, yet I see that there is a fair amount of code related to "haddock project" in the Cabal library as well, which doesn't really make sense to me.

I'm not objecting to this PR (which I am happy to see), but I would like to make sure that you have taken into consideration the logical separation between the Cabal library and cabal-install.

[9999years]

One concern I have is that CommonSetupFlags is meant to be for flags common to all Setup invocations ...

Yes, I wasn't entirely sure where to put this flag. There seem to be a lot of flags that are shared between multiple commands (like --enable-split-objs or --builddir or --with-compiler or --package-db etc. etc. etc. — in fact, diffing the --help messages for cabal build and cabal install reveals that the vast majority of the flags are identical). These flags seem to be placed in essentially random places in the code (if there's a schema for these decisions, it's not documented in any place I looked).

I picked CommonSetupFlags because it's already passed to all the places I need it in #9367. I think that adding it to (e.g.) the build flags or install flags or configure flags specifically would result in a much more invasive change. However, if there's some place you know it should be, I can look into moving it.

[mpickering]

I am not sure this is accurate, some commands did support --keep-tmp-files before?

What happens if you are using repl -keep-tmp-files with an older Cabal version? Is that broken by this patch?

[geekosaur]

Some commands did, in an ad hoc fashion; my understanding is that the point of this patch is to extend that to all commands and ensure they behave consistently.

[geekosaur]

See also #10209.

[9999years]

I've added a test that I believe will exercise this case and run it with --boot-cabal-lib:

https://github.com/MercuryTechnologies/cabal/blob/8255fc1e83df0f9f5f29222b128815ce37978cc6/cabal-testsuite/PackageTests/MultiRepl/CustomSetupKeepTempFiles/cabal.test.hs

Let me know if this is sufficient, or if I missed a detail.

Here's the patch to validate.sh I used:

diff --git a/validate.sh b/validate.sh
index b22e033f8..40c4bfcb1 100755
--- a/validate.sh
+++ b/validate.sh
@@ -505,7 +505,7 @@ CMD="$($CABALLISTBIN cabal-install:test:integration-tests2) -j1 --hide-successes
 step_cli_suite() {
 print_header "cabal-install: cabal-testsuite"
 
-CMD="$($CABALLISTBIN cabal-testsuite:exe:cabal-tests) --builddir=$CABAL_TESTSUITE_BDIR --with-cabal=$($CABALLISTBIN cabal-install:exe:cabal) $TESTSUITEJOBS  --with-ghc=$HC --hide-successes --intree-cabal-lib=$PWD --test-tmp=$PWD/testdb $RTSOPTS"
+CMD="$($CABALLISTBIN cabal-testsuite:exe:cabal-tests) --builddir=$CABAL_TESTSUITE_BDIR --with-cabal=$($CABALLISTBIN cabal-install:exe:cabal) $TESTSUITEJOBS  --with-ghc=$HC --hide-successes --boot-cabal-lib $RTSOPTS PackageTests/MultiRepl/CustomSetupKeepTempFiles/cabal.test.hs"
 (cd cabal-testsuite && timed $CMD) || exit 1
 }

[mpickering]

I think the patch looks good, apart from I think it won't work if you are using a custom setup built with an older version of Cabal. See my comment.

[geekosaur]

We need an old-ghcs test job with custom setup, as we already have backward compatibility breakage that leaked through (#10379).

[9999years]

This PR has gotten too large to effectively review, so I've split out many of the changes into separate, smaller PRs:

• Filter CommonSetupFlags more consistently in UnpackedPackage #10395
• Add haddock --keep-temp-files test #10392
• Add repl --keep-temp-files test with custom Setup.hs #10391
• Add cabal repl --keep-temp-files test #10390
• Fix withFileContents documentation (lazy IO strikes again) #10389
• Move addFields to ParseUtils #10394
• Add cabal-testsuite helpers #10388
• haddock-project: add CommonSetupFlags #10393

Once those PRs are all merged, this change will only have ~100 lines to review.