[R-package] Use Rprintf for logging in the R package (fixes #1440, fixes #1909) #2901

jameslamb · 2020-03-12T16:08:09Z

This PR proposes a change to get the R package closer to CRAN's standards (#629). See #1440 and #1909 for background. The basic issue, as documented in Writing R Extensions, is this:

Compiled code is checked for symbols corresponding to functions which might terminate R or write to stdout/stderr instead of the console.

To get LightGBM into compliance with this check, I propose using Rprintf() and Rvprintf() from R's headers. This is the approach CRAN recommends

The most useful function for printing from a C routine compiled into R is Rprintf. This is used in exactly the same way as printf, but is guaranteed to write to R’s output (which might be a GUI console rather than a file, and can be re-directed by sink). It is wise to write complete lines (including the "\n") before returning to R. It is defined in R_ext/Print.h.

Notes for Reviewers

cmake/modules/FindLibR.cmake was included to find R and its headers on any system. It borrows heavily from xgboost's implementation.
I've tested this approach on my personal Travis and it seems to be working. One task (mpi) is failing but I think it is unrelated.
I ran R CMD check lightgbm_2.3.2.tar.gz --as-cran and can confirm that this NOTE is gone!

File ‘/home/travis/build/microsoft/LightGBM/lightgbm.Rcheck/lightgbm/libs/lib_lightgbm.so’:
  Found ‘printf’, possibly from ‘printf’ (C)
  Found ‘putchar’, possibly from ‘putchar’ (C)
  Found ‘stderr’, possibly from ‘stderr’ (C)
  Found ‘stdout’, possibly from ‘stdout’ (C)

Compiled code should not call entry points which might terminate R nor
write to stdout/stderr instead of to the console, nor use Fortran I/O
nor system RNGs. The detected symbols are linked into the code but
might come from libraries and not actually be called.

jameslamb · 2020-03-12T16:17:22Z

R-package/src/cmake/modules/FindLibR.cmake

+    # CRAN may run RD CMD CHECK instead of R CMD CHECK,
+    # which can lead to this infamous error:
+    # 'R' should not be used without a path -- see par. 1.6 of the manual
+    find_program(
+      LIBR_EXECUTABLE
+      NO_DEFAULT_PATH
+      HINTS "${CMAKE_CURRENT_BINARY_DIR}" "/usr/bin" "/usr/lib/"
+      NAMES R R.exe
+    )
+
+    if(LIBR_EXECUTABLE MATCHES ".*lightgbm\\.Rcheck.*")
+      message(FATAL_ERROR "Hit this block. '${LIBR_EXECUTABLE}'")
+    endif()


This section is something I added that you won't find in the xgboost script. It solves an issue that only matters at testing time...basically inside of R CMD check, CRAN actually runs a different command called RD CMD check which uses a special version of R / R.exe. We faced this in pkgnet as well.

From Writing R Extensions

Do not invoke R by plain R, Rscript or (on Windows) Rterm in your examples, tests, vignettes, makefiles or other scripts. As pointed out in several places earlier in this manual, use something like

"$(R_HOME)/bin/Rscript"
"$(R_HOME)/bin$(R_ARCH_BIN)/Rterm"

with appropriate quotes (as, although not recommended, R_HOME can contain spaces).

The hints that I gave here will only work on Unix-alike operating systems. When I do #2335 it will get changed. But since this only matters for testing (in the context of R CMD check), not for normal use, I think it's ok to merge.

Without this block, the builds will fail with this error:

'R' should not be used without a path -- see par. 1.6 of the manual

jameslamb · 2020-03-12T16:18:23Z

include/LightGBM/R_object_helper.h

@@ -39,9 +39,9 @@

  // 64bit pointer
  #if INTPTR_MAX == INT64_MAX
-    typedef int64_t R_xlen_t;
+    typedef int64_t xlen_t;


I had to rename this type to avoid conflicts with R_xlen_t defined in R's headers.

jameslamb · 2020-03-12T16:20:03Z

include/LightGBM/utils/log.h

+  #include <R_ext/Error.h>
+  #include <R_ext/Print.h>
+#endif
+


for those who are new to it:

R_NO_REMAP: Do not change symbols like Rf_error() to error()

R_USE_C99_IN_CXX: Make sure that Rvprintf() is defined. (reference)

Note: R_USE_C99_IN_CXX is defined in R_ext/Print.h in R 4.0. Will this break when using R 4.0?

R-package/src/cmake/modules/FindLibR.cmake

jameslamb · 2020-03-12T18:16:42Z

Travis is struggling today, seeing a lot of these

StrikerRUS

@jameslamb Awesome job!

Below are some initial comments.

R-package/src/cmake/modules/FindLibR.cmake

jameslamb · 2020-03-14T05:29:46Z

I just tried building this branch on my Windows laptop (Windows 10, R3.6.1, Visual Studio 16 2019) with Rscript build_r.R and can confirm this worked!

The first time I ran I did hit https://github.com/jameslamb/LightGBM/blob/feat/r-header/R-package/src/cmake/modules/FindLibR.cmake#L41, but that was easy to fix by following the error message and adding the Rtools minGW bin/ directory to the path.

R-package/src/cmake/modules/FindLibR.cmake

StrikerRUS

@jameslamb I left some comments. Please take a look.

CMakeLists.txt

R-package/src/cmake/modules/FindLibR.cmake

jameslamb · 2020-03-17T16:54:02Z

I just rebased to master since there have been a lot of changes on there since this PR was opened. Made sure to pull the suggestions committed in the browser before doing that, so they didn't get reverted by the force push.

Laurae2 · 2020-03-17T20:11:25Z

include/LightGBM/utils/log.h

+
+    // R code should write back to R's error stream,
+    // otherwise to stderr
+    #ifndef LGB_R_BUILD


We should agree on preprocessor directive indentations on a later PR. ping @guolinke @StrikerRUS @jameslamb for later.

Laurae2

See R-devel (R 4.0): https://cran.r-project.org/doc/manuals/r-devel/NEWS.html in C-level facilities:

Header ‘R_ext/Print.h’ defines R_USE_C99_IN_CXX and hence exposes Rvprintf and REvprintf if used with a C++11 (or later) compiler.

Do we need a workaround for it?

Laurae2 · 2020-03-17T20:18:29Z

include/LightGBM/utils/log.h

+  #include <R_ext/Error.h>
+  #include <R_ext/Print.h>
+#endif
+


Note: R_USE_C99_IN_CXX is defined in R_ext/Print.h in R 4.0. Will this break when using R 4.0?

jameslamb · 2020-03-17T21:05:58Z

See R-devel (R 4.0): https://cran.r-project.org/doc/manuals/r-devel/NEWS.html in C-level facilities:

Header ‘R_ext/Print.h’ defines R_USE_C99_IN_CXX and hence exposes Rvprintf and REvprintf if used with a C++11 (or later) compiler.

Do we need a workaround for it?

Still investigating, but for the sake of documenting the discussion...divest (as one example), uses this same pattern and recently passed CRAN nightly checks against r-devel (which is 4.0.0 now) https://cran.r-project.org/web/checks/check_results_divest.html

Laurae2

It seems there is no issue for R_USE_C99_IN_CXX according to R_ext/Print.h in R-devel (4.0):

/*
 *  R : A Computer Language for Statistical Data Analysis
 *  Copyright (C) 1998-2019    The R Core Team
 *
 *  This header file is free software; you can redistribute it and/or modify
 *  it under the terms of the GNU Lesser General Public License as published by
 *  the Free Software Foundation; either version 2.1 of the License, or
 *  (at your option) any later version.
 *
 *  This file is part of R. R is distributed under the terms of the
 *  GNU General Public License, either Version 2, June 1991 or Version 3,
 *  June 2007. See doc/COPYRIGHTS for details of the copyright status of R.
 *
 *  This program is distributed in the hope that it will be useful,
 *  but WITHOUT ANY WARRANTY; without even the implied warranty of
 *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *  GNU Lesser General Public License for more details.
 *
 *  You should have received a copy of the GNU Lesser General Public License
 *  along with this program; if not, a copy is available at
 *  https://www.R-project.org/Licenses/
 */

/* Included by R.h: API */

#ifndef R_EXT_PRINT_H_
#define R_EXT_PRINT_H_

#ifdef  __cplusplus
/* If the vprintf interface is defined at all in C++ it may only be
   defined in namespace std.  It is part of the C++11 standard. */
# if __cplusplus >= 201103L && !defined(R_USE_C99_IN_CXX)
#  define R_USE_C99_IN_CXX
# endif
# ifdef R_USE_C99_IN_CXX
#  include <cstdarg>
#  define R_VA_LIST std::va_list
# endif
extern "C" {
#else
# include <stdarg.h>
# define R_VA_LIST va_list
#endif

jameslamb · 2020-03-17T21:31:02Z

Thanks for the review @Laurae2 ! I won't merge until @StrikerRUS and I agree on some of the lingering comments on FindLibR.cmake

jameslamb · 2020-03-19T15:04:12Z

@StrikerRUS I saw you resolved the comment threads on FindLibR.cmake. Am I ok to merge this?

Thanks for the review! btw I made PR into xgboost with one of the improvements you suggested: dmlc/xgboost#5427

StrikerRUS · 2020-03-19T16:10:49Z

@jameslamb Please give me some time to make a second round of review.

jameslamb · 2020-03-19T16:13:16Z

@jameslamb Please give me some time to make a second round of review.

sure no hurry at all! Glad that I asked

StrikerRUS

@jameslamb Finally got a minute to drop a second round of review. Sorry for the delay!

CMakeLists.txt

R-package/src/cmake/modules/FindLibR.cmake

jameslamb · 2020-03-22T22:45:28Z

@jameslamb Finally got a minute to drop a second round of review. Sorry for the delay!

no problem! I'll look at these right now.

Co-Authored-By: Nikita Titov <nekit94-08@mail.ru>

R-package/src/cmake/modules/FindLibR.cmake

jameslamb · 2020-03-23T19:03:07Z

R + Mac CI on Travis died with this error:

Looks like a timeout for Homebrew. I have been seeing that slightly more the last few weeks. I imagine all the people now working remote is putting a lot of pressure on ISPs.

(just restarted it manually in the Travis console)

StrikerRUS · 2020-03-23T19:06:55Z

@jameslamb

Looks like a timeout for Homebrew. I have been seeing that slightly more the last few weeks. I imagine all the people now working remote is putting a lot of pressure on ISPs.

Yeah, it seems to be so...

I cannot remember how many times I re-run Travis macOS builds last days. Also, Travis had some problems with macOS recently: https://www.traviscistatus.com/

StrikerRUS

@jameslamb Overall looks good! But please take a look at some my questions below.

CMakeLists.txt

R-package/src/cmake/modules/FindLibR.cmake

Co-Authored-By: Nikita Titov <nekit94-08@mail.ru>

StrikerRUS

@jameslamb LGTM! Great step towards CRAN release! 🎉

Thanks a lot for your patience!

…#1440, fixes microsoft#1909) (microsoft#2901) * [R-package] started cutting over from custom R-to-C interface to R.h * replaced LGBM_SE with SEXP * fixed error about ocnflicting definitions of length * got linking working * more stuff * eliminated R CMD CHECK note about printing * switched from hard-coded include dir to the one from FindLibR.cmake * cleaned up formatting in FindLibR.cmake * commented-out everything in CI that does not touch R * more changes * trying to get better logs * tried ignoring * added error message to confirm a suspicion * still trying to find R during R CMD CHECK * restore full CI * fixed comment * Update R-package/src/cmake/modules/FindLibR.cmake * changed strategy for finding LIBR_HOME on Windows * Removed 32-bit Windows stuff in FindLibR.cmake * Update R-package/src/cmake/modules/FindLibR.cmake * Update CMakeLists.txt Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * Update CMakeLists.txt Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * Update R-package/src/cmake/modules/FindLibR.cmake Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * removed some duplication in cmake scripts * Update R-package/src/cmake/modules/FindLibR.cmake Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * Update R-package/src/cmake/modules/FindLibR.cmake Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * Update R-package/src/cmake/modules/FindLibR.cmake Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * Update R-package/src/cmake/modules/FindLibR.cmake Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * Update R-package/src/cmake/modules/FindLibR.cmake Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * Update R-package/src/cmake/modules/FindLibR.cmake Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * added LIBR_CORE_LIBRARY back * small fixes to CMakeLists * simplified FindLibR.cmake * some fixes for windows * Apply suggestions from code review Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * allowed for directly passing LIBR_EXECUTABLE to FindLibR.cmake * reorganized FindLibR.cmake to catch more cases * clean up inconsistencies in R calls in FindLibR.cmake * Update R-package/src/cmake/modules/FindLibR.cmake Co-Authored-By: Nikita Titov <nekit94-08@mail.ru> * removed unnecessary log messages * removed unnecessary unset() call Co-authored-by: Nikita Titov <nekit94-08@mail.ru>

github-actions · 2023-08-24T13:31:46Z

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.

jameslamb requested review from chivee, Laurae2, guolinke and StrikerRUS March 12, 2020 16:08

jameslamb commented Mar 12, 2020

View reviewed changes

R-package/src/cmake/modules/FindLibR.cmake Outdated Show resolved Hide resolved

StrikerRUS reviewed Mar 12, 2020

View reviewed changes

jameslamb added the feature label Mar 14, 2020

This was referenced Mar 14, 2020

[R-package] Ability to control the printed messages? #1440

Closed

[R-package] adding routine registration in R package (fixes #1910) #2911

Merged

[R-Package] CRAN issues #629

Closed

jameslamb added the awaiting review label Mar 14, 2020

jameslamb commented Mar 16, 2020

View reviewed changes

R-package/src/cmake/modules/FindLibR.cmake Outdated Show resolved Hide resolved

StrikerRUS reviewed Mar 16, 2020

View reviewed changes

jameslamb force-pushed the feat/r-header branch from 7568546 to 73eafe5 Compare March 17, 2020 16:09

Laurae2 reviewed Mar 17, 2020

View reviewed changes

Laurae2 approved these changes Mar 17, 2020

View reviewed changes

jameslamb added 5 commits March 17, 2020 20:05

[R-package] started cutting over from custom R-to-C interface to R.h

d23e086

replaced LGBM_SE with SEXP

7e03224

fixed error about ocnflicting definitions of length

88b4950

got linking working

f12eaa2

more stuff

35e1b42

some fixes for windows

4375584

jameslamb mentioned this pull request Mar 20, 2020

[R-package] [ci] added more linters #2803

Merged

StrikerRUS reviewed Mar 22, 2020

View reviewed changes

Apply suggestions from code review

561ac4e

Co-Authored-By: Nikita Titov <nekit94-08@mail.ru>

StrikerRUS reviewed Mar 22, 2020

View reviewed changes

R-package/src/cmake/modules/FindLibR.cmake Outdated Show resolved Hide resolved

allowed for directly passing LIBR_EXECUTABLE to FindLibR.cmake

2403a61

jameslamb mentioned this pull request Mar 23, 2020

[R-package] [ci] Add Windows CI for R package (fixes #2335) #2936

Merged

jameslamb added 2 commits March 23, 2020 11:37

reorganized FindLibR.cmake to catch more cases

5b60e8e

clean up inconsistencies in R calls in FindLibR.cmake

c9dd50d

StrikerRUS reviewed Mar 23, 2020

View reviewed changes

jameslamb and others added 3 commits March 23, 2020 17:24

Update R-package/src/cmake/modules/FindLibR.cmake

c00c928

Co-Authored-By: Nikita Titov <nekit94-08@mail.ru>

removed unnecessary log messages

20aeb82

removed unnecessary unset() call

ab870d8

StrikerRUS approved these changes Mar 24, 2020

View reviewed changes

StrikerRUS removed the awaiting review label Mar 24, 2020

StrikerRUS merged commit 0341906 into microsoft:master Mar 24, 2020

jameslamb deleted the feat/r-header branch March 25, 2020 19:54

jameslamb mentioned this pull request Mar 31, 2020

[R-package] CMake fixes to support MSVC #2963

Merged

jameslamb mentioned this pull request Sep 7, 2020

[R-package] Remove ability to install precompiled lib_lightgbm (fixes #3320) #3360

Merged

This was referenced Oct 4, 2020

[R-package] changes to support read/write performance in CMake builds feedzai/LightGBM#4

Closed

Fix model locale issue and improve model R/W performance. #3405

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R-package] Use Rprintf for logging in the R package (fixes #1440, fixes #1909) #2901

[R-package] Use Rprintf for logging in the R package (fixes #1440, fixes #1909) #2901

jameslamb commented Mar 12, 2020 •

edited

Loading

jameslamb Mar 12, 2020

jameslamb Mar 12, 2020

jameslamb Mar 12, 2020

Laurae2 Mar 17, 2020

jameslamb commented Mar 12, 2020

StrikerRUS left a comment

jameslamb commented Mar 14, 2020

StrikerRUS left a comment

jameslamb commented Mar 17, 2020

Laurae2 Mar 17, 2020

Laurae2 left a comment

Laurae2 Mar 17, 2020

jameslamb commented Mar 17, 2020

Laurae2 left a comment

jameslamb commented Mar 17, 2020

jameslamb commented Mar 19, 2020

StrikerRUS commented Mar 19, 2020

jameslamb commented Mar 19, 2020

StrikerRUS left a comment

jameslamb commented Mar 22, 2020

jameslamb commented Mar 23, 2020 •

edited

Loading

StrikerRUS commented Mar 23, 2020

StrikerRUS left a comment

StrikerRUS left a comment

github-actions bot commented Aug 24, 2023

[R-package] Use Rprintf for logging in the R package (fixes #1440, fixes #1909) #2901

[R-package] Use Rprintf for logging in the R package (fixes #1440, fixes #1909) #2901

Conversation

jameslamb commented Mar 12, 2020 • edited Loading

jameslamb Mar 12, 2020

Choose a reason for hiding this comment

jameslamb Mar 12, 2020

Choose a reason for hiding this comment

jameslamb Mar 12, 2020

Choose a reason for hiding this comment

Laurae2 Mar 17, 2020

Choose a reason for hiding this comment

jameslamb commented Mar 12, 2020

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented Mar 14, 2020

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented Mar 17, 2020

Laurae2 Mar 17, 2020

Choose a reason for hiding this comment

Laurae2 left a comment

Choose a reason for hiding this comment

Laurae2 Mar 17, 2020

Choose a reason for hiding this comment

jameslamb commented Mar 17, 2020

Laurae2 left a comment

Choose a reason for hiding this comment

jameslamb commented Mar 17, 2020

jameslamb commented Mar 19, 2020

StrikerRUS commented Mar 19, 2020

jameslamb commented Mar 19, 2020

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented Mar 22, 2020

jameslamb commented Mar 23, 2020 • edited Loading

StrikerRUS commented Mar 23, 2020

StrikerRUS left a comment

Choose a reason for hiding this comment

StrikerRUS left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2023

jameslamb commented Mar 12, 2020 •

edited

Loading

jameslamb commented Mar 23, 2020 •

edited

Loading