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.

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 debian-clang-devel CI job for the R package #4164

Merged
merged 15 commits into from
Apr 12, 2021

Conversation

jameslamb
Copy link
Collaborator

The R package failed on CRAN check on its most recent submission (documented in #4105).

This PR proposes a CI job that attempts to replicate that environment, so that similar problems can be caught before submitting to CRAN in the future.

Following the suggestion in #4105 (comment), this tries to replicate CRAN's r-devel-linux-x86_64-debian-clang test, using the same container image that R Hub uses for that purpose.

@jameslamb
Copy link
Collaborator Author

I was able to replicate the error from CRAN locally with this approach.

image

Pushing now to see if it's replicated on GitHub Actions as well. If it is, then I'll add another commit to try to actually patch the test so it will pass.

@jameslamb
Copy link
Collaborator Author

Seems like this check is working! CUDA jobs are failing for probably-unrelated reasons (#4158 (comment)), but I can see that that new debian-clang-devel job failed with exactly the same error as seen on CRAN.

https://github.com/microsoft/LightGBM/pull/4164/checks?check_run_id=2275252015

@jameslamb
Copy link
Collaborator Author

@StrikerRUS I received an email from Guolin yesterday. CRAN is asking us fix this problem and re-submit.

Dear maintainer,

Please see the problems shown on
https://cran.r-project.org/web/checks/check_results_lightgbm.html.

Apparently your package fails its checks in a strict Latin-1* locale,
e.g. under Linux using LANG=en_US.iso88591 (see the debian-clang
results).

Please correct before 2021-04-21 to safely retain your package on CRAN.

Best,
-k

I think it's very important that {lightgbm} not get kicked off of CRAN even for a day, as that would break any users' automation that relies on installing {lightgbm} from CRAN.

I'll update this PR to make sure the package passes tests on r-devel-linux-x86_64-debian-clang.

@StrikerRUS which of these options would you prefer? I'm ok with whichever one you want, so we can come to a quick resolution well before April 21.

  1. Submit the R package to CRAN as version 3.2.0.1 (this idea was rejected in [RFC] version changes + more frequent releases #3210 but we could consider this a special case)
  2. Do a 3.2.1 release of all of LightGBM

@StrikerRUS
Copy link
Collaborator

@jameslamb I'd prefer variant 2. Luckly, we haven't merged any breaking changes that doesn't fit for minor release into master yet.

@jameslamb
Copy link
Collaborator Author

@jameslamb I'd prefer variant 2. Luckly, we haven't merged any breaking changes that doesn't fit for minor release into master yet.

ok sounds good! I'll get this PR working today, and I'll open a separate PR for the release with the usual release checklist.

@jameslamb
Copy link
Collaborator Author

jameslamb commented Apr 9, 2021

/gha run r-solaris

Workflow Solaris CRAN check has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/734409844

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.0.99.tar.gz-c7db3e53db4e4344bf08bf7f18b4af43
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.0.99.tar.gz-b1123d0e5c734026ba562be06f63df8a
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
Status: success ✔️.

@jameslamb jameslamb mentioned this pull request Apr 9, 2021
17 tasks
@jameslamb jameslamb marked this pull request as ready for review April 9, 2021 21:53
@jameslamb
Copy link
Collaborator Author

Ok I think the debian check will pass now!

@jameslamb
Copy link
Collaborator Author

Ok I've updated this with all the blocking CI fixes that are now on master (#4161, #4167, #4168, #4158), so hopefully now it should be ready for a normal review with no need to use any admin privileges.

@StrikerRUS
Copy link
Collaborator

I've pushed empty commit with the aim to re-run CI and not overwrite important logs for Dask ranker error at the same time.

.github/workflows/r_package.yml Show resolved Hide resolved
@@ -1229,7 +1234,7 @@ test_that("lgb.train() supports non-ASCII feature names", {
# UTF-8 strings are not well-supported on Windows
# * https://developer.r-project.org/Blog/public/2020/05/02/utf-8-support-on-windows/
# * https://developer.r-project.org/Blog/public/2020/07/30/windows/utf-8-build-of-r-and-cran-packages/index.html
if (!ON_WINDOWS) {
if (UTF8_LOCALE && !ON_WINDOWS) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thanks a lot for improving testing CRAN environment coverage!

R-package/cran-comments.md Outdated Show resolved Hide resolved
@jameslamb jameslamb merged commit 5a678f6 into master Apr 12, 2021
@jameslamb jameslamb deleted the ci/debian-clang-test branch April 12, 2021 02:06
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants