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] [R-package] remove code for 'rchk' checks #6545

Merged
merged 9 commits into from
Oct 29, 2024
Merged

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Jul 14, 2024

Replaces #6332

#6266 and related PRs made this project's shell scripts stricter, so they now exit with a non-0 exit code the first time anything goes wrong (e.g. some variable they require isn't set or some command they run fails).

That work revealed that the CI job supposed to be checking the R package with rchck has not actually been working, for maybe the last year.

That job currently prints this:

/__w/LightGBM/LightGBM/.ci/test_r_package.sh: line 178: docker: command not found

And then just exits "successfully" with an exit code of 0 🙃

I suspect that that's been silently broken for over a year... since #5638. That PR moved all of the Linux R-package jobs inside of containers, but the ubuntu:latest image doesn't have docker in it and isn't set up for docker-in-docker by default. Following the docs in https://github.com/kalibera/rchk/blob/master/doc/DOCKER.md, that job expects to build the R package on the host, then docker run a container with rchk in it and that package mounted in.

In addition, it seems that since then R-devel / LLVM / LightGBM / CRAN have all changed in ways that mean that rchk can no longer be run on {lightgbm}. See kalibera/rchk#35.

This PR proposes removing the rchk CI check here.

Benefits of these changes

  • simplifies the script used by most macOS and Linux R-package CI jobs in the project

How I tested this

See #6545 (comment) and the history of commits on this PR.

References

The rchk job was first added here in #4449.

CRAN rchk check description: https://raw.githubusercontent.com/kalibera/cran-checks/master/rchk/README.txt (linked from https://cran.r-project.org/web/checks/check_issue_kinds.html).

@jameslamb
Copy link
Collaborator Author

jameslamb commented Oct 28, 2024

This is not working. Running it locally, I see the same thing I'm seeing in CI.

Built the package locally, from this branch, which should result in significant stack-protection errors because I removed many Rf_unprotect() calls in lightgbm_R.cpp.

rm -rf ./packages
mkdir -p ./packages
sh build-cran-package.sh --no-build-vignettes
cp lightgbm*.tar.gz ./packages

Followed the r-hub instructions (docs link), using their images:

docker run \
    --rm \
    --platform linux/amd64 \
    -v "$(pwd)/packages":/check \
    ghcr.io/r-hub/containers/rchk:latest \
    r-check

Saw this:

objdump: Warning: Unrecognized form: 0x23
objdump: Warning: DIE at offset 0x11167d refers to abbreviation number 4258 which does not exist
Running bcheck
/opt/R/devel-rchk/bin/rchk.sh: line 83:  1224 Aborted                 $RCHK/src/$T $RBC $F > $FOUT 2>&1
==== rchk bcheck =========================================
ERROR: too many states (abstraction error?) in function strptime_internal
ERROR: too many states (abstraction error?) in function bcEval_loop
ERROR: too many states (abstraction error?) in function StringValue
ERROR: too many states (abstraction error?) in function RunGenCollect
bcheck: /usr/lib/llvm-14/include/llvm/ADT/APInt.h:1470: uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits() <= 64 && "Too many bits for uint64_t"' failed.

note: image digest from docker inspect = ghcr.io/r-hub/containers/rchk@sha256:5a05030db78164b698ae05025ba383e4129fc9fb53d7aa6b18ebaee9b8aa9c68

Followed CRAN's instructions (docs link, linked from https://cran.r-project.org/web/checks/check_issue_kinds.html) and using the kalibera/rchck images CRAN recommends using.

docker run \
    --rm \
    --platform linux/amd64 \
    -v $(pwd)/packages:/rchk/packages \
    kalibera/rchk:latest \
    /rchk/packages/lightgbm_4.5.0.99.tar.gz

Saw the same results:

objdump: Warning: Unrecognized form: 0x23
objdump: Warning: DIE at offset 0x1052d1 refers to abbreviation number 4258 which does not exist
/rchk/scripts/check_package.sh: line 100:   736 Aborted                 $RCHK/src/$T $RBC $F > $FOUT 2>&1

Library name (usually package name): lightgbm
Initialization function: R_init_lightgbm
Functions: 58
Checked call to R_registerRoutines: 1
ERROR: too many states (abstraction error?) in function strptime_internal
ERROR: too many states (abstraction error?) in function RunGenCollect
bcheck: /usr/lib/llvm-14/include/llvm/ADT/APInt.h:1470: uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits() <= 64 && "Too many bits for uint64_t"' failed.

Rchk version: 1cae90e208e97a5c41f1c3e128d99b197478443e
R version: 84255/R Under development (unstable) (2023-04-13 r84255)
LLVM version: 14.0.0

Repeated both of those with with the latest release of {lightgbm} (v4.5.0), which has been on CRAN for 3+ months (since July 26, 2024).

rm -rf ./packages
mkdir -p ./packages
wget \
    -O ./packages/lightgbm_4.5.0.tar.gz \
    https://cran.r-project.org/src/contrib/lightgbm_4.5.0.tar.gz

Saw the exact same results with both the r-hub approach/images and the CRAN/kalibera approach/images.

Some other notes:

@jameslamb
Copy link
Collaborator Author

jameslamb commented Oct 28, 2024

Given all of what's written in #6545 (comment) ... I really think that CRAN is just not running rchck on {lightgbm} any more.

It does say at https://raw.githubusercontent.com/kalibera/cran-checks/master/rchk/README.txt (which CRAN links to) that rchk

... has been written primarily for C and works less well with C++.

@StrikerRUS I think we should do the following:

  • ask on the r-package-devel mailing list if rchk is still being used, and how to tell if CRAN is running it on {lightgbm}
  • remove all rchk code and configuration in LightGBM's CI (since it is already not working)
  • possibly re-introduce it in the future if the issues described above are resolve

What do you think?

@StrikerRUS
Copy link
Collaborator

@jameslamb Thanks a lot for your investigation!

Totally support your plan. Last commit in kalibera/cran-checks was 5 months ago... So I guess they stopped running it.

@jameslamb
Copy link
Collaborator Author

Totally support your plan.

Thanks!

I noticed that a recent issue got a timely response on kalibera/rchk, so before going to the r-package-devel mailing list, I posted there to ask the rchk author directly: kalibera/rchk#35

I'll update this PR's code and description tomorrow to just remove rchk checks from this repo for now.

@jameslamb jameslamb changed the title WIP: [ci] [R-package] re-enable 'rchk' checks [ci] [R-package] remove code for 'rchk' checks Oct 28, 2024
@jameslamb
Copy link
Collaborator Author

I've updated the title and description to reflect the comments above. This is ready for review.

@jameslamb jameslamb marked this pull request as ready for review October 28, 2024 17:20
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.

LGTM! Thank you very much for connecting with rchk developer and asking questions!

@StrikerRUS StrikerRUS merged commit 4a60a53 into master Oct 29, 2024
48 checks passed
@StrikerRUS StrikerRUS deleted the ci/rchk-job branch October 29, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants