Skip to content
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

Chain configuration of ldProgram #9270

Closed
wants to merge 5 commits into from
Closed

Chain configuration of ldProgram #9270

wants to merge 5 commits into from

Conversation

erikd
Copy link
Member

@erikd erikd commented Sep 18, 2023 •
edited
Loading

Cabal currently adds the -r (relocatable) flag to some invocations of ldProgram, but lld (commonly used on WIndows) does not understand the -r flag. This PR just detects if ldProgram accepts -r and conditionally adds it, only if it is supported.

Include the following checklist in your PR:

Bonus points for added automated tests!

@mergify mergify bot mentioned this pull request Sep 18, 2023
Closed
4 tasks
@erikd erikd force-pushed the erikd/relocatable-flag branch 5 times, most recently from 969b58e to 8c0b0d5 Compare September 19, 2023 04:32
andreabedini
andreabedini approved these changes Sep 19, 2023
View reviewed changes
Copy link
Collaborator

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

Much gried was caused by Distribution.Simple.GHC.Internal overriding programPostConf set in Distribution.Simple.Program.Builtin.
I cannot shake the feeing this is an area that could be improved but this is ok for now IMHO.

@erikd erikd force-pushed the erikd/relocatable-flag branch 2 times, most recently from ee1fe1b to ce4a84d Compare September 20, 2023 00:30
@Mikolaj
Copy link
Member

Mikolaj commented Sep 20, 2023

Is there an issue ticket connected to this PR? If not, could you add some motivation and explanation to the PR description? Thank you.

@andreabedini
Copy link
Collaborator

@erikd I understand this is a part of a series of patches towards a goal but @Mikolaj is right in pointing out it's not clear where this goal is explicitated :)

I found these links
#9236
#9229
I think something was mentioned in #9226
Then there's https://github.com/input-output-hk/devx-aux/issues/143

Could you write a summary of what you are aiming for?

@erikd
Copy link
Member Author

erikd commented Sep 21, 2023

@Mikolaj @andreabedini The main issue is that the lld linker (which I think is most commonly used on Windows) does not understand the -r flag. This PR simply detects if the linker supports the -relocateable flag and only adds -r when that flag is supported.

Does that clear things up?

@angerman
Copy link
Collaborator

This PR makes cabal agnostic to the linker. For GHCs using lld it will not try to create relocatabled pre-linked ghci libs. For GHCs using GNU ld (or any other linker that supports -r, potentially lld in the future) it permits the creation pre-linked ghci objects. These are significantly faster to load wirh our static rts linker in GHC.

While GHC linked to control the toolchain it ships with for native deployments. This PR makes cabal agnostic to the used toolchain, and especially prevents breakage for cross compiled GHCs when using the gun toolchain.

@erikd erikd force-pushed the erikd/relocatable-flag branch from ce4a84d to 0cead37 Compare September 24, 2023 22:31
@erikd
Copy link
Member Author

erikd commented Sep 24, 2023

Rebased and force pushed.

@mergify
Copy link
Contributor

mergify bot commented Sep 24, 2023

⚠️ The sha of the head commit of this PR conflicts with #9236. Mergify cannot evaluate rules on this PR. ⚠️

@erikd erikd mentioned this pull request Sep 25, 2023
Closed
4 tasks
@erikd erikd force-pushed the erikd/relocatable-flag branch 4 times, most recently from 791495f to 79162ee Compare September 28, 2023 23:37
@erikd erikd requested review from bgamari and geekosaur September 28, 2023 23:38
@erikd erikd force-pushed the erikd/relocatable-flag branch 4 times, most recently from c3800d6 to 60076d0 Compare October 3, 2023 04:25
@bgamari
Copy link
Contributor

bgamari commented Oct 3, 2023

@erikd, can you please edit the MR description to provide an overview of what issue this MR addresses and how it does so?

@erikd erikd force-pushed the erikd/relocatable-flag branch 8 times, most recently from 9c9cfdf to 635c854 Compare November 7, 2023 02:06
@erikd erikd mentioned this pull request Nov 7, 2023
Closed
@erikd erikd force-pushed the erikd/relocatable-flag branch 3 times, most recently from 6f35659 to d2f40f8 Compare November 7, 2023 04:12
@erikd erikd added the merge me Tell Mergify Bot to merge label Nov 7, 2023
@erikd erikd force-pushed the erikd/relocatable-flag branch from d2f40f8 to eb8e6d5 Compare November 7, 2023 07:40
@erikd erikd removed the merge me Tell Mergify Bot to merge label Nov 7, 2023
@erikd erikd force-pushed the erikd/relocatable-flag branch from 1ed5b6a to baec786 Compare November 7, 2023 07:56
@Kleidukos Kleidukos added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Nov 9, 2023
@erikd erikd force-pushed the erikd/relocatable-flag branch from baec786 to b72fa3a Compare November 12, 2023 20:36
@erikd erikd marked this pull request as draft November 13, 2023 04:27
andreabedini and others added 5 commits November 14, 2023 12:23
@andreabedini @erikd
Partially revert haskell#8071
fe24c78
@erikd
Fix configuation of ldProgram
fc440b8 
Standard GNU `ld` ues `--relocatable` while `ld.gold` uses a `-relocatable`
flag (with a single `-`). Code will now detect both versions.
@erikd
Chain configuration of ldProgram
4a0affc 
`ldProgram` gets configured in two places, a seemingly default and
a GHC specific version. The later needs to be updated so that it
first calls the default configuration and then the new GHC version.
@erikd
combineObjectFiles: Error if the linker does not support -r
9f05d8f 
The `lld` linker (commonly used on Windows) does not support
relocatable output.

Closes: haskell#9414
@erikd
CI: Tweaks
341b5b0 
* Update ghc versions used.
* Disable huge number of failing test for ghc == 9.4.* on Windows.
@erikd erikd force-pushed the erikd/relocatable-flag branch from b72fa3a to 341b5b0 Compare November 14, 2023 02:25
@erikd erikd mentioned this pull request Nov 14, 2023
Merged
5 tasks
@erikd
Copy link
Member Author

erikd commented Nov 14, 2023

Hope to replace this PR with #9443

@erikd
Copy link
Member Author

erikd commented Nov 17, 2023

This is redundant now that #9443 is merged.

@erikd erikd closed this Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kleidukos Kleidukos Kleidukos left review comments

@andreabedini andreabedini andreabedini approved these changes

@geekosaur geekosaur geekosaur approved these changes

@bgamari bgamari bgamari approved these changes

@angerman angerman angerman approved these changes

Assignees
No one assigned
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@erikd @Mikolaj @andreabedini @angerman @bgamari @Kleidukos @geekosaur