Skip to content

Expand and unify --keep-temp-files #10292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Nov 7, 2024
Merged

Expand and unify --keep-temp-files #10292

merged 3 commits into from
Nov 7, 2024

Conversation

9999years
Copy link
Collaborator

@9999years 9999years commented Aug 29, 2024 •
edited
Loading

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

@9999years 9999years force-pushed the rebeccat/keep-temp-files branch 4 times, most recently from 69f0104 to 1e05052 Compare September 5, 2024 00:56
@9999years 9999years marked this pull request as ready for review September 5, 2024 16:29
@9999years 9999years added the merge me Tell Mergify Bot to merge label Sep 6, 2024
@sheaf
Copy link
Collaborator

sheaf commented Sep 8, 2024

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
Copy link
Collaborator Author

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.

@9999years 9999years mentioned this pull request Sep 10, 2024
Merged
4 tasks
@9999years 9999years force-pushed the rebeccat/keep-temp-files branch 2 times, most recently from e0ea1a0 to 4156c93 Compare September 16, 2024 23:59
mpickering
mpickering reviewed Sep 25, 2024
View reviewed changes
@mpickering
Copy link
Collaborator

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 geekosaur mentioned this pull request Sep 25, 2024
Merged
5 tasks
@geekosaur
Copy link
Collaborator

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

@9999years 9999years force-pushed the rebeccat/keep-temp-files branch from 4156c93 to 8255fc1 Compare September 26, 2024 20:03
9999years added a commit to MercuryTechnologies/cabal that referenced this pull request Sep 27, 2024
@9999years
Correctly propagate --keep-temp-files to old Cabal library versions
d797bc4 
This lets the `--keep-temp-files` options to `cabal haddock` and `cabal
repl` work with an older Cabal version.

Pointed out by @mpickering:
haskell#10292 (comment)
@9999years 9999years force-pushed the rebeccat/keep-temp-files branch from b209b2f to d797bc4 Compare September 27, 2024 22:38
This was referenced Sep 27, 2024
Merged
Merged
Merged
Merged
9999years added a commit to MercuryTechnologies/cabal that referenced this pull request Sep 27, 2024
@9999years
Correctly propagate --keep-temp-files to old Cabal library versions
8cefa2c 
This lets the `--keep-temp-files` options to `cabal haddock` and `cabal
repl` work with an older Cabal version.

Pointed out by @mpickering:
haskell#10292 (comment)
@9999years 9999years force-pushed the rebeccat/keep-temp-files branch from d797bc4 to 8cefa2c Compare September 27, 2024 23:18
@9999years 9999years mentioned this pull request Sep 27, 2024
Merged
5 tasks
@9999years 9999years mentioned this pull request Sep 27, 2024
Merged
2 tasks
@9999years 9999years force-pushed the rebeccat/keep-temp-files branch from e9016e8 to f7369cf Compare September 27, 2024 23:58
@9999years
Copy link
Collaborator Author

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

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

@9999years 9999years force-pushed the rebeccat/keep-temp-files branch 4 times, most recently from facc238 to b60ee3f Compare October 2, 2024 17:44
@9999years 9999years removed the merge me Tell Mergify Bot to merge label Oct 2, 2024
@9999years 9999years force-pushed the rebeccat/keep-temp-files branch 2 times, most recently from 0fb8ef7 to 93a5b0b Compare October 4, 2024 16:35
@9999years 9999years force-pushed the rebeccat/keep-temp-files branch from 93a5b0b to 7d29dc2 Compare October 12, 2024 00:06
@9999years 9999years requested a review from mpickering October 12, 2024 07:44
@Mikolaj
Copy link
Member

Mikolaj commented Oct 16, 2024

Could you have another look @sheaf and @mpickering?

mpickering
mpickering approved these changes Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@mpickering mpickering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@9999years 9999years added the merge me Tell Mergify Bot to merge label Nov 5, 2024
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Nov 5, 2024
@9999years
Copy link
Collaborator Author

@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
Move addFields to ParseUtils, add aliasField helper
39db107 
`addFields` should be in `ParseUtils` with the rest of the field
helpers.
@9999years
haddock-project: add CommonSetupFlags
836327b
@9999years
Expand and unify --keep-temp-files
7a04395 
Currently, `cabal repl` has a `--keep-temp-files` option, and
`cabal.project` has a `keep-temp-files` option but it only effects
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) but is also used to determine when to keep
response files.
@9999years 9999years force-pushed the rebeccat/keep-temp-files branch from 7d29dc2 to 7a04395 Compare November 7, 2024 17:44
@9999years
Copy link
Collaborator Author

Rebased for merge queue.

@mergify mergify bot merged commit cb353ba into haskell:master Nov 7, 2024
47 checks passed
@9999years 9999years deleted the rebeccat/keep-temp-files branch November 7, 2024 22:40
@9999years 9999years mentioned this pull request Nov 25, 2024
Open
erikd pushed a commit to erikd/cabal that referenced this pull request Jan 9, 2025
@9999years @erikd
Filter CommonSetupFlags more consistently in UnpackedPackage
242933b 
In haskell#10292, we will move the `--keep-temp-files` setting into
`CommonSetupFlags` and out of `ReplFlags`/`HaddockFlags`. This means
that the flag-filtering behavior (which adapts flags from new versions
of `cabal-install` to old version of `Cabal`) will need to know which
command is being run to provide the correct `CommonSetupFlags`.

Therefore, this change adds several new `filterFooFlags` functions to
provide this behavior, and removes the `commonFlags` used for all
subcommands in `Distribution.Client.ProjectBuilding.UnpackedPackage`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpickering mpickering mpickering approved these changes

@geekosaur geekosaur geekosaur approved these changes

@coot coot Awaiting requested review from coot

@sheaf sheaf Awaiting requested review from sheaf

Assignees
No one assigned
Labels
API change merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge merge+no rebase ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@9999years @sheaf @mpickering @geekosaur @Mikolaj