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] fix MM_PREFETCH and MM_MALLOC checks in configure.ac #3510

Merged
merged 2 commits into from
Nov 1, 2020

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Oct 31, 2020

This PR fixes two bugs in the CRAN package

  • uses a C++ compiler, not a C compiler, for MM_PREFETCH and MM_MALLOC checks
  • removes extra int main(...) scaffolding in MM_PREFETCH and MM_MALLOC checks, which was causing them to fail

More explanation

Look at the diff for R-package/configure and the autoconf documentation on AC_LANG_CONFTEST...autoconf adds that scaffolding for you! So this check was always failing for the CRAN package on Mac and Linux failing for the CRAN on Mac, because the conftest code has an extra int main(...)!!

NOTE: Once CI runs, I'm going to update this and a table here comparing the MM_MALLOC and MM_PREFETCH check results for CRAN vs. CMake builds...I think they'll match now.

Comparison of R 4.0 builds

✅ below means

checking whether <thing> works... yes

❌ means

checking whether <thing> works...no

Latest commit on master: https://github.com/microsoft/LightGBM/actions/runs/338789296

CRAN CMake (gcc) CMake (clang)
Mac MM_PREFETCH ❌
MM_PREFETCH ❌
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
Linux MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅

this PR: https://github.com/microsoft/LightGBM/actions/runs/338808198

CRAN CMake (gcc) CMake (clang)
Mac MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
Linux MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅
MM_PREFETCH ✅

Notes for Reviewers

@StrikerRUS I am really glad you asked me about the compiler used in R-package/configure.ac in #3509 (review). I think I misunderstood your question the first time I read it, but that misunderstanding led me to an investigation that resulted in fixing a bug 😀

@guolinke I think we should try to get this into 3.1.0 (#3484)

@@ -402,6 +402,7 @@ python-package/lightgbm/VERSION.txt

# R build artefacts
**/autom4te.cache/
conftest*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

/gha run r-valgrind

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@jameslamb Happy to see this bugfix! Awesome job, thanks a lot! I remember I was very upset seeing this check failing for CRAN.

@jameslamb jameslamb merged commit 9065d59 into microsoft:master Nov 1, 2020
@jameslamb jameslamb deleted the fix/configure-compiler branch November 1, 2020 04:30
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants