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

CoreFoundation: use rpath to fix issues when using frameworks #27598

Merged
merged 3 commits into from Aug 29, 2017
Merged

CoreFoundation: use rpath to fix issues when using frameworks #27598

merged 3 commits into from Aug 29, 2017

Conversation

LnL7
Copy link
Member

@LnL7 LnL7 commented Jul 23, 2017 •
edited

Motivation for this change

Implementation of #24693

This allow a binary to decide what version of CoreFoundation is loaded based on it's rpath. This allows binaries to use the version from /System/Library/Frameworks and propagate that down to all dylibs, including libraries that are built against the pure version.

I think keeping the setup hook from #22571 might still be useful, since a dylib that depends on frameworks won't work with the pure CF and hook might not get propagated all the way to the binary to set the rpath. /cc @NixOS/darwin-maintainers

Fixes #12346, #24124, #24127, #27353

$ otool -L ./result/bin/duti
./result/bin/duti:
        /System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices (compatibility version 1.0.0, current version 48.0.0)
        @rpath/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1153.18.0)
        /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1258.1.0)
        /nix/store/adl0qydsan3cb16wwdl9j77ny7mpky50-Libsystem-osx-10.11.6/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1226.10.1)
        /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 728.13.0)

$ otool -l ./result/bin/duti
Load command 17
          cmd LC_RPATH
      cmdsize 80
         path /nix/store/2k1h8pafhw1z754jz155ax7q03amgkhj-duti-1.5.4pre/lib (offset 12)
Load command 18
          cmd LC_RPATH
      cmdsize 40
         path /System/Library/Frameworks (offset 12)
Things done

Please check what applies. Note that these are not hard requirements but mereley serve as information for reviewers.

  • 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.

@mention-bot
Copy link

@LnL7, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pikajude, @edolstra, @Ericson2314, @LnL7 and @copumpkin to be potential reviewers.

@LnL7 LnL7 changed the title CoreFoundation: CoreFoundation: use rpath to fix issues when using frameworks Jul 23, 2017
@LnL7
Copy link
Member Author

LnL7 commented Jul 27, 2017

Waiting for the results of https://hydra.nixos.org/eval/1378812?compare=1378396#tabs-new to make sure nothing breaks.

@mstone
Copy link
Contributor

mstone commented Aug 14, 2017

@LnL7 -- thanks for publishing; these changes got me unstuck by fixing a busted go1.8 binary installed from recent nixpkgs on macOS 10.12.6 earlier this weekend.

@LnL7
Copy link
Member Author

LnL7 commented Aug 14, 2017

That's great! Did you notice issues with any other packages?

@ajbouh
Copy link

ajbouh commented Aug 22, 2017

I'm also getting bitten by a busted go1.8. Is there a plan to merge this soon?

Also, how can I apply this change locally and help test?

@LnL7
Copy link
Member Author

LnL7 commented Aug 23, 2017

I'll probably merge this to staging soon. You can test it by using my fork, eg.

nix-build https://github.com/LnL7/nixpkgs/archive/darwin-cf-rpath.tar.gz -A hello

@copumpkin
Copy link
Member

Sounds good!

@LnL7 LnL7 changed the base branch from master to staging August 28, 2017 19:59
@LnL7
darwin-CF: use @rpath for library id and add an rpath entry for CF ba…
92652b4 
…sed on NIX_COREFOUNDATION_RPATH
@LnL7
darwin-frameworks: use the system CF when frameworks are used
cc1bfbd 
This will get propagated down to other libraries loaded because
everything in nixpkgs references CF based on an rpath entry.
@LnL7
darwin-frameworks: move fixup setup-hook
5a28fd6
@LnL7 LnL7 merged commit adca58c into NixOS:staging Aug 29, 2017
@copumpkin
Copy link
Member

Awesome awesome awesome

@LnL7 LnL7 deleted the darwin-cf-rpath branch August 30, 2017 17:26
Ericson2314
Ericson2314 reviewed Aug 31, 2017
View reviewed changes
pkgs/build-support/cc-wrapper/ld-wrapper.sh
@@ -145,6 +145,10 @@ if [ "$NIX_@infixSalt@_DONT_SET_RPATH" != 1 ]; then
fi
done
done

if [ -n "${NIX_COREFOUNDATION_RPATH:-}" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I missed this before, but this should have probably done the @infixSalt@ thing, to support darwin->darwin cross compilation where the path may not be the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote this before the salts where added. Not sure if it really matters since it will only point to /System/Library/Frameworks, similar to the dyld path with LD_DYLD_PATH.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, my bad @LnL7. I thought this was from the PR you just merged in the last 24 hours, and never double checked!

That it's analogous to LD_DYLD_PATH makes perfect sense :)

@fpletz fpletz added this to the 17.09 milestone Sep 17, 2017
@fpletz fpletz mentioned this pull request Sep 17, 2017
Merged
@LnL7 LnL7 mentioned this pull request Oct 4, 2017
Closed
This was referenced Oct 4, 2017
Closed
Closed
Closed
@lukeadams lukeadams mentioned this pull request Oct 4, 2017
Merged
8 tasks
@LnL7 LnL7 mentioned this pull request Nov 1, 2017
Merged
7 tasks
@marsam marsam mentioned this pull request Mar 29, 2019
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Ericson2314 left review comments

@copumpkin copumpkin Awaiting requested review from copumpkin

@FRidh FRidh Awaiting requested review from FRidh

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@LnL7 @mention-bot @mstone @ajbouh @copumpkin @Ericson2314 @fpletz