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

libomp 5.0.1 (new formula) #20589

Closed
wants to merge 1 commit into from
Closed

libomp 5.0.1 (new formula) #20589

wants to merge 1 commit into from

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Nov 13, 2017

Add OpenMP for Apple Clang. Apple Clang now has support for OpenMP, but
it has been disabled in the driver and is not included with High Sierra.
It can be built and used, though; this formula will build it and
provides hints on the correct usage.

There have been a few other formula in the past, such as clang-omp, that provided
special compilers and other tricks to add OpenMP. This is not like those; this is for
the built-in system Clang. On at least High Sierra, this is now possible due to the fact
that it is based on a version of Clang (4.0) that has OpenMP support; it's just not built and
included by Apple.

This is the message printed by the formula:

On Apple Clang, you need to add several options to use OpenMP's front end
instead of the standard driver option. This usually looks like:
  -Xpreprocessor -fopenmp -lomp

You might need to make sure the lib and include directories are discoverable if /usr/local is not searched:
  -L/usr/local/opt/libomp/lib -I/usr/local/opt/libomp/include

For CMake, the following will flags will cause the OpenMP::OpenMP_CXX target to be set up correctly:
  -DOpenMP_CXX_FLAGS="-Xpreprocessor -fopenmp -I/usr/local/opt/libomp/include" -DOpenMP_CXX_LIB_NAMES="omp" -DOpenMP_omp_LIBRARY=/usr/local/opt/libomp/lib/libomp.a

And, future thoughts:

  • Support could be added to FindOpenMP for automatic discovery, at least if the OpenMP::* targets are used. I'm investigating a patch for CMake.

Note: Original idea from https://stackoverflow.com/questions/44380459/is-openmp-available-in-high-sierra-llvm/47230419#47230419


  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@fxcoudert
Copy link
Member

I do not think this is a good idea.

  • It's not supported by Apple (the front-end option is there currently, but they might remove it in the future)
  • The library version is not matching the compiler version exactly, and the mismatch might be problematic.
  • We already ship llvm, which includes OpenMP support (GCC does as well, too)

@henryiii
Copy link
Contributor Author

However, building with a completely different compiler can cause problems; for example, using Boost + llvm 5 vs. Boost + Apple llvm 9 vs. Boost + GCC. This does not require a completely different compiler chain.

@ilovezfs
Copy link
Contributor

Agreed with @fxcoudert. This duplicates what's available in the llvm formula. It's already possible to use openmp from the llvm formula with Xcode's clang.

Sorry @henryiii! We look forward to your next contribution.

@ilovezfs ilovezfs closed this Nov 13, 2017
@henryiii
Copy link
Contributor Author

@ilovezfs , how would you do that? Including /usr/local/opt/llvm\@4/lib/clang/4.0.1/include/ causes errors due to the other files in that directory (not omp.h itself). If I manually copy omp.h to the current directory, and use the line:

clang++ -Xpreprocessor -fopenmp simple.cxx -I. -L/usr/local/opt/llvm/lib -lomp

it works.

@henryiii
Copy link
Contributor Author

henryiii commented Nov 13, 2017

If anyone needs it, I've moved the PR contents to a Tap, https://github.com/CLIUtils/homebrew-apple. A cmake include file that conditionally adds this if it's installed is in https://github.com/CLIUtils/cmake/blob/master/PatchOpenMPApple.cmake. I'm using it in GooFit successfully. I'll probably write a blog post about it eventually (which I'll link in the Stack Overflow answers above)

@ilovezfs ilovezfs reopened this Feb 27, 2018
@ilovezfs
Copy link
Contributor

@henryiii mind updating this for llvm 5? We'd like to experiment with using

depends_on "libomp"

instead of

depends_on "gcc"
fails_with :clang

for formulae that need openmp.

@henryiii
Copy link
Contributor Author

Okay, though keep in mind, the build and linking line is a bit different than GCC. I can make a PR to CMake to help with that in the future.

@ilovezfs
Copy link
Contributor

ilovezfs commented Feb 27, 2018

Okay, though keep in mind, the build and linking line is a bit different than GCC. I can make a PR to CMake to help with that in the future.

Right I think we need some superenv (see https://github.com/Homebrew/brew/tree/master/Library/Homebrew/shims/super) support for it. It would be really nice to be able to have

depends_on "libomp"

"just work"

@henryiii
Copy link
Contributor Author

Done.

@ilovezfs
Copy link
Contributor

@henryiii thanks! Have you been able to figure out what the minimum Xcode for this magic to work is?

@MikeMcQuaid MikeMcQuaid changed the title libomp 4.0.1 (new formula) libomp 5.0.1 (new formula) Feb 27, 2018
@ilovezfs
Copy link
Contributor

@henryiii the test block seems to pass even if I remove the -Xpreprocessor -fopenmp. It would be great if you could make the test do something that actually fails if that isn't passed.

@henryiii
Copy link
Contributor Author

Okay, will fix that in a minute. Working on adding a travis test on multiple versions of XCode (running, unfortunately it seems, that test block...)

@henryiii
Copy link
Contributor Author

Is there something obvious missing from the test block? I can get that to fail or pass manually, just not when it's part of the formula; then it always passes.

@ilovezfs
Copy link
Contributor

I would think the test should fail if the -Xpreprocessor -fopenmp is not passed.

@henryiii
Copy link
Contributor Author

Oh, I thought the rest ran when install ran. Nevermind, I now have a test that fails properly if those parts are removed. I'll commit it in a second.

@ilovezfs
Copy link
Contributor

Ah, I see. Yeah you have to manually run brew test --verbose --debug foo

@henryiii
Copy link
Contributor Author

henryiii commented Feb 27, 2018

@ilovezfs , it works with XCode 7+, from a simple test. XCode 6 fails. See https://travis-ci.org/henryiii/test_omp . (Build is still finishing for XCode 6, but it will eventually fail)

Is it better to have the caveat print -L$(brew --prefix libomp)/lib -I$(brew --prefix libomp)/include or the actual expanded value of the path (as it does now)? (For me, the unexpanded line is incorrect because I use fish shell)

@ilovezfs
Copy link
Contributor

It should use #{opt_lib} and #{opt_include}.

Also, I think we want

depends_on :macos => :yosemite

since that's the minimum version for Xcode 7.

-L#{opt_lib} -I#{opt_include}

For CMake, the following will flags will cause the OpenMP::OpenMP_CXX target to be set up correctly:
-DOpenMP_CXX_FLAGS="-Xpreprocessor -fopenmp -I#{opt_include}" -DOpenMP_CXX_LIB_NAMES="omp" -DOpenMP_omp_LIBRARY=#{opt_lib}/libomp.a
Copy link
Contributor

Choose a reason for hiding this comment

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

I think by default we'd want to suggest the dylib not the .a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a typo, I don't think it even makes a .a.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. (Actually, this one does make .a and .dylib; the one I had in my tap only made the dylib)

Add OpenMP for Apple Clang. Apple Clang now has support for OpenMP, but
it has been disabled in the driver and is not included with High Sierra.
It can be built and used, though; this formula will build it and
provides hints on the correct usage.
@ilovezfs ilovezfs added the new formula PR adds a new formula to Homebrew/homebrew-core label Feb 28, 2018
@henryiii
Copy link
Contributor Author

What about adding the bottles? I can see the hashes from Jenkins, should I copy and paste them in?

@ilovezfs ilovezfs added the ready to merge PR can be merged once CI is green label Feb 28, 2018
@ilovezfs
Copy link
Contributor

@henryiii no that is handled automatically by brew pull

@henryiii
Copy link
Contributor Author

Oh, okay, thanks. Wasn't sure how that was setup.

@ilovezfs
Copy link
Contributor

If you're curious https://github.com/Homebrew/brew/blob/master/Library/Homebrew/dev-cmd/pull.rb#L19-L20

@ilovezfs ilovezfs closed this in 3c6486f Feb 28, 2018
@ilovezfs
Copy link
Contributor

Thanks for your first contribution to Homebrew, @henryiii! Without people like you submitting PRs we couldn't run this project. You rock!

@henryiii henryiii deleted the libomp branch February 28, 2018 19:47
@henryiii
Copy link
Contributor Author

henryiii commented Mar 1, 2018

I've got a patch I'm working on for CMake's FindOpenMP: https://gitlab.kitware.com/cmake/cmake/merge_requests/1812

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants