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] fixed overly-strict compiler check in install.libs.R (fixes #2259) #2339

Merged
merged 2 commits into from Aug 21, 2019

Conversation

jameslamb
Copy link
Collaborator

As of #2164 , the R package picked up a fix in install.libs.R that handles difficulty finding OpenMP stuff when building on Mac OS Mojave.

Per discussion on #2259 , we found that that fix only works if environment variable CC is set to something that literally starts with gcc (and the same with CXX and g++). That could cause build failures if you pass a fully-qualified path like CXX=/usr/local/bin/g++-8. Thanks to @brandenkmurray for the help identifying that as the problem!

In this PR, I relax that check so that both g++-8, g++, and /some/path/to/g++-8 will all work. I did not take an opinion on the versions of g++ that are known to work with lightgbm (as suggested in this comment) as I think that would increase our maintenance burden by duplicating the number of places that is specified.

I tested that this works (lucky for us I'm running Mojave 10.14.5 on my laptop) and can confirm it's fixed. Code to test that it worked:

export CC=gcc-8 CXX=g++-8
Rscript build_r.R
export CXX=/usr/local/bin/g++-8 CC=/usr/local/bin/gcc-8
Rscript build_r.R

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Oh, it's really useful fix! It seems that a lot of users are used to specify full paths in CC and CXX!

I guess that Python-package also has the same problem, so I'll propose the same fix for that in a separate PR.

The only thing I'm worrying about is possible collisions. I mean, isn't there any rare compiler name which contains gcc in its' name. For instance, clang and appleclang. According to the CMake docs and Wiki, there are no such names, but maybe I just should search better 😄.

@StrikerRUS StrikerRUS changed the title fixed overly-strict copmiler check in install.libs.R (fixes #2259) [R-package] fixed overly-strict compiler check in install.libs.R (fixes #2259) Aug 20, 2019
@jameslamb
Copy link
Collaborator Author

I did find pgcc (http://www.dartmouth.edu/~rc/HPC/man/pgcc.html) on that Wikipedia page you linked! It's a compiler for C. It does seem like some people might be using it (https://stackoverflow.com/search?tab=newest&q=pgcc). I'll make the regular expression a bit more strict!

@jameslamb
Copy link
Collaborator Author

@StrikerRUS mind taking one more look? I sense that you were half-joking, but I do think guarding against name collisions is probably a good idea.

To check that the regular expressions I introduced here work, you can run these examples in an R session.

gcc <- 'gcc'
gcc8 <- 'gcc-8'
gcc8_fq <- '/usr/bin/whatever/gcc-8'
pgcc <- 'pgcc'
pgcc8 <- 'pgcc-8'
pgcc8_fq <- '/usr/bin/whatever/pgcc-8'

gpp <- 'g++'
gpp8 <- 'g++-8'
gpp8_fq <- '/usr/bin/whatever/g++-8'
whateverpp <- 'somethingg++'
whateverpp8 <- 'somethingg++-8'
whateverpp8_fq <- '/home/root/bin/somethingg++-8'

gcc_pattern <- '^gcc$|[/\\]+gcc|^gcc\\-[0-9]+$|[/\\]+gcc\\-[0-9]+$'

# should be true
grepl(gcc_pattern, gcc)
grepl(gcc_pattern, gcc8)
grepl(gcc_pattern, gcc8_fq)

# should be false
grepl(gcc_pattern, pgcc)
grepl(gcc_pattern, pgcc8)
grepl(gcc_pattern, pgcc8_fq)


gpp_pattern <- '^g\\+\\+$|[/\\]+g\\+\\+|^g\\+\\+\\-[0-9]+$|[/\\]+g\\+\\+\\-[0-9]+$'

# should be true
grepl(gpp_pattern, gpp)
grepl(gpp_pattern, gpp8)
grepl(gpp_pattern, gpp8_fq)

# should be false
grepl(gpp_pattern, whateverpp)
grepl(gpp_pattern, whateverpp8)
grepl(gpp_pattern, whateverpp8_fq)

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot for your research!
I don't think we need to consider some weird things in CC like gcconsole, which will trigger the regex.

@jameslamb jameslamb merged commit 15d861c into microsoft:master Aug 21, 2019
@jameslamb jameslamb deleted the bugfix/r_install_libs branch January 27, 2020 00:15
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants