Skip to content

[macOS] better rpath logic #7094

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

Closed
wants to merge 3 commits into from

Conversation

angerman
Copy link
Collaborator

@angerman angerman commented Sep 28, 2020

This is the continuation of #7076


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 (add file to changelog.d directory).
  • The documentation has been updated, if necessary.

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

This interferes with dead_strip_dylib, and pollutes the load commands,
which have a size limit in recent macOS versions.  We now rely on GHC
to construct the correct -rpath entries *after* the linking phase by
inspecting the produced dynamic product and adding -rpaths for all
non-dead-stripped referenced libraries.

See https://gitlab.haskell.org/ghc/ghc/-/commit/4ff93292243888545da452ea4d4c1987f2343591
or  ghc/ghc@4ff9329

Commissioned-by: Mercury Technologies, Inc. (mercury.com)
@chessai
Copy link
Member

chessai commented Oct 26, 2020

Restarted the failed windows jobs

@chessai
Copy link
Member

chessai commented Oct 26, 2020

@phadej @bgamari could one of you review? This actually fixes a pretty annoying bug on MacOS. I can confirm that it works as intended (tested on 15+ dev machines).

@phadej
Copy link
Collaborator

phadej commented Oct 26, 2020

I'm uneasy on this. I'd like to see the tests, which would break if that code is changed again.

So if we need to wait until GHC 9.2 or 9.0 is released with install_name_tool command in the --info to write a test, then we will wait. (The patch won't work anyway if you aren't using it with such GHC).

Copy link
Member

@emilypi emilypi left a comment

Choose a reason for hiding this comment

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

Hi @angerman - let's talk about how to write tests for this, and I'd be happy to help see this through.

@ulysses4ever
Copy link
Collaborator

This is marked as "In progress" for 3.10. Is it?

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022
@ulysses4ever ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022
@Kleidukos
Copy link
Member

@angerman Yo! Do you need any help with this PR?

@Mikolaj Mikolaj mentioned this pull request Aug 11, 2023
@andreabedini
Copy link
Collaborator

@angerman should we pick this up again? AFAIK ghc --info includes install_name_tool command since 8.10.7.

@alt-romes
Copy link
Collaborator

Note: if we decide to do this, we must carefully take into account that GHC will only keep as RPATHs the paths where linked libraries are found at link time. Specifically, some runtime paths should be included in the binary regardless of any library being found there at link time, if the user specifies so (namely extra-lib-dirs #7339).

So it is kind of fine if getRPaths were to return an empty list for GHCs which workaround the darwin linker issue by post-link-injecting the paths of the libraries found at link time to the RPATH entries, but I'm also not sure to what extent it is necessary, since the list is often not that long.

In fact, getRPaths is typically 2 or 3 directories only: the .cabal/ghc store where all external dependency libraries are stored. In my case, building a shared library these rpaths are computed:

-optl-Wl,-rpath,/Users/romes/.ghcup/ghc/9.8.1/lib/ghc-9.8.1/lib/aarch64-osx-ghc-9.8.1
-optl-Wl,-rpath,/Users/romes/.cabal/store/ghc-9.8.1/lib
-optl-Wl,-rpath,/Library/Developer/CommandLineTools/SDKs/MacOSX14.2.sdk/usr/lib

However, with nix stores I think one might get one path per dependency.

The paths determined by getRPaths/depLibraryPaths are likely where libraries are found at both linktime and runtime, so it should be fine to not pass them on on macOS, I guess.

@ffaf1
Copy link
Collaborator

ffaf1 commented May 8, 2025

Hello, I am going through old PRs to check whether they are stale.

If this PR is still “live”, write a comment and I will remove the consider closing label. Otherwise in ≃ 2 weeks this PR will be closed.

@angerman angerman closed this May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants