search

dmlc / dmlc-core

A common bricks library for building scalable and portable distributed machine learning.
Apache License 2.0
864 stars 520 forks source link

Add OpenMP as CMake target #586

Closed hcho3 closed 4 years ago

hcho3 commented 4 years ago

Simplify logic for importing OpenMP into the CMake build by treating OpenMP as a target. This way, we no longer have to set compiler flags to enable OpenMP. See https://cliutils.gitlab.io/modern-cmake/chapters/packages/OpenMP.html for more information.

@dmlc/dmlc-core-committer @trivialfis Please help review. This will be especially useful for Mac OSX users. The old way requires them to use Homebrew GCC, which is a heavy dependency (*). The new way allows them to use Apple Clang (default system compiler) instead.

(*) See discussion at https://github.com/Homebrew/homebrew-core/pull/43246, where Homebrew maintainers asked to remove GCC dependency from XGBoost.

hcho3 commented 4 years ago

Sorry for the noise. I'm now done ironing out all loose ends. This PR is now ready for review.

hcho3 commented 4 years ago

@trivialfis Do you mean that we should remove unittest_gtest for OSX?

trivialfis commented 4 years ago

@hcho3 No. I see package gcc@7 is removed, so we are no-longer testing dmlc-core compiled with gcc on OSX. Just curious whether the test is required for dmlc-core.

hcho3 commented 4 years ago

@trivialfis I put back gcc. Now dmlc-core is tested against both gcc and Apple Clang.

hcho3 commented 4 years ago

@larroy

larroy commented 4 years ago

sorry I missed this LGTM