Skip to content

Make GHC targetDir commands relative to current directory #7581

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.

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

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

matthewbauer
Copy link
Collaborator

@matthewbauer matthewbauer commented Aug 26, 2021

GHC does not support response files (@file), so we are constrained in
how many characters we can pass to ghc invocations. On macOS <11 this
is 262144 (getconf ARG_MAX), but this also includes the environment
which can be very big. On Windows, ARG_MAX is probably even
less (32K?).

In projects with a lot of modules being linked (1000+), it’s very easy
to hit this limit because of the long length of cabal v2- build
prefixes. For example:
/Users/matthewbauer/my-haskell-project/dist-newstyle/build/x86_64-osx/ghc-8.10.3/mwb-0/noopt/build/Some/Generic/ModulePrefix/MyModule.dyn_o
is 139 chars and 139 * 1200 = 166800. This leads to errors like:

ghc: createProcess: runInteractiveProcess: exec: resource exhausted (Argument list too long)

To minimize this likelihood, we should limit the path prefixes to what
is necessary. This can be done by converting our absolute build
directory to a relative build directory with
makeRelativeToCurrentDirectory.

Note this only really effects v2- cabal since v1- cabal utilizes ghc
--make resulting in much less args in general (and ghc doesn’t have to
call itself).


Please include the following checklist in your PR:

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

@Mikolaj
Copy link
Member

Mikolaj commented Aug 27, 2021

Amazing! And even when the original problem is eventually fixed in GHC, the shorter paths make logs shorter and more readable (even if sometimes surprising). Any chance for a changelog.d snippet and simple test (I guess a golden test in cabal-testsuite/PackageTests/ would suffice, but I have no idea in which directory)?

@matthewbauer
Copy link
Collaborator Author

... simple test (I guess a golden test in cabal-testsuite/PackageTests/ would suffice, but I have no idea in which directory)?

Hmm... I'm not sure if I have enough familiarity with cabal to write an adequate test. The effected function, buildOrReplLib, is not obviously testable...

@Mikolaj
Copy link
Member

Mikolaj commented Sep 9, 2021

Hmm... I'm not sure if I have enough familiarity with cabal to write an adequate test. The effected function, buildOrReplLib, is not obviously testable...

I mean, is there any difference in -v2 output stemming from your change? Or perhaps a single line in the log that clearly shows that it works?

@phadej
Copy link
Collaborator

phadej commented Sep 9, 2021

FYI: https://gitlab.haskell.org/ghc/ghc/-/issues/16476

I'd personally avoid such hacks as this PR, and prefer absolute / canonicalized paths everywhere as soon as possible. (They just make reasoning easier, when you don't need to worry about CWD). I.e. fix GHC to accept response files. (or we can also start eating vowels from all paths in OSX 🙈 )

@Mikolaj
Copy link
Member

Mikolaj commented Sep 9, 2021

@phadej: when the feature is added to GHC, if you say we should standardize on canonicalized, that's probably what we should do. That means we'd need to revert this PR as soon as that happens. But for now, we have a direct need and we don't have a standard, so I don't see a better course of action than to merge it. Do I miss anything?

@jneira
Copy link
Member

jneira commented Sep 9, 2021

that's probably what we should do. That means we'd need to revert this PR as soon as that happens

and that ghc version is the oldest one cabal supports, so many many years :-)
what about make more packages with less modules? 😝

@gbaz
Copy link
Collaborator

gbaz commented Sep 9, 2021

My feeling is that this change is the sort of thing where the existing tests cover it very well -- i.e. if something went wrong here it would break many things.

@Mikolaj
Copy link
Member

Mikolaj commented Sep 9, 2021

My feeling is that this change is the sort of thing where the existing tests cover it very well -- i.e. if something went wrong here it would break many things.

@gbaz: if you say so, I'm totally comforted. Still, it's in @matthewbauer's interest to add a test that would prevent us from removing this feature prematurely or just completely inadvertently. @matthewbauer: your call.

@Mikolaj
Copy link
Member

Mikolaj commented Sep 30, 2021

@matthewbauer: what's your decision?

@Mikolaj
Copy link
Member

Mikolaj commented Oct 4, 2021

OK, in the interest of preventing bit-rot, let us merge. The test can be added in another PR.

@Mikolaj Mikolaj added the merge me Tell Mergify Bot to merge label Oct 4, 2021
GHC does not support response files (@file), so we are constrained in
how many characters we can pass to ghc invocations. On macOS <11 this
is 262144 (getconf ARG_MAX), but this also includes the environment
which can be very big. On Windows, ARG_MAX is probably even
less (32K?).

In projects with a lot of modules being linked (1000+), it’s very easy
to hit this limit because of the long length of cabal v2- build
prefixes. For example:
/Users/matthewbauer/my-haskell-project/dist-newstyle/build/x86_64-osx/ghc-8.10.3/mwb-0/noopt/build/Some/Generic/ModulePrefix/MyModule.dyn_o
is 139 chars and 139 * 1200 = 166800. This leads to errors like:

> ghc: createProcess: runInteractiveProcess: exec: resource exhausted (Argument list too long)

To minimize this likelihood, we should limit the path prefixes to what
is necessary. This can be done by converting our absolute build
directory to a relative build directory with
makeRelativeToCurrentDirectory.

Note this only really effects v2- cabal since v1- cabal utilizes ghc
--make resulting in much less args in general (and ghc doesn’t have to
call itself).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants