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

jameslamb · 2020-03-23T02:38:50Z

This PR adds CI support for the R package on Windows. It also bumps up the version of R we test against (on all operating systems) from 3.6.1 to 3.6.3.

As of this PR, 4 R CMD check NOTES remain on Windows. All of them can be addressed in later PRs and none of them were introduced by changes in this PR.

R CMD CHECK NOTES (click for details)

note-1

* checking package dependencies ... NOTE
Packages suggested but not available for checking:
  'ggplot2', 'knitr', 'rmarkdown'

note-2

* checking installed package size ... NOTE
  installed size is  6.9Mb
  sub-directories of 1Mb or more:
    libs   6.3Mb

note-3

* checking compiled code ... NOTE
File 'lightgbm/libs/x64/lib_lightgbm.dll':
  Found no calls to: 'R_registerRoutines', 'R_useDynamicSymbols'

It is good practice to register native routines and to disable symbol
search.

note-4

* checking compiled code ... NOTE
Note: information on .o files for x64 is not available
File 'C:/projects/lightgbm/lightgbm.Rcheck/lightgbm/libs/x64/lib_lightgbm.dll':
  Found 'abort', possibly from 'abort' (C), 'runtime' (Fortran)
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.

note-1: not relevant for us, won't actually show up on CRAN and we shouldn't incur the extra time to install those packages. CRAN will have all Suggests dependencies available.
note-2: only showing up on Windows. Probably some build files not caught by .Rbuildignore. Can be addressed in a follow-up PR.
note-3: addressed by [R-package] adding routine registration in R package (fixes #1910) #2911
note-4: Can be addressed in a follow-up PR.

Currently, .appveyor.yml on master only has code for the Python package. It has some code there (like setting up the test-env conda environment) that is identical to code in .vsts-ci.yml. In this PR, I decided to consolidate that code in .ci/test_windows.ps1. That made it easier to do things like if $env:TASK -eq "r-package", and is similar to how we handle Mac and Linux in .ci/test.sh.

The R portion of .ci/test_windows.ps1 borrows heavily from @StrikerRUS 's setup for the RGF package (https://github.com/RGF-team/rgf/blob/master/R-package/.R.appveyor.ps1). Thank you for that!

This PR has so many commits in it because I spent a lot of time getting this working in the Travis, Appveyor, and Azure DevOps setups for my fork.

successful builds from jameslamb/LightGBM fork

Azure DevOps

AppVeyor

Travis

This PR supports the progress on #629 and would give us more confidence in things like #2901 and #987

jameslamb · 2020-03-23T03:05:02Z

Azure is failing with this error, which I did see one other time during my testing:

This is a server for TeX packages. The one time I saw this while I was preparing this PR, it was fixed by just waiting a minute or two and rebuilding, so I'm going to close and reopen to trigger CI.

Everything on Travis is passing except our old friend, the check-docs task.

jameslamb · 2020-03-23T03:05:16Z

close-reopen to trigger CI

StrikerRUS · 2020-03-23T15:13:23Z

@jameslamb Awesome job, thanks!

I see Azure is failing again with the error you've mentioned. As we know that it occurs quite often, maybe we should retry download? For example: https://forums.hak5.org/topic/36911-powershell-retry-download-until-complete/?tab=comments#comment-268464

.ci/test_windows.ps1

jameslamb · 2020-03-24T15:40:46Z

Ok I addressed and in e54954a.

Once I got past those, I also had to add some changes for MSVC. I realized we don't really need LIBR_LIB_DIR in FindLibR.cmake...it was just an intermediary used to find R.dll. So I tried changing some things there to get MSVC working.

On my personal Azure DevOps, the R build took 32 minutes to complete (but did complete successfully) and the Appveyor build timed out after an hour. I'm investigating more on my fork.

I think the issue here is that this PR was working for just "compile lib_lightgbm and build the R package that calls it". But now that as of #2901 we're linking to R's shared library, there are additional things I need to fix.

jameslamb · 2020-03-24T23:15:16Z

I'm currently working in my fork on getting the builds to work again now that we're linking to R.dll. I am seeing this tough-to-understand behavior where, when using MSVC, the build just hangs for an hour and dies: https://ci.appveyor.com/project/jameslamb/lightgbm/builds/31695220/job/ia9pdstbxx0r3anj

I'm not seeing that behavior locally on my laptop running Windows 10, using MSVC bundled with Visual Studio 16 2019. Will let you all know when this is ready for review, it was probably premature to turn it from Draft to Ready :/

jameslamb · 2020-03-25T22:06:21Z

Ok here's where we stand:

Appveyor build with MinGW seems to be working, and ran in 14 minute for the R package which seems ok.
On AzureDevops, I'm still seeing an issue with downloading MiKTeX. The retry-logic is working but now it's hitting a different error where some step in miktexsetup.exe is trying to hit a server that's timing out. Did you ever face that in RGF @StrikerRUS ?

StrikerRUS · 2020-03-25T22:43:55Z

@jameslamb

Did you ever face that in RGF?

Never. I remember only one issue related to miktexsetup:

Update Miktex installation: https://miktex.org/announcement/miktex-portable-sfx-discontinued.
RGF-team/rgf#295

Maybe Azure blocks some IPs like Travis blocks apts outside of their whitelist?..
https://github.com/travis-ci/apt-package-safelist

jameslamb · 2020-03-25T22:46:18Z

@jameslamb

Did you ever face that in RGF?

Never. I remember only one issue related to miktexsetup:

Update Miktex installation: https://miktex.org/announcement/miktex-portable-sfx-discontinued.
RGF-team/rgf#295

Maybe Azure blocks some IPs like Travis blocks apts outside of their whitelist?..
https://github.com/travis-ci/apt-package-safelist

yeah I was thinking it might be something Azure-specific because I've only ever seen it there. That's a good idea!

I have an idea too, going to try it on my fork 😀

StrikerRUS · 2020-03-26T13:58:40Z

@jameslamb
If nothing helped, we can re-host installation package at our releases page like we do for AMD-APP-SDK

LightGBM/.ci/setup.sh

Line 31 in c870c57

    
           wget -q https://github.com/microsoft/LightGBM/releases/download/v2.0.12/AMD-APP-SDKInstaller-v3.0.130.136-GA-linux64.tar.bz2

jameslamb · 2020-03-31T03:13:03Z

build_r.R

@@ -5,6 +5,9 @@
 # Sys.setenv("CXX" = "/usr/local/bin/g++-8")


this is not strictly related to this PR and is being added in #2957 . It'll disappear from the diff when that gets merged

now that #2957 has been merged, I'll rebase to master before the next commit I push here

… all MINGW builds

jameslamb · 2020-04-19T17:43:18Z

@StrikerRUS thanks for the great review so far!

@Laurae2 @guolinke could one of you take a look at this? @StrikerRUS and I will keep talking through miscellaneous things in CI scripts, but I don't want to merge this until an R reviewer also approves.

StrikerRUS

@jameslamb Can we fix this warning?

Warning message:
In normalizePath(path.expand(path), winslash, mustWork) :
  path[1]="C:/projects/lightgbm/RLibrary/R/lib": The system cannot find the file specified

Please check one more round of review below.

@guolinke Printing out build logs with MinGW allowed to spot many compilation warnings. Can you please check them?

.ci/test_r_package_windows.ps1

.vsts-ci.yml

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

jameslamb · 2020-04-20T01:48:32Z

@jameslamb Can we fix this warning?
Warning message:
In normalizePath(path.expand(path), winslash, mustWork) :
  path[1]="C:/projects/lightgbm/RLibrary/R/lib": The system cannot find the file specified
Please check one more round of review below.

@guolinke Printing out build logs with MinGW allowed to spot many compilation warnings. Can you please check them?

On the point of compilation warnings...if we determine that they were not introduced by this PR (I really think they were not), let's please open a separate issue and not delay this PR any further.

The normalizePath() warning is fixed in #2963. The issue is that we use R.home('lib') but that function just constructs a directory. It doesn't guarantee that that directory exists. That code is used to generate LIBR_LIB_DIR, which is only necessary for MSVC builds. I completely removed it in #2963. It can safely be ignored in this PR and will be addressed in the followup MSVC PRs.

Co-Authored-By: Nikita Titov <[email protected]>

StrikerRUS · 2020-04-20T14:19:47Z

@jameslamb

On the point of compilation warnings...if we determine that they were not introduced by this PR (I really think they were not), let's please open a separate issue and not delay this PR any further.

I wanted to init the discussion here where we can see these logs live. Of course, they shouldn't prevent merging of this PR in any way.

The normalizePath() warning is fixed in #2963. The issue is that we use R.home('lib') but that function just constructs a directory.

Ah, OK!

StrikerRUS

@jameslamb Thanks a lot for all your hard work! I don't think that the rest minor comments should prevent from merging, but please check them.

.ci/test_r_package_windows.ps1

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

.ci/test_r_package_windows.ps1

Co-Authored-By: Nikita Titov <[email protected]>

jameslamb · 2020-04-25T16:49:30Z

Just merged in master to get all the new tests from #3002, #3004, #3005, #3006, #3007, and #3009.

@Laurae2 @guolinke once this builds, it is just waiting on a review from one of you whenever you have time. Thank you!

Laurae2 · 2020-04-26T20:29:58Z

R-package/src/install.libs.R

@@ -67,6 +67,7 @@ if (!use_precompile) {
  # Check if Windows installation (for gcc vs Visual Studio)
  if (WINDOWS) {
    if (use_mingw) {
+      print("Trying to build with MinGW")


Change in another PR print for message.

sure! Let's talk about it in a separate issue / PR. We have several other print() statements in install.libs.R and no uses of message() so I'm curious what makes message() preferable here.

Laurae2 · 2020-04-26T20:31:04Z

.ci/test_r_package_windows.ps1

+  } while(!$?);
+}
+
+$env:R_WINDOWS_VERSION = "3.6.3"


Do you think we should test with R 4.0 ?

yes I think we should have a separate issue for testing on R 4.0.0 across all platforms, now that it's officially released. Good call!

I just opened #3024 to capture that work (and added it to #2302 )

Laurae2 · 2020-04-26T20:43:18Z

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

+set(LIBR_HOME ${LIBR_HOME} CACHE PATH "R home directory")
+set(LIBR_EXECUTABLE ${LIBR_EXECUTABLE} CACHE PATH "R executable")
+set(LIBR_INCLUDE_DIRS ${LIBR_INCLUDE_DIRS} CACHE PATH "R include directory")
+set(LIBR_LIB_DIR ${LIBR_LIB_DIR} CACHE PATH "R shared libraries directory")


In another PR, maybe we should change the name and use LIBR_LIB_DIRS instead of LIBR_LIB_DIR (cmake Find result variables should be plural for include_dirs / lib_dirs, usually they are placeholders for include_dir / lib_dir if they are singular directories).

In this case it is a single directory. This isn't "all directories passed to the linker in -L flags", it is "what is the one directory where R.dll / R.so is likely to live".

Anyway, I'm removing LIBR_LIB_DIR completely in #2963 . It's only being used in MSVC installs, and only indirectly as a way to find LIBR_CORE_LIBRARY.

jameslamb · 2020-04-26T21:05:24Z

Thanks for the helpful reviews! I'm excited to merge this one 😀

StrikerRUS · 2020-05-14T19:00:56Z

@guolinke Now as we run R-package tests at Appveyor we'll face corresponding job failures due to CRAN/CTAN network availability issues quite often (for example, refer to the latest master failure). I wonder, is it possible to give maintainers privileges to re-run failed jobs as we have for Travis/Azure?

It seems that it is technically possible now: https://www.appveyor.com/docs/team-setup/.
But it may require to manually transfer the current account (via e-mail request).

jameslamb · 2020-05-16T19:32:41Z

^ related to #2936 (comment), without that we can find ourselves in this situation where one long-running AppVeyor build is blocking all others from starting:

Notice that the build at the bottom has been running for 45 minutes. It is stuck trying to download something from a CTAN mirror. Ideally, I'd like to be able to kill it in the AppVeyor UI. But since I don't have permissions right now all other builds have to wait.

StrikerRUS · 2020-05-31T21:39:43Z

Gently ping @guolinke for #2936 (comment)

guolinke · 2020-06-20T02:34:02Z

@StrikerRUS @jameslamb
I have sent invitation to you.

jameslamb · 2020-06-20T23:50:15Z

@guolinke thanks! Although now that we have #3168 , as soon as that is merged AppVeyor should no longer be a problem 😂

That PR and all others are blocked until you can help us address #3000 (comment)

StrikerRUS · 2020-06-21T01:57:13Z

@guolinke Thanks a lot! Now I can re-run failed jobs.

github-actions · 2023-08-24T13:06:02Z

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 Laurae2, guolinke and StrikerRUS March 23, 2020 02:38

jameslamb added the maintenance label Mar 23, 2020

jameslamb mentioned this pull request Mar 23, 2020

R unit tests are not run in CI #2335

Closed

jameslamb closed this Mar 23, 2020

jameslamb reopened this Mar 23, 2020

jameslamb mentioned this pull request Mar 23, 2020

[ci] changed strategy for installing R on AppVeyor RGF-team/rgf#321

Merged

StrikerRUS reviewed Mar 23, 2020

View reviewed changes

.ci/test_windows.ps1 Outdated Show resolved Hide resolved

This was referenced Mar 24, 2020

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

Merged

[R-Package] CRAN issues #629

Closed

jameslamb added the in progress label Mar 24, 2020

jameslamb force-pushed the ci/r-windows-ci branch from a479fd1 to e54954a Compare March 24, 2020 15:35

jameslamb force-pushed the ci/r-windows-ci branch from 3678829 to 248d36d Compare March 25, 2020 19:39

jameslamb mentioned this pull request Mar 29, 2020

Path smoothing #2950

Merged

jameslamb force-pushed the ci/r-windows-ci branch from 248d36d to c9ba4d1 Compare March 29, 2020 22:50

jameslamb mentioned this pull request Mar 30, 2020

[R-package] Create portable configuration with 'configure' scripts #2960

Closed

guolinke mentioned this pull request Mar 30, 2020

Feature Requests & Voting Hub #2302

Open

jameslamb force-pushed the ci/r-windows-ci branch 2 times, most recently from f911585 to 45adca9 Compare March 31, 2020 03:01

jameslamb commented Mar 31, 2020

View reviewed changes

jameslamb added 2 commits April 19, 2020 12:31

added CXX, CC for Windows builds and changed back to building docs on…

6ca2be8

… all MINGW builds

stored LIBR_CORE_LIBRARY hints in one variable

cdf1df8

StrikerRUS reviewed Apr 19, 2020

View reviewed changes

jameslamb and others added 3 commits April 20, 2020 02:49

Apply suggestions from code review

9c59f33

Co-Authored-By: Nikita Titov <[email protected]>

changes from code review

747378e

increased parallel builds for Azure CI

c0b87d6

jameslamb added the awaiting review label Apr 20, 2020

StrikerRUS approved these changes Apr 20, 2020

View reviewed changes

jameslamb and others added 2 commits April 21, 2020 04:24

Apply suggestions from code review

6820fc6

Co-Authored-By: Nikita Titov <[email protected]>

Merge branch 'master' into ci/r-windows-ci

7075b80

Laurae2 approved these changes Apr 26, 2020

View reviewed changes

jameslamb mentioned this pull request Apr 26, 2020

[R-package] [ci] Add tests on R 4.0 compatibility #3024

Closed

jameslamb merged commit 2c18a0f into microsoft:master Apr 26, 2020

jameslamb added a commit to jameslamb/LightGBM that referenced this pull request Apr 26, 2020

merged changes from microsoft#2936

25cb78a

jameslamb mentioned this pull request Apr 26, 2020

[R-package] added tests on LGBM_BoosterResetTrainingData_R #3020

Merged

jameslamb deleted the ci/r-windows-ci branch April 26, 2020 23:09

StrikerRUS removed the awaiting review label Apr 27, 2020

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] [ci] Add Windows CI for R package (fixes #2335) #2936

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

jameslamb commented Mar 23, 2020

jameslamb commented Mar 23, 2020

jameslamb commented Mar 23, 2020

StrikerRUS commented Mar 23, 2020

jameslamb commented Mar 24, 2020

jameslamb commented Mar 24, 2020

jameslamb commented Mar 25, 2020 •

edited

Loading

StrikerRUS commented Mar 25, 2020

jameslamb commented Mar 25, 2020

StrikerRUS commented Mar 26, 2020

jameslamb Mar 31, 2020

jameslamb Mar 31, 2020

jameslamb commented Apr 19, 2020

StrikerRUS left a comment

jameslamb commented Apr 20, 2020

StrikerRUS commented Apr 20, 2020

StrikerRUS left a comment

jameslamb commented Apr 25, 2020

Laurae2 Apr 26, 2020

jameslamb Apr 26, 2020

Laurae2 Apr 26, 2020

jameslamb Apr 26, 2020

Laurae2 Apr 26, 2020

jameslamb Apr 26, 2020

jameslamb commented Apr 26, 2020

StrikerRUS commented May 14, 2020

jameslamb commented May 16, 2020

StrikerRUS commented May 31, 2020

guolinke commented Jun 20, 2020

jameslamb commented Jun 20, 2020

StrikerRUS commented Jun 21, 2020

github-actions bot commented Aug 24, 2023

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

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

Conversation

jameslamb commented Mar 23, 2020

jameslamb commented Mar 23, 2020

jameslamb commented Mar 23, 2020

StrikerRUS commented Mar 23, 2020

jameslamb commented Mar 24, 2020

jameslamb commented Mar 24, 2020

jameslamb commented Mar 25, 2020 • edited Loading

StrikerRUS commented Mar 25, 2020

jameslamb commented Mar 25, 2020

StrikerRUS commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb commented Apr 19, 2020

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented Apr 20, 2020

StrikerRUS commented Apr 20, 2020

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented Apr 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb commented Apr 26, 2020

StrikerRUS commented May 14, 2020

jameslamb commented May 16, 2020

StrikerRUS commented May 31, 2020

guolinke commented Jun 20, 2020

jameslamb commented Jun 20, 2020

StrikerRUS commented Jun 21, 2020

github-actions bot commented Aug 24, 2023

jameslamb commented Mar 25, 2020 •

edited

Loading