Always pass `ghc-options` #8717

9999years · 2023-01-31T21:46:39Z

The documentation for ghc-shared-options states that they are
combined with ghc-options:

Additional options for GHC when the package is built as shared library. The options specified via this field are combined with the ones specified via ghc-options, and are passed to GHC during both the compile and link phases.

However, only ghc-shared-options and not ghc-options are passed in many cases.

This is an issue because it requires setting ghc-shared-options even if the shared (dynamic) parts of the build don't actually need different options; this has the unpleasant side-effect of causing modules to be compiled twice, effectively doubling compile time! See here, where any non-empty ghc-shared-options causes Cabal to not set -dynamic-too:

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

Lines 1466 to 1469 in acbc0f3

    
           -- Should we use -dynamic-too instead of compiling twice? 
        
           useDynToo = dynamicTooSupported && isGhcDynamic 
        
                       && doingTH && withStaticExe 
        
                       && null (hcSharedOptions GHC bnfo)

This issue was discovered while integrating the mold linker with a Haskell project; when ghc-shared-options wasn't set to the same value as ghc-options, the gold linker would be used instead of mold for various links during the build.

Please 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.

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

I've tested this by using Cabal with this patch applied to build a large commercial Haskell project (the mercury.com backend) and saw that the ghc-options were correctly applied (in this case, that meant observing that no links were performed with gold and all links were correctly performed with mold, as the options requested, with the exception of the single link described in #4439).

Possibly related issues:

Kleidukos · 2023-06-02T09:50:45Z

@9999years would you like my help with fixing the conflicts of the PR?

9999years · 2023-06-02T17:31:33Z

Rebased against latest master, PTAL

fendor

Essentially, this makes sure that wherever we have shared options, we also pass in the ghc-options. I think the implementation makes sense. Ideally, we'd have some kind of test for this... But I wouldn't know how.

LGTM!

geekosaur

LGTM

hsyl20 · 2023-06-13T07:52:07Z

Cabal/src/Distribution/Simple/GHC.hs

@@ -717,7 +717,7 @@ buildOrReplLib mReplFlags verbosity numJobs pkg_descr lbi lib clbi = do
            , ghcOptFPic = toFlag True
            , ghcOptHiSuffix = toFlag "dyn_hi"
            , ghcOptObjSuffix = toFlag "dyn_o"
-            , ghcOptExtra = hcSharedOptions GHC libBi
+            , ghcOptExtra = hcOptions GHC libBi ++ hcSharedOptions GHC libBi


For consistency shouldn't you do the same thing for ghcOptExtra of profOpts a few line above?

ners · 2024-01-09T09:42:21Z

@9999years thanks for fixing this. Is there anything blocking this PR from getting merged?

I'm keen on using the solution outlined in https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9908#note_481299

Kleidukos · 2024-01-23T08:56:01Z

There are some merge conflicts to solve still

9999years · 2024-01-23T17:11:40Z

@Kleidukos Are you going to actually merge or review it? I've fixed conflicts multiple times with this PR over the last year and it still hasn't moved.

Kleidukos · 2024-01-23T17:36:53Z

@9999years Yes, I thought your reviewers would have merged it. I will take personal care of this PR. Thank you again for your patience. 🙇

Kleidukos · 2024-01-23T17:40:08Z

@9999years would you mind giving a thought to #8717 (comment) ?

Gabriella439 · 2024-08-14T17:46:52Z

Is there a reason that this hasn't been merged? This PR got two approvals and then it seemed like people wanted it merged and then nothing happened. Could we get this merged and address any follow-up concerns in a future PR?

geekosaur · 2024-08-14T17:49:36Z

There are still merge conflicts (see just after the incomplete CI record) that prevent it from being merged. I think we're still waiting on @Kleidukos but I can take a look at them later today if necessary.

Kleidukos · 2024-08-14T18:29:23Z

Is there a reason that this hasn't been merged? This PR got two approvals and then it seemed like people wanted it merged and then nothing happened. Could we get this merged and address any follow-up concerns in a future PR?

Maybe GitHub doesn't show you this but this is what I see:

I will try and find the time to rebase this PR this week.

The [documentation for `ghc-shared-options`][1] states that they are combined with `ghc-options`: > Additional options for GHC when the package is built as shared > library. The options specified via this field are combined with the > ones specified via `ghc-options`, and are passed to GHC during both > the compile and link phases. However, _only_ `ghc-shared-options` and not `ghc-options` are passed in many cases. This is an issue because it requires setting `ghc-shared-options` even if the shared (dynamic) parts of the build don't actually need different options; this has the unpleasant side-effect of causing modules to be compiled twice, effectively doubling compile time! See here, where any non-empty `ghc-shared-options` causes Cabal to not set `-dynamic-too`: https://github.com/haskell/cabal/blob/acbc0f3a5cc9faf0913ff3e270196693816cec41/Cabal/src/Distribution/Simple/GHC.hs#L1466-L1469 This issue was discovered while integrating the `mold` linker with a Haskell project. [1]: https://cabal.readthedocs.io/en/latest/cabal-package.html#pkg-field-ghc-shared-options

Kleidukos · 2024-08-14T21:32:56Z

@9999years Is there a procedure I can follow to ensure that the patch does what it says?

geekosaur · 2024-09-14T00:28:52Z

@mergify backport 3.12

mergify · 2024-09-14T00:29:02Z

backport 3.12

✅ Backports have been created

#10350 Always pass ghc-options (backport #8717) has been created for branch 3.12 but encountered conflicts

* Always pass `ghc-options` The [documentation for `ghc-shared-options`][1] states that they are combined with `ghc-options`: > Additional options for GHC when the package is built as shared > library. The options specified via this field are combined with the > ones specified via `ghc-options`, and are passed to GHC during both > the compile and link phases. However, _only_ `ghc-shared-options` and not `ghc-options` are passed in many cases. This is an issue because it requires setting `ghc-shared-options` even if the shared (dynamic) parts of the build don't actually need different options; this has the unpleasant side-effect of causing modules to be compiled twice, effectively doubling compile time! See here, where any non-empty `ghc-shared-options` causes Cabal to not set `-dynamic-too`: https://github.com/haskell/cabal/blob/acbc0f3a5cc9faf0913ff3e270196693816cec41/Cabal/src/Distribution/Simple/GHC.hs#L1466-L1469 This issue was discovered while integrating the `mold` linker with a Haskell project. [1]: https://cabal.readthedocs.io/en/latest/cabal-package.html#pkg-field-ghc-shared-options * Add documentation * Also enhance profArgs and profDynArgs --------- Co-authored-by: Hécate Moonlight <[email protected]> Co-authored-by: Hécate <[email protected]> (cherry picked from commit 9c775f2) # Conflicts: # Cabal/src/Distribution/Simple/GHC.hs # doc/cabal-package-description-file.rst

* Always pass `ghc-options` The [documentation for `ghc-shared-options`][1] states that they are combined with `ghc-options`: > Additional options for GHC when the package is built as shared > library. The options specified via this field are combined with the > ones specified via `ghc-options`, and are passed to GHC during both > the compile and link phases. However, _only_ `ghc-shared-options` and not `ghc-options` are passed in many cases. This is an issue because it requires setting `ghc-shared-options` even if the shared (dynamic) parts of the build don't actually need different options; this has the unpleasant side-effect of causing modules to be compiled twice, effectively doubling compile time! See here, where any non-empty `ghc-shared-options` causes Cabal to not set `-dynamic-too`: https://github.com/haskell/cabal/blob/acbc0f3a5cc9faf0913ff3e270196693816cec41/Cabal/src/Distribution/Simple/GHC.hs#L1466-L1469 This issue was discovered while integrating the `mold` linker with a Haskell project. [1]: https://cabal.readthedocs.io/en/latest/cabal-package.html#pkg-field-ghc-shared-options * Add documentation * Also enhance profArgs and profDynArgs --------- Co-authored-by: Hécate Moonlight <[email protected]> Co-authored-by: Hécate <[email protected]> (cherry picked from commit 9c775f2)

Always pass `ghc-options` (backport #8717)

9999years force-pushed the always-use-ghc-options branch from 433d05b to 2188ace Compare January 31, 2023 23:44

9999years marked this pull request as ready for review February 1, 2023 00:55

Kleidukos added the attention: needs-review label May 17, 2023

9999years force-pushed the always-use-ghc-options branch from 9f309b7 to 11257c5 Compare June 2, 2023 17:31

Kleidukos requested a review from fendor June 2, 2023 19:52

fendor approved these changes Jun 12, 2023

View reviewed changes

geekosaur approved these changes Jun 12, 2023

View reviewed changes

Kleidukos removed the attention: needs-review label Jun 12, 2023

hsyl20 reviewed Jun 13, 2023

View reviewed changes

Kleidukos self-assigned this Jan 23, 2024

Kleidukos force-pushed the always-use-ghc-options branch from 11257c5 to ce56702 Compare August 14, 2024 19:03

Add documentation

b0c5c5b

Kleidukos force-pushed the always-use-ghc-options branch from ce56702 to b0c5c5b Compare August 14, 2024 19:10

Kleidukos added squash+merge me Tell Mergify Bot to squash-merge attention: needs-backport 3.12 labels Aug 17, 2024

Also enhance profArgs and profDynArgs

9c702ad

Kleidukos force-pushed the always-use-ghc-options branch from 4ecf237 to 9c702ad Compare August 17, 2024 12:15

Merge branch 'master' into always-use-ghc-options

f587d21

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 19, 2024

mergify bot merged commit 9c775f2 into haskell:master Aug 19, 2024
53 checks passed

geekosaur removed the attention: needs-backport 3.12 label Sep 14, 2024

mergify bot mentioned this pull request Sep 14, 2024

Always pass ghc-options (backport #8717) #10350

Merged

3 tasks

mergify bot added a commit that referenced this pull request Sep 14, 2024

Merge pull request #10350 from haskell/mergify/bp/3.12/pr-8717

8b037e7

Always pass `ghc-options` (backport #8717)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always pass `ghc-options` #8717

Always pass `ghc-options` #8717

9999years commented Jan 31, 2023 •

edited

Loading

Kleidukos commented Jun 2, 2023

9999years commented Jun 2, 2023

fendor left a comment •

edited

Loading

geekosaur left a comment

hsyl20 Jun 13, 2023

ners commented Jan 9, 2024

Kleidukos commented Jan 23, 2024

9999years commented Jan 23, 2024

Kleidukos commented Jan 23, 2024

Kleidukos commented Jan 23, 2024

Gabriella439 commented Aug 14, 2024

geekosaur commented Aug 14, 2024 •

edited

Loading

Kleidukos commented Aug 14, 2024

Kleidukos commented Aug 14, 2024

geekosaur commented Sep 14, 2024

mergify bot commented Sep 14, 2024 •

edited

Loading

	-- Should we use -dynamic-too instead of compiling twice?
	useDynToo = dynamicTooSupported && isGhcDynamic
	&& doingTH && withStaticExe
	&& null (hcSharedOptions GHC bnfo)

Always pass ghc-options #8717

Always pass ghc-options #8717

Conversation

9999years commented Jan 31, 2023 • edited Loading

Kleidukos commented Jun 2, 2023

9999years commented Jun 2, 2023

fendor left a comment • edited Loading

Choose a reason for hiding this comment

geekosaur left a comment

Choose a reason for hiding this comment

hsyl20 Jun 13, 2023

Choose a reason for hiding this comment

ners commented Jan 9, 2024

Kleidukos commented Jan 23, 2024

9999years commented Jan 23, 2024

Kleidukos commented Jan 23, 2024

Kleidukos commented Jan 23, 2024

Gabriella439 commented Aug 14, 2024

geekosaur commented Aug 14, 2024 • edited Loading

Kleidukos commented Aug 14, 2024

Kleidukos commented Aug 14, 2024

geekosaur commented Sep 14, 2024

mergify bot commented Sep 14, 2024 • edited Loading

✅ Backports have been created

Always pass `ghc-options` #8717

Always pass `ghc-options` #8717

9999years commented Jan 31, 2023 •

edited

Loading

fendor left a comment •

edited

Loading

geekosaur commented Aug 14, 2024 •

edited

Loading

mergify bot commented Sep 14, 2024 •

edited

Loading