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.

Sign up for GitHub

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

Jump to bottom

[R-package] suppress warning about empty 'compute' directory and fix Rtools installation #3277

Merged
merged 17 commits into from Aug 9, 2020
Merged

[R-package] suppress warning about empty 'compute' directory and fix Rtools installation #3277

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
cc1285b
[R-package] suppress warning about empty 'compute' directory
jameslamb Aug 6, 2020
8c6670e
Merge branch 'master' into fix/build-r-warning
jameslamb Aug 6, 2020
4a641c6
get more logs from Rtools install
jameslamb Aug 6, 2020
2bc2b33
Merge branch 'fix/build-r-warning' of github.com:jameslamb/LightGBM i…
jameslamb Aug 6, 2020
c7ac7c6
empty commit
jameslamb Aug 8, 2020
d5196a3
add a timeout
jameslamb Aug 8, 2020
a69526d
trying Invoke-WebRequest instead
jameslamb Aug 8, 2020
7c5cd6d
Merge branch 'master' into fix/build-r-warning
jameslamb Aug 8, 2020
c579bd0
switch to CRAN link
jameslamb Aug 8, 2020
f817fb0
do not use Start-Process
jameslamb Aug 8, 2020
ad892b7
relative paths
jameslamb Aug 8, 2020
7d02be6
trying suppressmessageboxes
jameslamb Aug 9, 2020
92d74d3
Merge branch 'master' into fix/build-r-warning
jameslamb Aug 9, 2020
8308596
switch strategy for running Rtools
jameslamb Aug 9, 2020
ac6e543
Update requirements_rtd.txt
StrikerRUS Aug 9, 2020
75ccb1e
Update requirements_base.txt
StrikerRUS Aug 9, 2020
3c688cd
Merge pull request #34 from StrikerRUS/StrikerRUS-patch-1
jameslamb Aug 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions .ci/test_r_package_windows.ps1
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ function Download-File-With-Retries {
do {
Write-Output "Downloading ${url}"
sleep 5;
(New-Object System.Net.WebClient).DownloadFile($url, $destfile)
Invoke-WebRequest -Uri $url -OutFile $destfile
} while(!$?);
}

Expand Down Expand Up @@ -102,16 +102,18 @@ if ($env:R_BUILD_TYPE -eq "cmake") {

# 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/$env:RTOOLS_EXE_FILE" -destfile "Rtools.exe"
Download-File-With-Retries -url "https://cran.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://cran.r-project.org/bin/windows/Rtools/$env:RTOOLS_EXE_FILE" -destfile "Rtools.exe"

ls
jameslamb marked this conversation as resolved.
Show resolved Hide resolved

# 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=$RTOOLS_INSTALL_PATH" ; Check-Output $?
./Rtools.exe /VERYSILENT /SUPPRESSMSGBOXES /DIR=$RTOOLS_INSTALL_PATH
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's try to use previous scheme with Start-Process and see whether only SUPPRESSMSGBOXES can help.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At least we should place exit code check back.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how is Start-Process better than ./something.exe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it doesn't matter, I guess. Switching back to Start-Process and adding in SUPPRESSMSGBOXES, the R 4.0 Windows builds are still running after 30 minutes: https://github.com/microsoft/LightGBM/pull/3277/checks?check_run_id=962759622

I'm going to switch to ./Rtools.exe and try adding the exit code check back

Copy link
Collaborator

Choose a reason for hiding this comment

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

how is Start-Process better than ./something.exe?

Here are some thoughts about it: https://social.technet.microsoft.com/wiki/contents/articles/7703.powershell-running-executables.aspx. TL;DR: it gives you full control.

Write-Output "Done installing Rtools"

Write-Output "Installing dependencies"
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/main.yml
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ jobs:
test:
name: ${{ matrix.task }} (${{ matrix.os }}, ${{ matrix.compiler }}, R ${{ matrix.r_version }}, ${{ matrix.build_type }})
runs-on: ${{ matrix.os }}
timeout-minutes: 60
Comment on lines 14 to +15
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need it in the master.

Suggested change
runs-on: ${{ matrix.os }}
timeout-minutes: 60
runs-on: ${{ matrix.os }}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why not? What is the problem with including it?

The default is 6 hours, which is way longer than these jobs need

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are dozens of config params. Including them all will make our file less readable. I believe we should keep configs as much clean and short as it is possible. Normally, our jobs don't need neither 1 hour nor 6 hours. So I don't see any differences why we should change this param from 360 to 60. Pending status more than 20 mins already indicates that something is wrong with our builds (for the current number of tests).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I respectully disagree but it is not worth holding up this blocking PR. GitHub won't allow me to commit this suggestion in the browser, but I'll commit it in a few hours when I am home.

strategy:
fail-fast: false
matrix:
Expand Down
23 changes: 16 additions & 7 deletions build_r.R
View file
Open in desktop
Expand Up @@ -10,6 +10,11 @@ INSTALL_AFTER_BUILD <- !("--skip-install" %in% args)
TEMP_R_DIR <- file.path(getwd(), "lightgbm_r")
TEMP_SOURCE_DIR <- file.path(TEMP_R_DIR, "src")

install_libs_content <- readLines(
file.path("R-package", "src", "install.libs.R")
)
USING_GPU <- any(grepl("use_gpu.*TRUE", install_libs_content))

# R returns FALSE (not a non-zero exit code) if a file copy operation
# breaks. Let's fix that
.handle_result <- function(res) {
Expand Down Expand Up @@ -100,13 +105,17 @@ result <- file.copy(
)
.handle_result(result)

result <- file.copy(
from = "compute/"
, to = sprintf("%s/", TEMP_SOURCE_DIR)
, recursive = TRUE
, overwrite = TRUE
)
.handle_result(result)
# compute/ is a submodule with boost, only needed if
# building the R package with GPU support
if (USING_GPU) {
result <- file.copy(
from = "compute/"
, to = sprintf("%s/", TEMP_SOURCE_DIR)
, recursive = TRUE
, overwrite = TRUE
)
.handle_result(result)
}

result <- file.copy(
from = "CMakeLists.txt"
Expand Down