[R-package] use C++ compiler for pre-compile checks on Windows #4504

jameslamb · 2021-08-09T02:13:24Z

Looking into #4131 as part of #4310, I've noticed some other areas for possible improvement in the configure and configure.win scripts.

This PR fixes one in configure.win. I noticed that the mm_prefetch and mm_malloc tests are being run with a C compiler on Windows. They should be done with a C++ compiler, as they are on Linux/Mac

LightGBM/R-package/configure.ac

Line 41 in 5fe27d5

AC_LANG(C++)

and for all CMake-based builds

LightGBM/CMakeLists.txt

Line 253 in 5fe27d5

check_cxx_source_compiles("

I'm not aware of any problems caused by the fact that these checks were being done by a C compiler, but a C++ compiler is going to be used for compiling lib_lightgbm.

Notes for Reviewers

To understand the difference between CXXFLAGS and CPPFLAGS and why they are both included, see the documentation from R CMD config.

CPPFLAGS C/C++ preprocessor flags, e.g. -I<dir> if you have headers in a nonstandard directory <dir>
CXXFLAGS compiler flags for CXX

jameslamb · 2021-08-09T21:34:49Z

I'm seeing several errors in CI today about connecting to GitHub.

For example, from an R package job: https://github.com/microsoft/LightGBM/pull/4504/checks?check_run_id=3282998279

  "C:\Program Files\Git\bin\git.exe" -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=5 origin +d08d1f3c51a3f4876ca39d591097269b68cc9bc3:refs/remotes/pull/4504/merge
  Error: fatal: unable to access 'https://github.com/microsoft/LightGBM/': Failed to connect to github.com port 443: Timed out
  The process 'C:\Program Files\Git\bin\git.exe' failed with exit code 128
  Waiting 11 seconds before trying again
  "C:\Program Files\Git\bin\git.exe" -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=5 origin +d08d1f3c51a3f4876ca39d591097269b68cc9bc3:refs/remotes/pull/4504/merge
  Error: fatal: unable to access 'https://github.com/microsoft/LightGBM/': Failed to connect to github.com port 443: Timed out
  The process 'C:\Program Files\Git\bin\git.exe' failed with exit code 128
  Waiting 14 seconds before trying again
  "C:\Program Files\Git\bin\git.exe" -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=5 origin +d08d1f3c51a3f4876ca39d591097269b68cc9bc3:refs/remotes/pull/4504/merge
  Error: fatal: unable to access 'https://github.com/microsoft/LightGBM/': Failed to connect to github.com port 443: Timed out
  Error: The process 'C:\Program Files\Git\bin\git.exe' failed with exit code 128

And a Python-package job: https://github.com/microsoft/LightGBM/pull/4504/checks?check_run_id=3282997602

  Submodule path 'external_libs/fast_double_parser': checked out 'ace60646c02dc54c57f19d644e49a61e7e7758ec'
  Error: fatal: unable to access 'https://github.com/fmtlib/fmt.git/': Failed to connect to github.com port 443: Operation timed out
  Fetched in submodule path 'external_libs/fmt', but it did not contain cc09f1a6798c085c325569ef466bcdcffdc266d4. Direct fetching of that commit failed.
  Error: The process '/usr/local/bin/git' failed with exit code 1

It does not seem like there were any incidents on GitHub today (https://www.githubstatus.com/history). Azure is not reporting any recent incidents either (https://status.azure.com/en-us/status/history/).

I'm not sure what's happening :/

StrikerRUS · 2021-08-10T12:26:23Z

/gha run r-solaris

Workflow Solaris CRAN check has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/1116581396

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-8fbce35a96824fd9bc025ca6079ab602
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-7f6f062abb6744b789eaa3dcd31cd71a
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
Status: success ✔️.

StrikerRUS · 2021-08-10T12:26:32Z

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/1116582111

Status: failure ❌.

jameslamb · 2021-08-10T13:48:21Z

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/1116855777

Status: success ✔️.

jameslamb · 2021-08-10T13:50:00Z

Just pushed 4884a69, adding {Matrix} to the setup for the valgrind tests. It is an Imports-level dependency so it should be installed. Maybe one of the other dependencies was introducing it before and that's why it was ok for us to not explicitly install.packages() it.

jameslamb · 2021-08-10T16:57:22Z

Installing {Matrix} seemed to fix the failures in the valgrind job!

#4504 (comment)

StrikerRUS · 2021-08-10T23:17:20Z

Should we add Matrix in other places for the consistency, e.g. in config for sanitizers or debian clang?

jameslamb · 2021-08-10T23:38:17Z

Should we add Matrix in other places for the consistency, e.g. in config for sanitizers or debian clang?

I think so, yeah. I'll be away from my computer for a few hours. You can push a commit here directly if you'd like, otherwise I'll do it later tonight.

jameslamb · 2021-08-11T02:05:43Z

Added {Matrix} explicitly in the other places where we install R dependencies: b43b6c7

StrikerRUS

Good sign in Windows jobs!

checking whether MM_PREFETCH works...yes
checking whether MM_MALLOC works...yes

Please check two my questions about consistency with Unix version of configure below

R-package/configure.win

StrikerRUS · 2021-08-11T20:24:29Z

R-package/configure.win

@@ -6,7 +6,9 @@
 ###########################

 R_EXE="${R_HOME}/bin${R_ARCH_BIN}/R"
-CC=`"${R_EXE}" CMD config CC`
+CXX=`"${R_EXE}" CMD config CXX`


Why do we use CXX11 for Unix and CXX here to configure CXX variable (considering #4506 as well)??

LightGBM/R-package/configure.ac

Line 24 in b43b6c7

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

ah! thanks for noticing, that is an oversight. It should be CXX11 since we explicitly set a C++ standard

LightGBM/R-package/src/Makevars.win.in

Line 1 in 926526c

CXX_STD = CXX11

From https://cran.r-project.org/doc/manuals/R-exts.html#Configure-and-cleanup

If the package specifies a non-default C++ standard, use the config variable names (such as CXX17) appropriate to the standard

From R's perspective, our use of C++ is "non-default". See https://cran.r-project.org/doc/manuals/R-exts.html#Using-C_002b_002b-code.

As from R 4.0.0 a C++ compiler will be selected only if it conforms to the 2011 standard (‘C++11’). A minor update43 (‘C++14’) was published in December 2014 and will be used by default as from R 4.1.0 if supported

updated in b576a4f

Seems we are setting less variables than it's done in the example from https://cran.r-project.org/doc/manuals/R-exts.html#Using-C_002b_002b-code.

CXX11=`"${R_EXE}" CMD config CXX11` CXX11STD=`"${R_EXE}" CMD config CXX11STD` CXX="${CXX11} ${CXX11STD}" CXXFLAGS=`"${R_EXE}" CMD config CXX11FLAGS` CPPFLAGS=`"${R_EXE}" CMD config CPPFLAGS`

Also note that CXX11FLAGS is used to configure CXXFLAGS.

I was able to find a similar code in the wild by searching for CXX11FLAGS filename:configure.ac. For example,
https://github.com/cbg-ethz/MC-CBN/blob/6cd55b8289d22809a5029981f87cd1cbe5be27ea/configure.ac#L21-L38
https://github.com/cran/spp/blob/2603673566115871405f5842e489454805b9f5c0/configure.ac#L15-L22
https://github.com/akhikolla/ClusterTests/blob/1c7702ba4035511ffe7d4dd27e9021b6962718fe/BART/configure.ac#L19-L22

oooo interesting, I missed that! Thanks for pointing it out. I just made those changes here and on #4506 .

StrikerRUS · 2021-08-14T16:48:24Z

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/1130908327

Status: success ✔️.

StrikerRUS · 2021-08-14T16:48:40Z

/gha run r-solaris

Workflow Solaris CRAN check has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/1130908563

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-ffbce948622047e4a07e5e413fcbbb8d
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-381203fd44294e08bc4e13ac3b2d374d
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
Status: success ✔️.

StrikerRUS

Thank you very much for this PR! LGTM!

github-actions · 2023-08-23T18:55:42Z

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.

[R-package] use C++ compiler for pre-compile checks on Windows

7b68f50

jameslamb added the fix label Aug 9, 2021

jameslamb requested a review from StrikerRUS August 9, 2021 02:13

jameslamb requested a review from Laurae2 as a code owner August 9, 2021 02:13

Merge branch 'master' into fix/cpp-conftest

54c07e2

Merge branch 'master' into fix/cpp-conftest

5eab06b

install Matrix in valgrind test

4884a69

Add {Matrix} in more places in CI and docs

b43b6c7

jameslamb added the awaiting review label Aug 11, 2021

StrikerRUS removed the awaiting review label Aug 11, 2021

StrikerRUS reviewed Aug 11, 2021

View reviewed changes

jameslamb added 4 commits August 13, 2021 20:42

use CXX11

b576a4f

use flags specific to C++11

e06bdf5

Merge branch 'master' into fix/cpp-conftest

7fec6b3

missing backtick

04799cb

jameslamb mentioned this pull request Aug 14, 2021

[R-package] pass R-configured compiler flags to checks in configure #4506

Merged

StrikerRUS approved these changes Aug 14, 2021

View reviewed changes

jameslamb merged commit 86ead20 into master Aug 14, 2021

jameslamb deleted the fix/cpp-conftest branch August 14, 2021 19:04

github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R-package] use C++ compiler for pre-compile checks on Windows #4504

[R-package] use C++ compiler for pre-compile checks on Windows #4504

jameslamb commented Aug 9, 2021

jameslamb commented Aug 9, 2021

StrikerRUS commented Aug 10, 2021 •

edited by guolinke

Loading

StrikerRUS commented Aug 10, 2021 •

edited by guolinke

Loading

jameslamb commented Aug 10, 2021 •

edited by guolinke

Loading

jameslamb commented Aug 10, 2021

jameslamb commented Aug 10, 2021

StrikerRUS commented Aug 10, 2021

jameslamb commented Aug 10, 2021

jameslamb commented Aug 11, 2021

StrikerRUS left a comment •

edited

Loading

StrikerRUS Aug 11, 2021

jameslamb Aug 14, 2021

StrikerRUS Aug 14, 2021

StrikerRUS Aug 14, 2021

jameslamb Aug 14, 2021

StrikerRUS commented Aug 14, 2021 •

edited by guolinke

Loading

StrikerRUS commented Aug 14, 2021 •

edited by guolinke

Loading

StrikerRUS left a comment

github-actions bot commented Aug 23, 2023

[R-package] use C++ compiler for pre-compile checks on Windows #4504

[R-package] use C++ compiler for pre-compile checks on Windows #4504

Conversation

jameslamb commented Aug 9, 2021

Notes for Reviewers

jameslamb commented Aug 9, 2021

StrikerRUS commented Aug 10, 2021 • edited by guolinke Loading

StrikerRUS commented Aug 10, 2021 • edited by guolinke Loading

jameslamb commented Aug 10, 2021 • edited by guolinke Loading

jameslamb commented Aug 10, 2021

jameslamb commented Aug 10, 2021

StrikerRUS commented Aug 10, 2021

jameslamb commented Aug 10, 2021

jameslamb commented Aug 11, 2021

StrikerRUS left a comment • edited Loading

Choose a reason for hiding this comment

StrikerRUS Aug 11, 2021

Choose a reason for hiding this comment

jameslamb Aug 14, 2021

Choose a reason for hiding this comment

StrikerRUS Aug 14, 2021

Choose a reason for hiding this comment

StrikerRUS Aug 14, 2021

Choose a reason for hiding this comment

jameslamb Aug 14, 2021

Choose a reason for hiding this comment

StrikerRUS commented Aug 14, 2021 • edited by guolinke Loading

StrikerRUS commented Aug 14, 2021 • edited by guolinke Loading

StrikerRUS left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 23, 2023

StrikerRUS commented Aug 10, 2021 •

edited by guolinke

Loading

StrikerRUS commented Aug 10, 2021 •

edited by guolinke

Loading

jameslamb commented Aug 10, 2021 •

edited by guolinke

Loading

StrikerRUS left a comment •

edited

Loading

StrikerRUS commented Aug 14, 2021 •

edited by guolinke

Loading

StrikerRUS commented Aug 14, 2021 •

edited by guolinke

Loading