Use response files for `ghc` invocations #9367

Gabriella439 · 2023-10-24T21:58:27Z

Before this change, cabal could fail with the following error message when building very large Haskell packages:

ghc: createProcess: posix_spawnp: resource exhausted (Argument list too long)

This is because when the number of modules or dependencies grows large enough, then the ghc command line can potentially exceed the ARG_MAX command line length limit.

However, ghc supports response files in order to work around these sorts of command line length limitations, so this change enables the use of those response files.

Note that this requires taking a special precaution to not pass RTS options to the response file because there's no way that ghc can support RTS options via the response file. The reason why is because the Haskell runtime processes these options (not ghc), so if you store the RTS options in the response file then ghc's command line parser won't know what to do with them.

This means that ghc commands can still potentially fail if the RTS options get long enough, but this is less likely to occur in practice since RTS options tend to be significantly smaller than non-RTS options.

This also requires skipping the response file if the first argument is --interactive. See the corresponding code comment which explains why in more detail.

Please read Github PR Conventions and then fill in one of these two templates.

Template Α: This PR modifies cabal behaviour

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.

Bonus points for added automated tests!

Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, etc.)

Include the following checklist in your PR:

Patches conform to the coding conventions.

Depends-on: #10292

Gabriella439 · 2023-10-24T22:01:56Z

Note that this has been tested internally at Mercury not just for cabal commands but also downstream tools (like ghcid and haskell-language-server), which is how we stumbled upon the hie-bios issue.

We ran into this issue when building our internal Haskell monolith (which has ~7000 Haskell modules), and we verified that this change fixes the problem without breaking anything else.

geekosaur

Can you run fourmolu on this (make style)? That check is failing.

Cabal/src/Distribution/Simple/Program/GHC.hs

Kleidukos

Thanks a lot @Gabriella439 for this contribution. 🙇

Kleidukos · 2023-10-26T17:46:31Z

@Gabriella439 There are still some failures: https://github.com/haskell/cabal/actions/runs/6643530708/job/18050770478?pr=9367#step:16:14024

Gabriella439 · 2023-10-27T20:57:25Z

I'm trying to reproduce the problem locally, but when I run ./validate.sh -s build && ./validate.sh -s lib-suite I get a whole bunch of tests being skipped with "SKIP no cabal-install" and I'm not sure how to fix that

Mikolaj · 2023-10-28T14:15:00Z

Would any of these help?

https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#running-tests
https://github.com/haskell/cabal/blob/master/cabal-testsuite/README.md

mpickering · 2023-11-01T10:55:53Z

Cabal/src/Distribution/Simple/Program/GHC.hs

+      where
+        (newIsRTSOption, newResponseFileArgs, newOtherArgs) =
+          case arg of
+            "+RTS" -> (True, responseFileArgs, arg : otherArgs)


I wonder if this function already exists in cabal (something similar already exists as filterRtsOpts), perhaps abstract that and call the same function in both places?

mpickering · 2023-11-01T10:59:01Z

Cabal/src/Distribution/Simple/Program/GHC.hs

+
+      withResponseFile
+        verbosity
+        defaultTempFileOptions


Can you please add an option to keep this temporary file, in fact, is it a bug that cabal repl supports --keep-temp-files but that option is not honoured here?

mpickering

Looks like a good direction of travel to me.

I want to block this being merged until there is a way to keep the response file around.

A very common workflow for me is to copy the command line which cabal invokes GHC with so that I can debug GHC issues without going through cabal, it seems this patch would regress that workflow.

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). The flag will also be used for response files; see haskell#9367.

9999years · 2024-09-03T19:49:21Z

The requested support for preserving the temp files ended up being much more complicated to implement than I thought it would be, and I just wanted to check with the maintainers to make sure I'm on the right track.

I've put the flag in SetupCommonOptions because those are already passed to the build functions that need them, and I've attempted to thread it through the conversion functions in D.C.ProjectConfig.Legacy, but I keep getting test failures from the value getting dropped somewhere in the round-trip conversion; this is with cabal test cabal-install:unit-tests --test-options="-p ProjectConfig":

projectConfigKeepTempFiles = -NoFlag +Flag False,

Am I missing something? Is there a simpler way of doing this?

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.

Before this change, `cabal` could fail with the following error message when building very large Haskell packages: ``` ghc: createProcess: posix_spawnp: resource exhausted (Argument list too long) ``` This is because when the number of modules or dependencies grows large enough, then the `ghc` command line can potentially exceed the `ARG_MAX` command line length limit. However, `ghc` supports response files in order to work around these sorts of command line length limitations, so this change enables the use of those response files. Note that this requires taking a special precaution to not pass RTS options to the response file because there's no way that `ghc` can support RTS options via the response file. The reason why is because the Haskell runtime processes these options (not `ghc`), so if you store the RTS options in the response file then `ghc`'s command line parser won't know what to do with them. This means that `ghc` commands can still potentially fail if the RTS options get long enough, but this is less likely to occur in practice since RTS options tend to be significantly smaller than non-RTS options. This also requires skipping the response file if the first argument is `--interactive`. See the corresponding code comment which explains why in more detail. Co-Authored-By: Gabriella Gonzales <[email protected]>

9999years · 2024-09-10T00:09:16Z

Ready for review, but we should probably merge #10292 first for the --keep-temp-files changes.

geekosaur · 2024-09-10T00:21:49Z

Added a dependency so Mergify will hold off until it's merged.

geekosaur requested changes Oct 24, 2023

View reviewed changes

Cabal/src/Distribution/Simple/Program/GHC.hs Outdated Show resolved Hide resolved

Gabriella439 force-pushed the gabriella/response_files_2 branch 2 times, most recently from bbe0bd2 to b689455 Compare October 24, 2023 23:10

geekosaur approved these changes Oct 24, 2023

View reviewed changes

Kleidukos requested changes Oct 25, 2023

View reviewed changes

Cabal/src/Distribution/Simple/Program/GHC.hs Outdated Show resolved Hide resolved

Cabal/src/Distribution/Simple/Program/GHC.hs Outdated Show resolved Hide resolved

Kleidukos approved these changes Oct 25, 2023

View reviewed changes

mpickering reviewed Nov 1, 2023

View reviewed changes

mpickering requested changes Nov 1, 2023

View reviewed changes

9999years force-pushed the gabriella/response_files_2 branch from 9e4dad2 to b0ef42d Compare August 29, 2024 21:21

9999years mentioned this pull request Aug 29, 2024

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

Open

5 tasks

9999years force-pushed the gabriella/response_files_2 branch 6 times, most recently from eff868d to 6d2f9f3 Compare September 3, 2024 19:09

9999years force-pushed the gabriella/response_files_2 branch 7 times, most recently from e170a59 to 7d04b62 Compare September 5, 2024 16:36

9999years force-pushed the gabriella/response_files_2 branch from 7d04b62 to 07570f9 Compare September 6, 2024 19:12

9999years force-pushed the gabriella/response_files_2 branch from 07570f9 to 25d97f8 Compare September 6, 2024 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use response files for `ghc` invocations #9367

Use response files for `ghc` invocations #9367

Gabriella439 commented Oct 24, 2023 •

edited by geekosaur

Loading

Gabriella439 commented Oct 24, 2023

geekosaur left a comment

Kleidukos left a comment

Kleidukos commented Oct 26, 2023

Gabriella439 commented Oct 27, 2023 •

edited

Loading

Mikolaj commented Oct 28, 2023

mpickering Nov 1, 2023

mpickering Nov 1, 2023

mpickering left a comment

9999years commented Sep 3, 2024

9999years commented Sep 10, 2024

geekosaur commented Sep 10, 2024

Use response files for ghc invocations #9367

Are you sure you want to change the base?

Use response files for ghc invocations #9367

Conversation

Gabriella439 commented Oct 24, 2023 • edited by geekosaur Loading

Gabriella439 commented Oct 24, 2023

geekosaur left a comment

Choose a reason for hiding this comment

Kleidukos left a comment

Choose a reason for hiding this comment

Kleidukos commented Oct 26, 2023

Gabriella439 commented Oct 27, 2023 • edited Loading

Mikolaj commented Oct 28, 2023

mpickering Nov 1, 2023

Choose a reason for hiding this comment

mpickering Nov 1, 2023

Choose a reason for hiding this comment

mpickering left a comment

Choose a reason for hiding this comment

9999years commented Sep 3, 2024

9999years commented Sep 10, 2024

geekosaur commented Sep 10, 2024

Use response files for `ghc` invocations #9367

Use response files for `ghc` invocations #9367

Gabriella439 commented Oct 24, 2023 •

edited by geekosaur

Loading

Gabriella439 commented Oct 27, 2023 •

edited

Loading