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: #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 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).

[Mikolaj]

Could you have another look @sheaf and @mpickering?

[mpickering]

Thanks, looks good. Needs an entry in the changelog to say that -keep-tmp-files now will also keep the response files?

[9999years]

@mpickering --keep-tmp-files would keep the response files previously as well, the only change here is that the flag is available on commands other than cabal repl.

[9999years]

Rebased for merge queue.

[nh2]

@9999years This seems wrong to me:

Cabal < 3.13 did not support this flag (it did for ... haddock but not ... repl). So Setup.hs compiled with such older versions rejects it.

This causes errors for users of haskell-language-server if they have a custom Setup.hs depending on Cabal < 3.13, because it makes hie-bios pass the nonexistent flag, causing error:

unrecognized 'repl' option `--keep-temp-files'
Error: [Cabal-7125]
repl failed for my-package-0.1.0.0.

Wouldn't it be correct to just remove that line?

filterCommonFlags already removes the flag for lib:Cabal < 3.13.

[nh2]

Tentative PR in case you agree: #12184