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

[R Package] autoconf.ac OpenMP test ignores LDFLAGS (fix provided). #4131

Closed
jlisic opened this issue Mar 28, 2021 · 9 comments · Fixed by #4507
Closed

[R Package] autoconf.ac OpenMP test ignores LDFLAGS (fix provided). #4131

jlisic opened this issue Mar 28, 2021 · 9 comments · Fixed by #4507
Assignees

Comments

@jlisic
Copy link

jlisic commented Mar 28, 2021

Description

The LDFLAGS R variable "R CMD config LDFLAGS" is not included as part of the OpenMP configure.ac test in the R package. This causes the compilation to not find OpenMP libraries due to ignoring the -L directories in .R/Makevars. This is a larger issue on the Mac where -L is usually needed to find the OpenMP libraries. Furthermore, since LDFLAGS are used for the package compilation, it would be a more reasonable test if LDFLAGS was used.

I wrote a fixed autoconf.ac, but I"m not sure how you would want to merge it due to the need to re-run autoconf.

Reproducible example

From the Mac

Current version does not look at the LDFLAGS when compiling the OpenMP test, therefore the test fails

/opt/homebrew/Cellar/llvm/11.1.0/bin/clang -o conftest conftest.c -lomp -Xclang -fopenmp

After adding the changes to autoconf.ac and running autoconf I was able to fix the issue and pass the OpenMP test by adding ${LDFLAGS} to the OpenMP test.

jlisic@565dd56#diff-9363f71bbcd7d64bb47ed62564d0bada0c8073646aaeecfe838068aa5bb17d2d

/opt/homebrew/Cellar/llvm/11.1.0/bin/clang -o conftest conftest.c -L/usr/local/lib -L/opt/homebrew/lib -lomp -L/opt/homebrew/opt/llvm/lib -Wl,-rpath,/opt/homebrew/opt/llvm/lib -lomp -Xclang -fopenmp

Environment info

LightGBM version or commit hash: 3.2.0.99

M1 Mac running big-sur (R 4.0.3.).
home-brew llvm 11.1.0 + libomp

cd lightgbm
R CMD INSTALL .

Additional Comments

~/.R/Makevars

CC=/opt/homebrew/Cellar/llvm/11.1.0/bin/clang
CXX=/opt/homebrew/Cellar/llvm/11.1.0/bin/clang++
CXX11=/opt/homebrew/Cellar/llvm/11.1.0/bin/clang++
CXX14=/opt/homebrew/Cellar/llvm/11.1.0/bin/clang++
CXX17=/opt/homebrew/Cellar/llvm/11.1.0/bin/clang++
CXX20=/opt/homebrew/Cellar/llvm/11.1.0/bin/clang++

CFLAGS=-g -O3 -Wall -pedantic -std=gnu99 -mtune=native -pipe -fopenmp -I/opt/homebrew/include -I/opt/homebrew/opt/llvm/include
CXXFLAGS=-g -O3 -Wall -pedantic -std=c++11 -mtune=native -pipe -fopenmp -I/opt/homebrew/include -I/opt/homebrew/opt/llvm/include
LDFLAGS=-L/usr/local/lib -L/opt/homebrew/lib -lomp -L/opt/homebrew/opt/llvm/lib -Wl,-rpath,/opt/homebrew/opt/llvm/lib

@jameslamb
Copy link
Collaborator

Thanks very much for this excellent report @jlisic ! We'd be grateful for a pull request that adds this, it looks like a good change to me.

I wrote a fixed autoconf.ac, but I"m not sure how you would want to merge it due to the need to re-run autoconf.

If you have docker on your mac, you can regenerate R-package/configure by following the steps at https://github.com/microsoft/LightGBM/blob/master/R-package/README.md#build-a-cran-package.

docker run \
    -v $(pwd):/opt/LightGBM \
    -t ubuntu:20.04 \
    /bin/bash -c "cd /opt/LightGBM && ./R-package/recreate-configure.sh"

Otherwise, if you open a pull request that just updates configure.ac, I can regenerate configure and push to the branch on your fork.

@jameslamb jameslamb added the bug label Mar 28, 2021
@jameslamb
Copy link
Collaborator

By the way, if you're feeling very generous with your time, I think our friends at {xgboost} might benefit from a similar patch: https://github.com/dmlc/xgboost/blob/a59c7323b4fcc739a9b1cbf380e2fe62e60c87ba/R-package/configure.ac#L36

@jlisic
Copy link
Author

jlisic commented Mar 28, 2021

To provide a bit more context here, this is actually an M1 Mac issue, or more specifically, a non-standard dyld search path issue due to the suggested install location moving to /opt/homebrew for native homebrew on M1 Macs. This is not an issue for intel Macs, as homebrew is installed in /usr/local/Hombrew and symlinks shared libraries to /usr/local/lib, as I discovered when I tested this issue on my Mac Pro running Catalina and my MBP running Big Sur.

The bottom line here is that /opt/homebrew/lib (as suggested on the homebrew website) is not searched for when linking against dyld, but /usr/local/lib is (the old default on intel Macs). This would also be the case for anyone using non-standard locations for libomp.

I'll make the suggestion over at xgboost to consider adding the change as this will be a bigger issue as more people use native R on m1 and later designs.

Reference:

search directories (also man dyld will provide info)

Homebrew 3.0.0 suggested Directories

@StrikerRUS
Copy link
Collaborator

A little bit offtopic: we have never tried to build LightGBM on macOS with LLVM's Clang, only with default AppleClang. Good to know that it compiles fine!

@jameslamb
Copy link
Collaborator

@jlisic are you interested in creating a pull request? If not I'll pick this up.

@StrikerRUS
Copy link
Collaborator

Gently ping @jlisic

@jameslamb jameslamb mentioned this issue May 20, 2021
21 tasks
@jameslamb
Copy link
Collaborator

Assigning this to myself and I will pick it up soon, to get this fix into the next release (#4310).

I'd also like to include here two possibly-relevant pieces of information that were recently posted on the R-pkg-devel mailing like.

  1. There is very detailed documentation available at https://cran.r-project.org/doc/manuals/r-release/R-exts.html#OpenMP-support on CRAN's recommendations for using OpenMP in R packages.
  2. @dakep shared a link to this configure.ac that does even more detailed OpenMP detection (checking for specific features that are only available in newer versions): https://github.com/dakep/pense-rpkg/blob/51e9be0e1d6b63bc55b8e650199cff058249050a/configure.ac

@jameslamb
Copy link
Collaborator

I am going to pick this up tonight, to get this fix in for the next release (#4310).

jameslamb added a commit that referenced this issue Aug 19, 2021
* [R-package] fix OpenMP checking on macOS

* adding clang flags back

* fix detection on Mac

* remove CC since it is now unused

* use flags specific to C++11

* regenerate configure
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants