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

Use linker capability detection to improve linker use #9443

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Use linker capability detection to improve linker use #9443

merged 3 commits into from
Nov 17, 2023

Conversation

erikd
Copy link
Member

@erikd erikd commented Nov 14, 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 during the configure step.

  • 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!)

Hopefully this far simpler implementation will replace #9270

@erikd erikd changed the title Erikd/relocatable flag 2 Use linker capability detection to improve linker use Nov 14, 2023
@erikd erikd requested review from geekosaur, Kleidukos, angerman and andreabedini and removed request for geekosaur and Kleidukos November 14, 2023 03:58
@erikd erikd marked this pull request as draft November 14, 2023 04:00
@erikd erikd mentioned this pull request Nov 14, 2023
Closed
4 tasks
@erikd erikd force-pushed the erikd/relocatable-flag-2 branch 2 times, most recently from 1180003 to e66f226 Compare November 14, 2023 04:08
@erikd erikd marked this pull request as ready for review November 14, 2023 18:57
@erikd erikd added the merge me Tell Mergify Bot to merge label Nov 14, 2023
@erikd erikd requested a review from bgamari November 15, 2023 00:13
angerman
angerman approved these changes Nov 15, 2023
View reviewed changes
Copy link
Collaborator

@angerman angerman left a comment

Choose a reason for hiding this comment

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

LGTM! Feature detection over assumption by proxy!

geekosaur
geekosaur requested changes Nov 15, 2023
View reviewed changes
Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

You still need to add a changelog entry.

Cabal/src/Distribution/Simple/Program/Builtin.hs Outdated Show resolved Hide resolved
@erikd erikd force-pushed the erikd/relocatable-flag-2 branch 2 times, most recently from 609049b to b7810b9 Compare November 15, 2023 01:45
@erikd
Copy link
Member Author

erikd commented Nov 15, 2023

@geekosaur Changelog entry added.

@erikd erikd mentioned this pull request Nov 15, 2023
Closed
@erikd erikd force-pushed the erikd/relocatable-flag-2 branch 2 times, most recently from eec42d8 to 1e3528a Compare November 15, 2023 02:09
@erikd erikd removed the request for review from Kleidukos November 15, 2023 04:34
andreabedini
andreabedini reviewed Nov 15, 2023
View reviewed changes
Cabal/src/Distribution/Simple/Configure.hs Outdated
Comment on lines 771 to 779
let linkerSupportsGhciLibs :: Bool
linkerSupportsGhciLibs =
case lookupProgramByName "ld" programDb'' of
Nothing -> True -- NOTE: This may still fail if the linker does not support -r.
Just ld ->
case Map.lookup "Supports relocatable output" $ programProperties ld of
Just "YES" -> True
Just "NO" -> False
_other -> True -- NOTE: This may still fail if the linker does not support -r.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry to be a PITA 😂

What this function does is checking whether we had previously detected that linker does not support -r. Am I correct? We use this information to ignore enable-library-for-ghci further down. I think this should be more explicit.

How about we

  1. Rename the function to not mention ghci but to mention the linker property (relocatable outputs? is it always -r?)
  2. Change the function to return Maybe Bool (as in Yes/No/I don't know).
  3. Delegate the fromMaybe True to withGHCiLib_

Does this sound fair?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 17, 2023
@erikd @Mikolaj
Fix configuation of ldProgram
fb14e4e 
Standard GNU `ld` ues `--relocatable` while `ld.gold` uses a `-relocatable`
flag (with a single `-`). Code will now detect both versions.
@erikd @Mikolaj
Chain configuration of ldProgram
4336f4c 
`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 @Mikolaj
Use linker capability detection to improve linker use
53fc3d3 
The function `comperSupportsGhciLibs` has been renamed to
`linkerSupportsGhciLibs` because its about the linker not the compiler.

The function `comperSupportsGhciLibs` was using the compiler version
as a proxy for whether the linker supports relocatable objects. Now
support for relocatable objects is detected by running the linker.
@mergify mergify bot merged commit 03809b3 into haskell:master Nov 17, 2023
45 checks passed
@erikd
Copy link
Member Author

erikd commented Nov 17, 2023

🎉 🎉 🎉

@erikd erikd deleted the erikd/relocatable-flag-2 branch November 18, 2023 04:47
@donya donya mentioned this pull request Jan 11, 2024
Open
@Bodigrim
Copy link
Collaborator

@Mikolaj @Kleidukos could this possibly be backported to 3.10.3.0?

@Mikolaj
Copy link
Member

Mikolaj commented Feb 16, 2024

@Kleidukos calls the shots re 3.10.3.0, but from what I know, it's 5 past midnight, so the PR would need to be backported by somebody else, which involves solving formatting conflicts. Is there a volunteer? It may be best to apply the changes afresh instead of backporting, but let me try an automatic backport just to confirm the conflicts.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 16, 2024

@mergify backport 3.10

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 16, 2024 •
edited
Loading

backport 3.10

✅ Backports have been created

@mergify mergify bot mentioned this pull request Feb 16, 2024
Closed
Kleidukos added a commit to Kleidukos/cabal that referenced this pull request Mar 11, 2024
@Kleidukos
Backport haskell#9443 to 3.10
b6f07a1
Kleidukos added a commit to Kleidukos/cabal that referenced this pull request Mar 11, 2024
@Kleidukos
Backport haskell#9443 to 3.10
106c1d5
@Kleidukos Kleidukos mentioned this pull request Mar 11, 2024
Merged
Kleidukos added a commit to Kleidukos/cabal that referenced this pull request Mar 11, 2024
@Kleidukos
Backport haskell#9443 to 3.10
a23391b
Kleidukos added a commit that referenced this pull request Mar 12, 2024
@Kleidukos
Backport of #9443 "Use linker capability detection to improve linker …
256f85d 
…use" (#9797)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreabedini andreabedini andreabedini left review comments

@angerman angerman angerman approved these changes

@geekosaur geekosaur geekosaur approved these changes

@bgamari bgamari Awaiting requested review from bgamari

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

Successfully merging this pull request may close these issues.
6 participants
@erikd @Bodigrim @Mikolaj @angerman @andreabedini @geekosaur