-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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] [ci] Add Windows CI for R package (fixes #2335) #2936
Changes from all commits
19ed854
16476da
f7573f0
311d3d8
c422b15
b2fce60
b973b7d
a8873f6
952ca55
b4cf398
cc6d02b
adfb832
92b0032
8d77941
b7e2abe
12fb638
1091504
96dcc18
97d9913
6ed498d
b275d22
f701325
618086a
90f91e5
2f4979a
3b07c7b
2d17cae
2e9fe09
855cdc9
6d8878b
c854fe3
86f4eec
8b12858
dc50ad5
065df95
432df25
6e73047
fec3de7
9b90d84
c305e9d
c70e352
517e519
56bf28a
f439ba2
2923111
65915a1
f18399d
1de8f6c
a912213
290e5b7
2565df7
cd89065
fbf494c
244a14a
42609cd
2804d45
af3c004
1c498d0
9e6aa49
76d264d
01359ab
d100027
912a2fd
d76b551
7cdfc62
1a2053f
bdf10e0
88a7a09
ae813f2
1b2363a
dd52dbf
7f9eb7c
374ba31
30d8af1
e3b9ac8
419e39b
bfebf1d
3e2dcfd
8ba021a
51e4d2d
5d74019
d7c160b
a8582db
8264f4a
d843795
10bd18d
6ca2be8
cdf1df8
9c59f33
747378e
c0b87d6
6820fc6
7075b80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
# Download a file and retry upon failure. This looks like | ||
# an infinite loop but CI-level timeouts will kill it | ||
function Download-File-With-Retries { | ||
param( | ||
[string]$url, | ||
[string]$destfile | ||
) | ||
do { | ||
Write-Output "Downloading ${url}" | ||
sleep 5; | ||
(New-Object System.Net.WebClient).DownloadFile($url, $destfile) | ||
} while(!$?); | ||
} | ||
|
||
$env:R_WINDOWS_VERSION = "3.6.3" | ||
$env:R_LIB_PATH = "$env:BUILD_SOURCESDIRECTORY/RLibrary" -replace '[\\]', '/' | ||
StrikerRUS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
$env:PATH = "$env:R_LIB_PATH/Rtools/bin;" + "$env:R_LIB_PATH/R/bin/x64;" + "$env:R_LIB_PATH/miktex/texmfs/install/miktex/bin/x64;" + $env:PATH | ||
$env:CRAN_MIRROR = "https://cloud.r-project.org/" | ||
$env:CTAN_MIRROR = "https://ctan.math.illinois.edu/systems/win32/miktex/tm/packages/" | ||
|
||
if ($env:COMPILER -eq "MINGW") { | ||
$env:CXX = "$env:R_LIB_PATH/Rtools/mingw_64/bin/g++.exe" | ||
$env:CC = "$env:R_LIB_PATH/Rtools/mingw_64/bin/gcc.exe" | ||
} | ||
|
||
cd $env:BUILD_SOURCESDIRECTORY | ||
tzutil /s "GMT Standard Time" | ||
[Void][System.IO.Directory]::CreateDirectory($env:R_LIB_PATH) | ||
|
||
if ($env:COMPILER -eq "MINGW") { | ||
Write-Output "Telling R to use MinGW" | ||
$install_libs = "$env:BUILD_SOURCESDIRECTORY/R-package/src/install.libs.R" | ||
((Get-Content -path $install_libs -Raw) -replace 'use_mingw <- FALSE','use_mingw <- TRUE') | Set-Content -Path $install_libs | ||
} | ||
|
||
# download R and RTools | ||
Write-Output "Downloading R and Rtools" | ||
Download-File-With-Retries -url "https://cloud.r-project.org/bin/windows/base/old/$env:R_WINDOWS_VERSION/R-$env:R_WINDOWS_VERSION-win.exe" -destfile "R-win.exe" | ||
Download-File-With-Retries -url "https://cloud.r-project.org/bin/windows/Rtools/Rtools35.exe" -destfile "Rtools.exe" | ||
|
||
# Install R | ||
Write-Output "Installing R" | ||
Start-Process -FilePath R-win.exe -NoNewWindow -Wait -ArgumentList "/VERYSILENT /DIR=$env:R_LIB_PATH/R /COMPONENTS=main,x64" ; Check-Output $? | ||
Write-Output "Done installing R" | ||
|
||
Write-Output "Installing Rtools" | ||
Start-Process -FilePath Rtools.exe -NoNewWindow -Wait -ArgumentList "/VERYSILENT /DIR=$env:R_LIB_PATH/Rtools" ; Check-Output $? | ||
Write-Output "Done installing Rtools" | ||
|
||
# MiKTeX and pandoc can be skipped on non-MINGW builds, since we don't | ||
# build the package documentation for those | ||
if ($env:COMPILER -eq "MINGW") { | ||
Write-Output "Downloading MiKTeX" | ||
Download-File-With-Retries -url "https://miktex.org/download/win/miktexsetup-x64.zip" -destfile "miktexsetup-x64.zip" | ||
Add-Type -AssemblyName System.IO.Compression.FileSystem | ||
[System.IO.Compression.ZipFile]::ExtractToDirectory("miktexsetup-x64.zip", "miktex") | ||
Write-Output "Setting up MiKTeX" | ||
.\miktex\miktexsetup.exe --remote-package-repository="$env:CTAN_MIRROR" --local-package-repository=./miktex/download --package-set=essential --quiet download ; Check-Output $? | ||
Write-Output "Installing MiKTeX" | ||
.\miktex\download\miktexsetup.exe --remote-package-repository="$env:CTAN_MIRROR" --portable="$env:R_LIB_PATH/miktex" --quiet install ; Check-Output $? | ||
Write-Output "Done installing MiKTeX" | ||
|
||
initexmf --set-config-value [MPM]AutoInstall=1 | ||
conda install -q -y --no-deps pandoc | ||
} | ||
|
||
Add-Content .Renviron "R_LIBS=$env:R_LIB_PATH" | ||
|
||
Write-Output "Installing dependencies" | ||
$packages = "c('data.table', 'jsonlite', 'Matrix', 'R6', 'testthat'), dependencies = c('Imports', 'Depends', 'LinkingTo')" | ||
Rscript --vanilla -e "options(install.packages.check.source = 'no'); install.packages($packages, repos = '$env:CRAN_MIRROR', type = 'binary', lib = '$env:R_LIB_PATH')" ; Check-Output $? | ||
|
||
Write-Output "Building R package" | ||
Rscript build_r.R --skip-install ; Check-Output $? | ||
|
||
$PKG_FILE_NAME = Get-Item *.tar.gz | ||
$LOG_FILE_NAME = "lightgbm.Rcheck/00check.log" | ||
|
||
$env:_R_CHECK_FORCE_SUGGESTS_ = 0 | ||
if ($env:COMPILER -ne "MINGW") { | ||
Write-Output "Running R CMD check without checking documentation" | ||
R.exe CMD check --no-multiarch --no-examples --no-manual --ignore-vignettes ${PKG_FILE_NAME} ; $check_succeeded = $? | ||
} else { | ||
Write-Output "Running R CMD check as CRAN" | ||
R.exe CMD check --no-multiarch --as-cran ${PKG_FILE_NAME} ; $check_succeeded = $? | ||
} | ||
|
||
Write-Output "R CMD check build logs:" | ||
Get-Content -Path $env:BUILD_SOURCESDIRECTORY\lightgbm.Rcheck\00install.out | ||
jameslamb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Check-Output $check_succeeded | ||
|
||
Write-Output "Looking for issues with R CMD check results" | ||
if (Get-Content "$LOG_FILE_NAME" | Select-String -Pattern "WARNING" -Quiet) { | ||
echo "WARNINGS have been found by R CMD check!" | ||
Check-Output $False | ||
} | ||
StrikerRUS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
$note_str = Get-Content "${LOG_FILE_NAME}" | Select-String -Pattern ' NOTE' | Out-String ; Check-Output $? | ||
$relevant_line = $note_str -match '.*Status: (\d+) NOTE.*' | ||
$NUM_CHECK_NOTES = $matches[1] | ||
$ALLOWED_CHECK_NOTES = 3 | ||
if ([int]$NUM_CHECK_NOTES -gt $ALLOWED_CHECK_NOTES) { | ||
Write-Output "Found ${NUM_CHECK_NOTES} NOTEs from R CMD check. Only ${ALLOWED_CHECK_NOTES} are allowed" | ||
Check-Output $False | ||
} | ||
|
||
Write-Output "No issues were found checking the R package" |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -174,17 +174,31 @@ execute_process( | |||
OUTPUT_VARIABLE LIBR_LIB_DIR | ||||
) | ||||
|
||||
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") | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In another PR, maybe we should change the name and use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case it is a single directory. This isn't "all directories passed to the linker in Anyway, I'm removing |
||||
|
||||
# where is R.so / R.dll / libR.so likely to be found? | ||||
set(LIBR_PATH_HINTS "${CMAKE_CURRENT_BINARY_DIR}" "${LIBR_LIB_DIR}" "${LIBR_HOME}/bin/${R_ARCH}" "${LIBR_HOME}/bin" "${LIBR_LIBRARIES}") | ||||
|
||||
# look for the core R library | ||||
find_library( | ||||
LIBR_CORE_LIBRARY | ||||
NAMES R | ||||
HINTS "${CMAKE_CURRENT_BINARY_DIR}" "${LIBR_LIB_DIR}" "${LIBR_HOME}/bin" "${LIBR_LIBRARIES}" | ||||
NAMES R R.dll | ||||
HINTS ${LIBR_PATH_HINTS} | ||||
) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @StrikerRUS I'm starting a thread here for this discussion on how to find There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be working! In https://ci.appveyor.com/project/guolinke/lightgbm/builds/32185340/job/5tcrah61l6hlv0ab I see
Don't worry about that "cannot find the file specified" up above those messages, I'm fixing it in #2963 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Struggling to get this working on Azure. I wrote up this comment bit-by-bit as I tried things, which is how it got so long. the problem:
When I say "directly", I mean going back to what I originally had
Let me show you:
From the
So then I thought "ok, the paths are the same...maybe the CMake versions
We did hear in #2995 (comment) that our setup wasn't working with I found an important-looking line in https://cmake.org/cmake/help/v3.17/release/3.17.html#id17
I'm confused though, it seems like passing
I'm going to try something like this:
I'd rather do that than replace the origin There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried that strategy and it seems like it worked!
Added these changes in d843795:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That comment was about what I thought before CTAN pinning allowed us to move forward. Sorry for confusing you. The current plan may be the following.
Please let me know WDYT about that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, for sure! Just like in Line 4 in 0d3e204
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with this plan --> #2936 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok these changes worked: jameslamb@c145b7d
Added back to this PR in: 6ca2be8 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||
|
||||
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") | ||||
# starting from CMake 3.17, find_library() will not find .dll files by default | ||||
# https://cmake.org/cmake/help/v3.17/release/3.17.html#other-changes | ||||
if (WIN32 AND NOT LIBR_CORE_LIBRARY) | ||||
StrikerRUS marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
find_file( | ||||
LIBR_CORE_LIBRARY | ||||
NAME R.dll | ||||
HINTS ${LIBR_PATH_HINTS} | ||||
) | ||||
endif() | ||||
|
||||
set(LIBR_CORE_LIBRARY ${LIBR_CORE_LIBRARY} CACHE PATH "R core shared library") | ||||
|
||||
if(WIN32 AND MSVC) | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change in another PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure! Let's talk about it in a separate issue / PR. We have several other |
||
cmake_cmd <- paste0(cmake_cmd, " -G \"MinGW Makefiles\" ") | ||
build_cmd <- "mingw32-make.exe _lightgbm" | ||
system(paste0(cmake_cmd, " ..")) # Must build twice for Windows due sh.exe in Rtools | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should test with R 4.0 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 )