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

Fix build on macOS Mojave #1923

Merged
merged 8 commits into from
Jan 26, 2019
Merged

Fix build on macOS Mojave #1923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2ad6f07
Fix build on macOS Mojave
henry0312 Dec 22, 2018
c121905
update setup.py
henry0312 Dec 24, 2018
ac3f5f1
update docs
henry0312 Dec 24, 2018
2180fc4
fix setup.py
henry0312 Dec 24, 2018
c723f3f
update docs
henry0312 Dec 24, 2018
2131023
update docs
henry0312 Dec 24, 2018
d2a3c20
update setup.py
henry0312 Dec 24, 2018
2e96aa9
update docs
henry0312 Dec 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,8 +211,8 @@ endif(USE_MPI)

if(USE_OPENMP)
if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
TARGET_LINK_LIBRARIES(lightgbm ${OpenMP_libomp_LIBRARY})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://gist.github.com/henry0312/c2e0550caf7acfce2e60a24984c322e9#file-make1-log

TARGET_LINK_LIBRARIES(_lightgbm ${OpenMP_libomp_LIBRARY})
TARGET_LINK_LIBRARIES(lightgbm OpenMP::OpenMP_CXX)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://gist.github.com/henry0312/c2e0550caf7acfce2e60a24984c322e9#file-make2-log

TARGET_LINK_LIBRARIES(_lightgbm OpenMP::OpenMP_CXX)
endif()
endif(USE_OPENMP)

Expand Down
25 changes: 25 additions & 0 deletions docs/Installation-Guide.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,19 @@ Only **Apple Clang** version 8.1 or higher is supported.

git clone --recursive https://github.com/Microsoft/LightGBM ; cd LightGBM
mkdir build ; cd build

# For Mojave (10.14)
henry0312 marked this conversation as resolved.
Show resolved Hide resolved
cmake \
-DOpenMP_C_FLAGS="-Xpreprocessor -fopenmp -I$(brew --prefix libomp)/include" \
-DOpenMP_C_LIB_NAMES="omp" \
-DOpenMP_CXX_FLAGS="-Xpreprocessor -fopenmp -I$(brew --prefix libomp)/include" \
-DOpenMP_CXX_LIB_NAMES="omp" \
-DOpenMP_omp_LIBRARY=$(brew --prefix libomp)/lib/libomp.dylib \
..

# For High Sierra or earlier (<= 10.13)
cmake ..

make -j4

gcc
Expand Down Expand Up @@ -436,7 +448,20 @@ Only **Apple Clang** version 8.1 or higher is supported.

git clone --recursive https://github.com/Microsoft/LightGBM ; cd LightGBM
mkdir build ; cd build

# For Mojave (10.14)
cmake \
-DUSE_MPI=ON \
-DOpenMP_C_FLAGS="-Xpreprocessor -fopenmp -I$(brew --prefix libomp)/include" \
-DOpenMP_C_LIB_NAMES="omp" \
-DOpenMP_CXX_FLAGS="-Xpreprocessor -fopenmp -I$(brew --prefix libomp)/include" \
-DOpenMP_CXX_LIB_NAMES="omp" \
-DOpenMP_omp_LIBRARY=$(brew --prefix libomp)/lib/libomp.dylib \
..

# For High Sierra or earlier (<= 10.13)
cmake -DUSE_MPI=ON ..

make -j4

gcc
Expand Down
14 changes: 14 additions & 0 deletions python-package/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,22 @@ def compile_cpp(use_mingw=False, use_gpu=False, use_mpi=False, nomp=False,
cmake_cmd.append("-DUSE_MPI=ON")
if nomp:
cmake_cmd.append("-DUSE_OPENMP=OFF")
if system() == 'Darwin' and not nomp:
cc = os.environ.get('CC')
cxx = os.environ.get('CXX')
if not (cc and cc.startswith('gcc') and cxx and cxx.startswith('g++')):
# Apple Clang
# https://github.com/Homebrew/homebrew-core/pull/20589
cmake_cmd.extend([
Copy link
Collaborator

@StrikerRUS StrikerRUS Dec 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We shouldn't use hardcoded paths for the first build attempt. Let's use hardcoded paths only after first attempt fail:

...
 else:  # Linux, Darwin (macOS), etc.
        logger.info("Starting to compile with CMake.")
        if system() == 'Darwin' and not nomp and not (cc and cc.startswith('gcc') and cxx and cxx.startswith('g++')):
            status = silent_call(cmake_cmd)  # first build attempt with CMake OpenMP autodetection (current behavior)
            if status != 0:
                logger.warning("Compilation failed.")
                logger.info("Starting to compile with OpenMP path guesses.")
                clear_path(os.path.join(CURRENT_DIR, "build_cpp"))
                <your modification here>  # second attempt with hardcoded OpenMP paths for Homebrew
        else:
            silent_call(cmake_cmd, raise_error=True, error_msg='Please install CMake and all required dependencies first')
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't use hardcoded paths for the first build attempt.

Why is this?
I think this trial is a really waste.

Copy link
Collaborator

@StrikerRUS StrikerRUS Dec 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

  1. This workaround was tested only on High Sierra and Mojave. We don't know how it works on earlier versions. However, the current method was tested by the time and there were no any complaints.
  2. There could be some other problems with this workaround which we don't know.
  3. User can install OpenMP in another folder.
  4. CMake team will repair OpenMP detection sometime and the current method will work fine again without any new hotfixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, it is correct for macOS users to set cmake options for AppleClang with OpenMP.
In ohter words, the current way (cmake ..) is wrong (than this change) because libomp requires the options.

Copy link
Contributor

@Laurae2 Laurae2 Dec 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@henry0312 @StrikerRUS What about something like this instead in README.md or whatever else for comments? (and let the user change the paths?)

DOpenMP_C_LIB_NAMES="omp" 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.dylib bash -c 'cmake ..'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Laurae2 I think that is one of options that we take, but that won't allow us to run just python setup.py install.
It is useful for many users.
And the fixed documetns are in https://github.com/Microsoft/LightGBM/pull/1923/files#diff-1f74fbb61fb97279854ec1f63430b9bb (to be more fixed for some comments).

Copy link
Collaborator

@StrikerRUS StrikerRUS Dec 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@henry0312

In ohter words, the current way (cmake ..) is wrong (than this change) because libomp requires the options.

Maybe... But if everything works (OK, worked before 10.14), why should we ask users do more things then needed?

@henry0312 @Laurae2 Hmm, what do you think about new setup args openmp-include-dir and openmp-library like we already have for GPU version?

Then we can do the following:

1. Try to compile with default "cmake .." for earlier macOS versions and future CMake changes.
2. If new args are provided, compile with them.
3. If not 2. (not provided or failed), then try to compile with path guesses for the maximum backward compatibility and user's happiness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why should we ask users do more things then needed?

No, no.
This change will never ask users to do more anything.
For users, nothing will be changed.
For us, this change will correct building options. That's all.

'-DOpenMP_C_FLAGS=-Xpreprocessor -fopenmp -I/usr/local/opt/libomp/include',
'-DOpenMP_C_LIB_NAMES=omp',
'-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.dylib'
])
if use_hdfs:
cmake_cmd.append("-DUSE_HDFS=ON")

if system() in ('Windows', 'Microsoft'):
if use_mingw:
if use_mpi:
Expand Down