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

Get rid of most @rpath nonsense on Darwin #30150

Merged
merged 1 commit into from Oct 8, 2017
Merged

Get rid of most @rpath nonsense on Darwin #30150

merged 1 commit into from Oct 8, 2017

Conversation

copumpkin
Copy link
Member

@copumpkin copumpkin commented Oct 6, 2017 •
edited

This requires some small changes in the stdenv, then working around the weird choice LLVM made to hardcode @rpath in its install name, and then lets us remove a ton of annoying workaround hacks in many of our Go packages. With any luck this will mean less hackery going forward.

Motivation for this change

I got sick of seeing people getting weird rpath-related errors when packaging Go programs.

Fixes #18131

@acowley this should fix the Go issue you encountered earlier today

Things left to do
  • Test some other stuff that depends on LLVM (e.g., rustc) to make sure it still runs now that I took out the rpath install name in libLLVM. I expect it to but it might have an extraneous rpath load command or two in it.
  • Remove rpath crap from other LLVM versions too, since they'll probably break without the stdenv support
  • Rebase onto staging because this is a mass rebuild. I forgot to work off staging and was too lazy to switch over and wait another couple of hours for everything to build again
  • Make sure I didn't break anything on Linux (but I'm pretty sure I didn't)
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@orivej
Copy link
Contributor

orivej commented Oct 6, 2017

Do you know why the problem of rpaths causing cyclic references between Go outputs does not exist on Linux?

@dezgeg
Copy link
Contributor

dezgeg commented Oct 6, 2017

Because of the patchelf --shrink-rpath stdenv does.

@copumpkin
Copy link
Member Author

Yeah, we could have replicated that, but given that the rpaths are almost entirely unnecessary on macOS, I'd rather just scrap them altogether than replicate the more complicated solution on a platform that doesn't need it.

@dezgeg
Copy link
Contributor

dezgeg commented Oct 6, 2017

How does dynamic library linkage work on darwin in general? (Just asking, I'm curious.) Do the binaries contain absolute paths to the .dylib files? If so, why does the concept of RPATH exist in the file format in the first place?

@orivej
Copy link
Contributor

orivej commented Oct 6, 2017

Do the binaries contain absolute paths to the .dylib files?

Yes, and the libraries contain absolute paths to themselves, so that when programs (and other libraries) are linked to a library, they refer to it by its builtin absolute path.

Linux ld loader also supports loading libraries by an absolute path (this is documented in ld.so(8)), but AFAIK ld linker can't write them.

@copumpkin
Copy link
Member Author

What @orivej said, but to answer your "why" question, it's to support relocateable libraries. We often distribute .app "files" that are basically bundles of dependencies in a folder, and in those cases you don't want your dependencies to expect to be in any particular place.

So basically, you can add @rpath as a special element to a library's self-reported path (which ends up the thing that anyone depending on that library refers to) and then the loader will search any relevant rpath entries in the executable being loaded for that lib. There's also a couple of other special cases like @executable_path and @loader_path, which refer to the top-level binary being loaded and the specific binary making the reference, respectively.

Ericson2314
Ericson2314 approved these changes Oct 6, 2017
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Looks great! Hope to do the same thing on Linux too, someday.

LnL7
LnL7 reviewed Oct 6, 2017
View reviewed changes
pkgs/stdenv/generic/default.nix
# TODO: This really wants to be in stdenv/darwin but we don't have hostPlatform
# there (yet?) so it goes here until then.
preHook = preHook+ lib.optionalString buildPlatform.isDarwin ''
export NIX_BUILD_DONT_SET_RPATH=1
Copy link
Member

Choose a reason for hiding this comment

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

I think the rpath flags for CoreFoundation are only added when this is not set. It probably should be moved outside of the conditional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, sorry, I don't understand. Where is the CF rpath code?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/cc-wrapper/ld-wrapper.sh#L145-L147

@FRidh FRidh added the 6.topic: darwin Running or building packages on Darwin label Oct 6, 2017
@copumpkin copumpkin changed the base branch from master to staging October 7, 2017 02:54
@FRidh FRidh removed their request for review October 7, 2017 07:14
@LnL7 LnL7 mentioned this pull request Oct 8, 2017
Closed
@copumpkin
Get rid of most @rpath nonsense on Darwin
b426c85 
This requires some small changes in the stdenv, then working around the
weird choice LLVM made to hardcode @rpath in its install name, and then
lets us remove a ton of annoying workaround hacks in many of our Go
packages. With any luck this will mean less hackery going forward.
@copumpkin copumpkin changed the title [WIP] Get rid of most @rpath nonsense on Darwin Get rid of most @rpath nonsense on Darwin Oct 8, 2017
@copumpkin copumpkin merged commit 416979f into NixOS:staging Oct 8, 2017
@copumpkin
Copy link
Member Author

@Ericson2314 if you want it on Linux, is there a ticket for that? I'd be curious to hear some discussion of what's involved and see what the community thinks of it

@Ericson2314
Copy link
Member

There's #24844

@copumpkin
Copy link
Member Author

Thanks!

@copumpkin copumpkin mentioned this pull request Oct 9, 2017
Open
@orivej
Copy link
Contributor

orivej commented Nov 25, 2017

I'm going to pick this into release (via staging-17.09) to ease the porting of Go packages to release-17.09. (Otherwise some of them encounter the "cycle detected" error.)

orivej pushed a commit that referenced this pull request Nov 25, 2017
@copumpkin @orivej
Get rid of most @rpath nonsense on Darwin
cfc55fe 
Merge pull request #30150 from copumpkin/no-rpath-nonsense
(cherry picked from commit 416979f)
@orivej
Copy link
Contributor

orivej commented Nov 26, 2017

Released in cfc55fe.

@copumpkin
Copy link
Member Author

Cool, thanks @orivej

@Mitsos101 Mitsos101 mentioned this pull request Jul 1, 2020
Closed
@sdedovic sdedovic mentioned this pull request Nov 14, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LnL7 LnL7 LnL7 left review comments

@Ericson2314 Ericson2314 Ericson2314 approved these changes

Assignees
No one assigned
Labels
1.severity: mass-darwin-rebuild 1.severity: mass-rebuild 6.topic: darwin Running or building packages on Darwin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@copumpkin @orivej @dezgeg @Ericson2314 @LnL7 @FRidh