remove support for Solaris (fixes #5216)#5226
Conversation
guolinke
left a comment
There was a problem hiding this comment.
I like the PR that deletes codes 😆
StrikerRUS
left a comment
There was a problem hiding this comment.
Excellent, thanks a lot! Really like to see these changes!
| testthat::skip_if( | ||
| ON_SOLARIS || ON_32_BIT_WINDOWS | ||
| ON_32_BIT_WINDOWS | ||
| , message = "Skipping on Solaris and 32-bit Windows" |
There was a problem hiding this comment.
| , message = "Skipping on Solaris and 32-bit Windows" | |
| , message = "Skipping on 32-bit Windows" |
|
Thanks for merging this @guolinke , but I didn't get a chance to address the suggestion in #5226 (comment). I'll open a follow-up PR to address that. In the future, can you please avoid merging before all suggestions have been addressed? |
Sorry, I just saw 2 approves, will check the details next time. |
|
no problem! @StrikerRUS, @jmoralez and I sometimes approve but with minor suggestions, meaning "I approve assuming you fix this minor suggestion", to avoid another round of reviews. |
Actually, I tried the GitHub App on my mobile phone to review issues and codes yesterday, and didn't realize it hides the approved comment by default 🤣 . |
|
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. |
Fixes #5216 .
Related to #3513.
Per the discussion in #5217 (comment), this PR proposes removing support for the Solaris operating system in LightGBM.
That support was added only to get
{lightgbm}(the R package) onto CRAN, so that it would be easier for R users to install.There is strong evidence (provided in that comment) that CRAN no longer requires packages to be installable on Solaris.
I'm proposing removing this support for the following reasons: