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.

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

alacritty: --add-rpath instead of --set-rpath #240773

Merged
merged 1 commit into from Jul 1, 2023
Merged

alacritty: --add-rpath instead of --set-rpath #240773

merged 1 commit into from Jul 1, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jun 30, 2023

Description of changes

As reported by @blucoat in #219213 alacritty's RPATH is missing many of the libraries which it links to, including for example glibc.

The problem was diagnosed by @kchibisov as being caused by alacritty's use of --set-rpath (which completely replaces the rpath) instead of --add-rpath (which adds additional entries to the rpath):

#219213 (comment)

This commit implements @kchibisov's idea to change --set-rpath to --add-rpath:

#219213 (comment)

Closes #219213

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

As reported by @blucoat in
#219213 alacritty's RPATH is
missing many of the libraries which it links to, including for
example glibc.

The problem was diagnosed by @kchibisov as being caused by
alacritty's use of `--set-rpath` (which completely replaces the
rpath) instead of `--add-rpath` (which adds additional entries to
the rpath):

  #219213 (comment)

This commit implements @kchibisov's idea to change `--set-rpath` to
`--add-rpath`:

  #219213 (comment)

Closes #219213
@ghost ghost requested review from trofi, Mic92 and Br1ght0ne June 30, 2023 18:29
@ghost
Copy link
Author

ghost commented Jun 30, 2023

It looks like we have 537 occurrences of --set-rpath in nixpkgs ☹️

Some of those are part of --set-rpath $(patchelf --print-rpath), which is okay, but most of them are probably causing problems similar to this one. Ugh.

@ghost
Copy link
Author

ghost commented Jun 30, 2023

Maybe our fixupPhase should fail if any binaries in the outpath have unresolved DT_NEEDED entries in their ELF structures. There would be a mechanism to override this, of course, if needed.

@kchibisov
Copy link

Acked-by: Kirill Chibisov contact@kchibisov.com

@NickCao NickCao merged commit e3a2a8e into NixOS:master Jul 1, 2023
24 checks passed
@ghost ghost deleted the pr/alacritty/rpath branch July 1, 2023 20:39
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.

Alacritty does not include glibc in rpath
3 participants