Add macOS libs ahead of brewed llvm libs in lib search path #3840

ts826848 · 2018-02-27T00:18:19Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew tests with your changes locally?

This ensures that libraries that are built with brewed LLVM but not
included in the Command Line Tools/Xcode (e.g. libomp) can be found
during a build, while still using system libraries for the essential
stuff (e.g. libc++)

Sorry it took me so long. Given how short this change is, I'm not sure why I haven't done this ages ago...

This should finally resolve homebrew-core/pull/10456 and #2234, and hopefully we can start using clang for OpenMP stuff too. Just got Fortran left...

This ensures that libraries that are built with brewed LLVM but not included in the Command Line Tools/Xcode (e.g. libomp) can be found during a build, while still using system libraries for the essential stuff (e.g. libc++)

MikeMcQuaid · 2018-02-27T08:38:51Z

Library/Homebrew/extend/ENV/super.rb

@@ -176,6 +176,8 @@ def determine_library_paths
    PATH.new(
      keg_only_deps.map(&:opt_lib),
      HOMEBREW_PREFIX/"lib",
+      "#{MacOS.sdk_path}/usr/lib",


Are you 100% sure adding this isn't going to affect any other builds in any other way?

Actually regardless: only do this on llvm_clang being the compiler; it's not worth the risk.

I think if CLT is installed we want "/usr/lib" not "#{MacOS.sdk_path}/usr/lib"

Also, I'm not sure what happens if we do that and have also set

ENV["SDKROOT"] = MacOS.sdk_path

in a formula, whether they're going to be competing with each other.

@ilovezfs If both CLT and Xcode are installed, which should be preferred?

Isn't MacOS.sdk_path empty or / on CLT anyway?

I just ended up splitting it into another if/else. Don't know if the nesting will be an issue.

MikeMcQuaid · 2018-02-27T08:39:17Z

Library/Homebrew/extend/ENV/super.rb

@@ -176,6 +176,8 @@ def determine_library_paths
    PATH.new(
      keg_only_deps.map(&:opt_lib),
      HOMEBREW_PREFIX/"lib",
+      "#{MacOS.sdk_path}/usr/lib",
+      (compiler == :llvm_clang ? Formula["llvm"].opt_lib.to_s : ""),


Don't use a ternary here; split out this logic by building the PATH differently.

I'm guessing that a ternary also shouldn't be used for separate CLT/Xcode SDK include paths?

Correct, thanks.

ilovezfs · 2018-02-27T09:45:05Z

@aw1621107 if the goal is primarily to be able to use llvm's libomp.dylib, it may be sufficient to

depends_on "libomp"

if we merge Homebrew/homebrew-core#20589.

Or is there some additional benefit/reason to actually build with llvm_clang at the same time as using libomp.dylib?

ts826848 · 2018-02-27T14:20:24Z

@ilovezfs I'm not aware of a reason to require llvm_clang, but I'm not really familiar with OpenMP and if it requires the same version of the compiler/dylib. I figured that this might be more straightforward for users, since OpenMP support would be pretty much "invisible" in terms of special requirements, beyond using brew's llvm instead of Apple's.

MikeMcQuaid · 2018-02-28T10:42:00Z

@aw1621107 Out of interest: do you have any use for llvm_clang except OpenMP?

ts826848 · 2018-02-28T13:22:12Z

@MikeMcQuaid Not yet, at least. I haven't had a need for OpenMP, but there's a chance that could change with one of the projects I'm looking into. In the past I also used it to try building formulae with LTO, since Apple's compiler didn't support it at the time, but I'd guess that that's not a use case that you guys are worried about, especially since Apple's Clang now supports LTO. My last guess would be for formulae which provide an option to build with the really new C++ language/library features, but I can't think of any that actually provide such an option. I know Apple's Clang is missing <optional>, even with -std=c++1z, but I don't know how many formulae actually use it.

fxcoudert · 2018-02-28T13:53:50Z

Regarding libomp specifically: I'm wary of using libomp from one version of upstream's llvm, with Apple's different version of clang. It might be tricky ABI incompatibility, and I have not found any definite statement on ABI compatibility between versions. I also don't know of any project actually using this and having done serious testing, so we would need to triple-check on an extensive testsuite before we use it for OpenMP support in our formulas.

MikeMcQuaid · 2018-03-07T16:03:22Z

@aw1621107 Made some tweaks. Thanks again!

Add macOS libs ahead of brewed llvm libs in lib search path

bfa4748

This ensures that libraries that are built with brewed LLVM but not included in the Command Line Tools/Xcode (e.g. libomp) can be found during a build, while still using system libraries for the essential stuff (e.g. libc++)

ts826848 mentioned this pull request Feb 27, 2018

ImageMagick's OpenMP support is broken Homebrew/homebrew-core#23461

Closed

5 tasks

MikeMcQuaid requested changes Feb 27, 2018

View reviewed changes

ts826848 and others added 3 commits February 28, 2018 09:26

Split ternary for PATH building into if/else cases

7b94842

super: refactor LLVM path addition.

7cb6ebf

super: tweak LLVM refactoring.

740e89d

MikeMcQuaid approved these changes Mar 7, 2018

View reviewed changes

MikeMcQuaid merged commit eee6e98 into Homebrew:master Mar 7, 2018

Homebrew locked and limited conversation to collaborators May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add macOS libs ahead of brewed llvm libs in lib search path #3840

Add macOS libs ahead of brewed llvm libs in lib search path #3840

ts826848 commented Feb 27, 2018 •

edited

Loading

MikeMcQuaid Feb 27, 2018

MikeMcQuaid Feb 27, 2018

ilovezfs Feb 27, 2018 •

edited

Loading

ilovezfs Feb 27, 2018 •

edited

Loading

ts826848 Feb 27, 2018

ilovezfs Feb 27, 2018

MikeMcQuaid Feb 27, 2018

ts826848 Feb 28, 2018

MikeMcQuaid Feb 27, 2018

ts826848 Feb 27, 2018

MikeMcQuaid Feb 27, 2018

ilovezfs commented Feb 27, 2018

ts826848 commented Feb 27, 2018

MikeMcQuaid commented Feb 28, 2018

ts826848 commented Feb 28, 2018

fxcoudert commented Feb 28, 2018

MikeMcQuaid commented Mar 7, 2018

Add macOS libs ahead of brewed llvm libs in lib search path #3840

Add macOS libs ahead of brewed llvm libs in lib search path #3840

Conversation

ts826848 commented Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilovezfs Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

ilovezfs Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilovezfs commented Feb 27, 2018

ts826848 commented Feb 27, 2018

MikeMcQuaid commented Feb 28, 2018

ts826848 commented Feb 28, 2018

fxcoudert commented Feb 28, 2018

MikeMcQuaid commented Mar 7, 2018

ts826848 commented Feb 27, 2018 •

edited

Loading

ilovezfs Feb 27, 2018 •

edited

Loading

ilovezfs Feb 27, 2018 •

edited

Loading