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

[ci] add CMake + R 3.6 test back (fixes #3469) #4053

Merged
merged 6 commits into from
Mar 10, 2021
Merged

[ci] add CMake + R 3.6 test back (fixes #3469) #4053

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
25c57d3
[ci] add CMake + R 3.6 test back (fixes #3469)
jameslamb Mar 8, 2021
558784a
Apply suggestions from code review
jameslamb Mar 8, 2021
6f0b876
Update .ci/test_r_package_windows.ps1
jameslamb Mar 8, 2021
0d67e7e
-Wait and remove rtools40
jameslamb Mar 9, 2021
aff80c9
empty commit
jameslamb Mar 9, 2021
03d9eb2
Merge branch 'master' into fix/r3.6-ci
jameslamb Mar 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .ci/test_r_package_windows.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,24 @@ function Run-R-Code-Redirect-Stderr {
Rscript --vanilla -e $decorated_code
}

# Remove all items matching some pattern from PATH environment variable
function Remove-From-Path {
param(
[string]$item_to_remove
)
$env:path = ($env:path.Split(';') | Where-Object { $_ -notmatch "$item_to_remove" }) -join ';'
jameslamb marked this conversation as resolved.
Show resolved Hide resolved
jameslamb marked this conversation as resolved.
Show resolved Hide resolved
}

# remove some details that exist in the GitHub Actions images which might
# cause conflicts with R and other components installed by this script
$env:RTOOLS40_HOME = ""
StrikerRUS marked this conversation as resolved.
Show resolved Hide resolved
Remove-From-Path ".*chocolatey.*"
Remove-From-Path ".*Chocolatey.*"
Remove-From-Path ".*Git.*mingw64.*"
Remove-From-Path ".*msys64.*"
Remove-From-Path ".*rtools40.*"
Copy link
Collaborator

@StrikerRUS StrikerRUS Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

WDYT about to remove this particular directory physically?
Looks like there could be conflicts during our own installation (but for R 4.x job)

LightGBM/.ci/test_r_package_windows.ps1

Line 45 in 3a5e3c0

$RTOOLS_INSTALL_PATH = "C:\rtools40"

Copy link
Collaborator Author

@jameslamb jameslamb Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I originally had something like Remove-Item "C:\rtools40" -Recurse", but it was failing with an error saying something like "insufficient permission to remove this, must be an administrator or this may be a system file" (don't remember the exact error message).

Although, I was just looking at your work in RGF...maybe we could borrow your solution from there to get past that error? 😀

https://github.com/RGF-team/rgf/blob/ab328a01d3af809c0c1fd4122d8b70b9fb2e9757/.ci/r_tests_windows.ps1#L20

Remove-Item C:\rtools40 -Force -Recurse -ErrorAction Ignore

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so! This command takes some time (several seconds) but I think it won't be excess and can protect us from some other problems.

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, I don't know why, but I had to manually install qpdf for Windows R jobs (I chose pacman package manager because it's bundled with Rtools and one-line solution) to pass --as-cran checks for vignettes.
Maybe this info will help you to save some time on other projects or even here in LightGBM.
https://github.com/RGF-team/rgf/blob/ab328a01d3af809c0c1fd4122d8b70b9fb2e9757/.ci/r_tests_windows.ps1#L42

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks! Maybe we'll have to worry about qpdf on #3946 , actually, so that is timely advicce

Remove-From-Path ".*Strawberry.*"
Comment on lines +42 to +47
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these paths all had some msys2 or mingw components in them.

I dumped the full PATH below so you can see what's there, including why I needed both chocolatey and Chocolatey 😂

$env:PATH.split(";")
full contents of PATH before removals 
C:\Program Files\PowerShell\7
C:\Users\runneradmin\.dotnet\tools
C:\Program Files\MongoDB\Server\4.4\bin
C:\aliyun-cli
C:\vcpkg
C:\cf-cli
C:\Program Files (x86)\NSIS\
C:\Program Files\Mercurial\
C:\hostedtoolcache\windows\stack\2.5.1\x64
C:\ProgramData\chocolatey\lib\ghc.8.10.3\tools\ghc-8.10.3\bin
C:\Program Files\dotnet
C:\mysql-5.7.21-winx64\bin
C:\Program Files\R\R-4.0.4\bin\x64
C:\SeleniumWebDrivers\GeckoDriver
C:\Program Files (x86)\sbt\bin
C:\Rust\.cargo\bin
C:\Program Files (x86)\GitHub CLI
C:\Program Files\Git\bin
C:\Program Files (x86)\pipx_bin
C:\hostedtoolcache\windows\go\1.15.8\x64\bin
C:\hostedtoolcache\windows\Python\3.7.9\x64\Scripts
C:\hostedtoolcache\windows\Python\3.7.9\x64
C:\hostedtoolcache\windows\Ruby\2.5.8\x64\bin
C:\Program Files\Java\jdk8u282-b08\bin
C:\npm\prefix
C:\Program Files\Microsoft SDKs\Azure\Azure Dev Spaces CLI
C:\Program Files\Microsoft SDKs\Azure\Azure Dev Spaces CLI\
C:\Program Files (x86)\Microsoft SDKs\Azure\CLI2\wbin
C:\ProgramData\kind
C:\windows\system32
C:\windows
C:\windows\System32\Wbem
C:\windows\System32\WindowsPowerShell\v1.0\
C:\windows\System32\OpenSSH\
C:\ProgramData\Chocolatey\bin
C:\Program Files\Microsoft\Web Platform Installer\
C:\Program Files\Docker
C:\Program Files\PowerShell\7\
C:\Program Files\dotnet\
C:\Program Files\Microsoft SQL Server\130\Tools\Binn\
C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\170\Tools\Binn\
C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit\
C:\Program Files (x86)\Microsoft SQL Server\110\DTS\Binn\
C:\Program Files (x86)\Microsoft SQL Server\120\DTS\Binn\
C:\Program Files (x86)\Microsoft SQL Server\130\DTS\Binn\
C:\Program Files (x86)\Microsoft SQL Server\140\DTS\Binn\
C:\Program Files (x86)\Microsoft SQL Server\150\DTS\Binn\
C:\Program Files\nodejs\
C:\ProgramData\chocolatey\lib\pulumi\tools\Pulumi\bin
C:\ProgramData\chocolatey\lib\maven\apache-maven-3.6.3\bin
C:\Program Files\Microsoft Service Fabric\bin\Fabric\Fabric.Code
C:\Program Files\Microsoft SDKs\Service Fabric\Tools\ServiceFabricLocalClusterManager
C:\Program Files\OpenSSL\bin
C:\Strawberry\c\bin
C:\Strawberry\perl\site\bin
C:\Strawberry\perl\bin
C:\Program Files\Git\cmd
C:\Program Files\Git\mingw64\bin
C:\Program Files\Git\usr\bin
c:\tools\php
C:\Program Files (x86)\sbt\bin
C:\Program Files\TortoiseSVN\bin
C:\SeleniumWebDrivers\ChromeDriver\
C:\SeleniumWebDrivers\EdgeDriver\
C:\Program Files\CMake\bin
C:\Program Files\Amazon\AWSCLIV2\
C:\Program Files\Amazon\SessionManagerPlugin\bin\
C:\Program Files\Amazon\AWSSAMCLI\bin\
C:\Program Files (x86)\Google\Cloud SDK\google-cloud-sdk\bin
C:\Program Files (x86)\Microsoft BizTalk Server\

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 had to look this up, but Strawberry is a perl environment for Windows: https://strawberryperl.com/. It's Program Files contain some mingw tools used to compile C programs


# Get details needed for installing R components
#
# NOTES:
Expand Down Expand Up @@ -95,6 +113,10 @@ Write-Output "Installing Rtools"
./Rtools.exe /VERYSILENT /SUPPRESSMSGBOXES /DIR=$RTOOLS_INSTALL_PATH ; Check-Output $?
Write-Output "Done installing Rtools"

# wait for all Rtools files to be written
Write-Output "Sleeping to allow Rtools install to finish"
Start-Sleep -Seconds 60
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can use Start-Process -FilePath Rtools.exe -NoNewWindow -Wait -ArgumentList ... above to not wait manually?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops! Seems I missed that:

I hit "refresh" in Windows Explorer several times and did not see mingw_64 show up until after 20 seconds or so had passed. So I think the install returned control to powershell before all the files were actually written, somehow 😱 .

Indeed, very interesting observation! 👍

Given that, please ignore my comment above about -Wait argument (it will not help). Instead, I believe we should do something like

while True:
    sleep(1)
    if exists("$RTOOLS_INSTALL_PATH/mingw_64"):
        break

instead of hardcoded sleep(60).

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 didn't know about -Wait! I don't write Powershell code anywhere else except this one script in this project, so I'm not very knowledgeable about it. Yes that seems a lot better than a sleep! Let me try that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or you think that writing all files into mingw_64 takes some significant time and this is not just about delay in mingw_64 folder creation?

Copy link
Collaborator Author

@jameslamb jameslamb Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ha we posted our responses at about the same time, I didn't see your second comment.

I don't think this while loop approach is exactly correct a reliable fix. The directory mingw_64 could exist without all of the other files and folders in it being populated yet. This is what I meant by this explanation in the PR description

know fixing a problem by just introducing a sleep isn't ideal, but I think it would be difficult to figure out the full list of files needed by ld.exe when compiling LightGBM

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I see... Could then please try -Wait workaround in RDP before merging this? I'm not an expert in PowerShell too, so maybe some magic will happen 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure! I won't be able to test it until later tonight though, maybe 6-8 hours from now

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem!
Maybe this is indeed a "recommended" official way of installation:
https://github.com/r-windows/rtools-base/blob/e60c36de70de7dedce89577c11d2fbe6008b2df4/scripts.ps1#L39

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh nice, I forgot about r-windows/r-tools. I vaguely remember reading a blog post about it once. If that's how Jeroen does it, it's definitely a good sign.

Happy to say that switching to a -Wait did work! I could see that the Rtools install command didn't return control to powershell until both ming_32 and mingw_64 directories were created.

image

image


Write-Output "Installing dependencies"
$packages = "c('data.table', 'jsonlite', 'Matrix', 'processx', 'R6', 'testthat'), dependencies = c('Imports', 'Depends', 'LinkingTo')"
Run-R-Code-Redirect-Stderr "options(install.packages.check.source = 'no'); install.packages($packages, repos = '$env:CRAN_MIRROR', type = 'binary', lib = '$env:R_LIB_PATH')" ; Check-Output $?
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/r_package.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ jobs:
compiler: clang
r_version: 4.0
build_type: cmake
- os: windows-latest
task: r-package
compiler: MINGW
toolchain: MINGW
r_version: 3.6
build_type: cmake
- os: windows-latest
task: r-package
compiler: MINGW
Expand Down