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] remove unused variables in configure.ac #3509

Merged
merged 1 commit into from
Oct 31, 2020

Conversation

jameslamb
Copy link
Collaborator

Short summary

This removes some unused variables in R-package/configure.ac.

Longer Summary

The CRAN build of the R package contains a file configure, which is a shell script that run whenever the package is installed. It looks around on the system to figure out things like "is OpenMP installed", then fills out variables in a templated Makefile (R-package/src/Makevars.[w]in), which is then used to compile the library.

configure is created from a file R-package/configure.ac using autoconf.

When configure.ac was originally added, I copied and pasted some code from other examples and "Writing R Extensions". That included this snippet:

CC=`"${R_HOME}/bin/R" CMD config CC`
CXX=`"${R_HOME}/bin/R" CMD config CXX11`
CFLAGS=`"${R_HOME}/bin/R" CMD config CFLAGS`
CPPFLAGS=`"${R_HOME}/bin/R" CMD config CPPFLAGS`

Now that I understand this better, I can see that CXX, CFLAGS, and CPPFLAGS are unused...they're not referenced in the AC_SUBST() calls used to replace template string in src/Makevars.[w]in.

How I tested this

I ran the following to test the package, before and after this change:

sh build-cran-package.sh
R CMD INSTALL lightgbm*.tar.gz

The compiler and linker flags used in both cases were identical, and the library installed successfully, so I'm confident that these variables can be removed.

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.

Very weird that CC is used while CXX is not... But if you are confident, OK!

@jameslamb
Copy link
Collaborator Author

Very weird that CC is used while CXX is not... But if you are confident, OK!

ah!!! I think it should be CXX!

We do the malloc check with a C++, not C, compiler, in CMake builds:

https://github.com/microsoft/LightGBM/blob/master/CMakeLists.txt#L241

But that change will require a bit more testing, so I'll do it as a separate PR.

@jameslamb
Copy link
Collaborator Author

Very weird that CC is used while CXX is not... But if you are confident, OK!

re-reading this, I realize I need to clarify....configure.ac is only used to template Makevars.in so that packages can override R's defaults. So R will use the compiler identifier by R CMD config CXX when compiling the library...we just didn't need to access it in configure.ac.

BUT...since we do a special check for malloc.h, we need a compiler for that test program...and that should be using CXX. I'll change that in another PR.

@jameslamb jameslamb merged commit 13194d2 into microsoft:master Oct 31, 2020
@jameslamb jameslamb deleted the fix/configure branch October 31, 2020 05:16
@github-actions
Copy link

This pull request 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 24, 2023
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.

2 participants