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] use R 4.2.1 in Windows CI jobs (fixes #4881) #5503

Merged
merged 4 commits into from
Oct 9, 2022

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Sep 24, 2022

Fixes #4881.
Replaces #5274.

Changes in this PR

Why did the check in configure.win need to change?

Compiling the test program used to check for inet_pton with the compiler that ships with RTools42 (gcc 10.3), results in the following error.

conftest.cpp: In function 'int main()':
conftest.cpp:3:13: error: invalid conversion from 'INT (*)(INT, LPCSTR, PVOID)' {aka 'int (*)(int, const char*, void*)'} to 'void*' [-fpermissive]
    3 |   void* p = inet_pton;
      |             ^~~~~~~~~
      |             |
      |             INT (*)(INT, LPCSTR, PVOID) {aka int (*)(int, const char*, void*)}

To avoid that casting, this PR proposes just assigning the address of inet_pton to a function pointer for a function with exactly the same signature as the one LightGBM's code expects.

Why does the use of "lightgbm_*.tar.gz" need to be replaced with "lightgbm_$env:LGB_VER.tar.gz"?

I'm not sure...probably related to R or some tool shipping with RTools42 not working well with wildcards. While testing in #4881, I saw errors like the following:

Error: no packages were specified

Notes for Reviewers

#4881 refers to creating a comment-triggered workflow for the UCRT toolchain for Windows builds, but I don't think that's necessary any more now that R 4.2.0 has actually been released and Rtools42 contains the UCRT build tools.

I closed #4881 in favor of this because that PR was getting very busy with debugging commits and comments.

References

There is some documentation about Rtools42 adding its own compilers to PATH, but I think that just means to R's PATH (referenced by R CMD commands).

e.g., see https://cran.r-project.org/bin/windows/base/howto-R-4.2.html

To make the use of Rtools42 simpler, when R is installed via the binary installer it by default uses Rtools42 for the compilers and libraries. PATH will be set by R (inside front-ends like RGui and RTerm, but also R CMD) to include the build tools (e.g. make) and the compilers (e.g. gcc). In addition, R installed via the binary installer will automatically set R_TOOLS_SOFT (and LOCAL_SOFT for backwards compatibility) to the Rtools42 location for building R packages. This feature is only present in the installer builds of R, not when R is installed from source.

and r-lib/actions#574 (comment)

Some additional relevant information in the r-lib/actions project:

How I tested this

Given the changes to configure.win, I also tested the project on R Hub and win-builder.

Generated the CRAN source distribution like this:

sh build-cran-package.sh

Then used that .tar.gz to test on win-builder and R Hub.

passing on win-builder (click me)
  • R-release (logs)
  • R-devel (logs)
  • R-oldrelease (logs)

See https://win-builder.r-project.org/. This project is run by some of the CRAN maintainers, and very closely matches the Windows builds on CRAN.

There are functions in {devtools} to automate uploading to this service, e.g. devtools::check_win_release(). Unfortunately, those only take a director to be packaged with R CMD build. That doesn't work with the custom stuff build-cran-package.sh has to do to build vignettes:

# Object files are left behind from compiling the library to generate vignettes.

So I manually changed the maintainer to myself in DESCRIPTION, built the package with build-cran-package.sh, then uploaded manually at https://win-builder.r-project.org/upload.aspx.

passing on R Hub (click me)

I tried checking the package built from this branch on all 4 platforms supported by R Hub.

  • windows-x86_64-devel (logs)
  • windows-x86_64-oldrel (logs)
  • windows-x86_64-patched (logs)
  • windows-x86_64-release (logs)
EMAIL <- "****" # my personal email
PACKAGE_TARBALL <- "lightgbm_3.3.2.99.tar.gz"

result <- rhub::check(
    path = PACKAGE_TARBALL
    , email = EMAIL
    , check_args = c(
        "--as-cran"
    )
    , platforms = c(
        "windows-x86_64-devel"
        , "windows-x86_64-oldrel"
        , "windows-x86_64-patched"
        , "windows-x86_64-release"
    )
    , env_vars = c(
        "R_COMPILE_AND_INSTALL_PACKAGES" = "always"
        , "_R_CHECK_FORCE_SUGGESTS_" = "true"
        , "_R_CHECK_CRAN_INCOMING_USE_ASPELL_" = "true"
    )
)

Copy link
Collaborator

@jmoralez jmoralez left a comment

Choose a reason for hiding this comment

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

I'm approving to unblock but I'm not the best person to review this. Hopefully someone else can provide their review.

R-package/README.md Outdated Show resolved Hide resolved
Co-authored-by: José Morales <jmoralz92@gmail.com>
@jameslamb
Copy link
Collaborator Author

Thanks for much for that honesty, @jmoralez , much appreciated!

I agree, it would have been better to have @guolinke or @shiyu1994 review this. Normally I'd wait for one of them, but since #5525 is so time-sensitive, I'm going to merge this to unblock testing there.

These changes will make it into that PR so they'll have another chance to review there, and I think all of the extra testing with win-builder and R Hub there will be give us high confidence that this is working (or catch any issues with it 😬 ).

@jameslamb jameslamb merged commit 45dd49e into master Oct 9, 2022
@jameslamb jameslamb deleted the ci/r-windows-4.2 branch October 9, 2022 04:45
@guolinke
Copy link
Collaborator

guolinke commented Oct 9, 2022

@jameslamb no problems, this PR looks good to me.

@jameslamb
Copy link
Collaborator Author

ok thank you!

@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 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R-package] Support R 4.2
3 participants