Skip to content

Comments

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

Merged
LnL7 merged 3 commits intoNixOS:stagingfrom
LnL7:darwin-cf-rpath
Aug 29, 2017
Merged

CoreFoundation: use rpath to fix issues when using frameworks#27598
LnL7 merged 3 commits intoNixOS:stagingfrom
LnL7:darwin-cf-rpath

Conversation

@LnL7
Copy link
Member

@LnL7 LnL7 commented Jul 23, 2017

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.

@LnL7 LnL7 added 1.severity: mass-rebuild 6.topic: darwin Running or building packages on Darwin labels Jul 23, 2017
@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 LnL7 force-pushed the darwin-cf-rpath branch from 8b66442 to 76d8586 Compare July 26, 2017 17:34
@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 force-pushed the darwin-cf-rpath branch from 76d8586 to 2aadd4a Compare August 28, 2017 19:58
@LnL7 LnL7 changed the base branch from master to staging August 28, 2017 19:59
@LnL7 LnL7 force-pushed the darwin-cf-rpath branch from 2aadd4a to b00b19c Compare August 28, 2017 20:45
LnL7 added 3 commits August 28, 2017 23:24
This will get propagated down to other libraries loaded because
everything in nixpkgs references CF based on an rpath entry.
@LnL7 LnL7 force-pushed the darwin-cf-rpath branch from b00b19c to 5a28fd6 Compare August 28, 2017 21:25
@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
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

@Ericson2314 Ericson2314 Aug 31, 2017

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: darwin Running or building packages on Darwin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants