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

Check existence of inet_pton for win32 in CMakeLists.txt (fixes #5019) #5159

Merged
merged 14 commits into from
May 23, 2022

Conversation

shiyu1994
Copy link
Collaborator

This is to fix #5019. We manually check the existence of inet_pton in CMakeLists.txt to avoid inet_pton being redefined in src/network/socket_wrapper.hpp.

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 for working on this!

Could you please add inet_pton existence check in configure.win too like it was stated in #5019 (comment)?
This is essential because CRAN doesn't use CMake and builds there will stop working with removed #ifndef _UCRT guard with some recent versions of MSYS according to #4923.

@StrikerRUS StrikerRUS added the fix label Apr 21, 2022
@shiyu1994 shiyu1994 requested a review from jmoralez as a code owner April 21, 2022 02:48
@shiyu1994
Copy link
Collaborator Author

Could you please add inet_pton existence check in configure.win too like it was stated in #5019 (comment)?

@StrikerRUS Thanks for the reminder. I've added that in ff8c604. Could you please help to check?

BTW, do you have any idea why some actions of R packages with gcc and R4.1 hangs?

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

R changes look good to me, thanks! I'm going to kick off the Solaris and valgrind jobs to give us greater confidence that this will work on CRAN.

CMakeLists.txt Show resolved Hide resolved
@jameslamb
Copy link
Collaborator

/gha run r-solaris

@jameslamb
Copy link
Collaborator

/gha run r-valgrind

@jameslamb jameslamb changed the title Check existence of inet_pton for win32 in CMakeLists.txt (fix #5019) Check existence of inet_pton for win32 in CMakeLists.txt (fixes #5019) Apr 22, 2022
@jameslamb
Copy link
Collaborator

/gha run r-valgrind

@jameslamb
Copy link
Collaborator

It looks like the valgrind build is being triggered by comments correctly

For example, https://github.com/microsoft/LightGBM/runs/6122965595?check_suite_focus=true

image

But is then failing prior to even building the R package.

https://github.com/microsoft/LightGBM/runs/6122967527?check_suite_focus=true

/usr/bin/git init /__w/LightGBM/LightGBM
  Initialized empty Git repository in /__w/LightGBM/LightGBM/.git/
  /usr/bin/git remote add origin https://github.com/microsoft/LightGBM
  Error: fatal: unsafe repository ('/__w/LightGBM/LightGBM' is owned by someone else)
  To add an exception for this directory, call:
  
  	git config --global --add safe.directory /__w/LightGBM/LightGBM
  Error: The process '/usr/bin/git' failed with exit code 1[28](https://github.com/microsoft/LightGBM/runs/6122967527?check_suite_focus=true#step:4:28)

I suspect that was just missed in #5152. Sorry about that! Will put up a PR shortly.

CMakeLists.txt Outdated Show resolved Hide resolved
R-package/configure.win Outdated Show resolved Hide resolved
R-package/configure.win Outdated Show resolved Hide resolved
R-package/configure.win Outdated Show resolved Hide resolved
src/network/socket_wrapper.hpp Outdated Show resolved Hide resolved
src/network/socket_wrapper.hpp Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
shiyu1994 and others added 2 commits April 24, 2022 22:48
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@shiyu1994
Copy link
Collaborator Author

shiyu1994 commented May 9, 2022

/gha run r-solaris

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

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.3.2.99.tar.gz-66562bfd23ac42ff942995a915f1b396
Reports also have been sent to LightGBM public e-mail: https://yopmail.com?lightgbm_rhub_checks
Status: failure ❌.

@shiyu1994
Copy link
Collaborator Author

shiyu1994 commented May 9, 2022

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/2292822851

Status: success ✔️.

@shiyu1994
Copy link
Collaborator Author

The r-solaris check fails with the following information

/usr/bin/ld: cannot find -lgfortran
collect2: error: ld returned 1 exit status
make[1]: *** [/usr/share/R/share/make/shlib.mk:10: Matrix.so] Error 1
make[1]: Leaving directory '/tmp/Rtmph9P7SX/R.INSTALL58396355090c/Matrix/src'
ERROR: compilation failed for package ‘Matrix’
* removing ‘/usr/local/lib/R/site-library/Matrix’

@StrikerRUS
Copy link
Collaborator

StrikerRUS commented May 10, 2022

/gha run r-solaris

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

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.3.2.99.tar.gz-4f49e421e97542b4876d80c696893832
Reports also have been sent to LightGBM public e-mail: https://yopmail.com?lightgbm_rhub_checks
Status: success ✔️. (Fake status set manually to pass CI checks. Actually, Solaris support was removed in #5226.)

R-package/configure.win Outdated Show resolved Hide resolved
@StrikerRUS
Copy link
Collaborator

The r-solaris check fails

Still fails.

Maybe worth reporting to https://github.com/r-hub/rhub/issues?
I believe our actions cannot affect Matrix package compilatibility.
@jameslamb WDYT?

@jameslamb
Copy link
Collaborator

WDYT?

I agree! I see the following error at the top of the logs from the Solaris jobs, which I suspect is the root cause.

46#> Fatal server error:

[  47](https://builder.r-hub.io/status/lightgbm_3.3.2.99.tar.gz-4f49e421e97542b4876d80c696893832#L47)#> Could not create server lock file: /tmp/.X22-lock

I found one issue with a similar error, where maintainers point to "GitHub rate limiting" as a problem: r-hub/rhub#492 (comment).

I also see the following in the logs:

Error: package or namespace load failed for ‘ps’ in dyn.load(file, DLLpath = DLLpath, ...):
 unable to load shared object '/export/home/XKCQ2Gb/R/00LOCK-ps/00new/ps/libs/ps.so':
  ld.so.1: R: fatal: relocation error: file /export/home/XKCQ2Gb/R/00LOCK-ps/00new/ps/libs/ps.so: symbol psll_memory_uss: referenced symbol not found

Anyway, I can try tonight submitting to R-hub from master. If that works, then maybe that means this was a temporary issue. If not, then I'll report it to {rhub} at that issues link.

Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@jameslamb
Copy link
Collaborator

Anyway, I can try tonight submitting to R-hub from master

I just built the package like this:

sh build-cran-package.sh

Then manually triggered Solaris builds like this:

package_tarball <- paste0("lightgbm_", readLines("VERSION.txt")[1], ".tar.gz")
rhub::check(
    path = package_tarball
    , email = "FILL_IN_EMAIL_HERE"
    , check_args = "--as-cran"
    , platform = c(
        "solaris-x86-patched"
        , "solaris-x86-patched-ods"
    )
    , env_vars = c(
        "R_COMPILE_AND_INSTALL_PACKAGES" = "always"
    )
)

With email set to the one linked in #5159 (comment).

Statuses should be sent there once the jobs complete.

Links:

@StrikerRUS
Copy link
Collaborator

image

Hasn't this meant that our token was overwritten?

token <- c(
91L, 98L, 91L, 142L, 142L, 99L, 96L, 91L, 98L, 94L, 99L, 92L, 94L, 144L, 90L, 139L,
139L, 143L, 139L, 91L, 99L, 142L, 97L, 93L, 144L, 99L, 139L, 143L, 97L, 99L, 97L, 94L
)

rhub::validate_email(
email = intToUtf8(email - 42L)
, token = intToUtf8(token - 42L)
)

@StrikerRUS
Copy link
Collaborator

I just get it! Matrix installation happens inside wch1/r-debug, not at RHub.

container: wch1/r-debug

- name: Run tests on Solaris
shell: bash
run: ./.ci/test_r_package_solaris.sh

# installation of dependencies needs to happen before building the package,
# since `R CMD build` needs to install the package to build vignettes
Rscript -e "install.packages(c('R6', 'data.table', 'jsonlite', 'knitr', 'Matrix', 'RhpcBLASctl', 'rmarkdown', 'rhub', 'testthat'), dependencies = c('Depends', 'Imports', 'LinkingTo'), repos = 'https://cran.r-project.org', Ncpus = parallel::detectCores())" || exit -1

Job fails even before submitting the LightGBM package to RHub.

Rscript ./.ci/run_rhub_solaris_checks.R lightgbm_*.tar.gz $log_file || exit -1

res_object <- rhub::check(

@jameslamb
Copy link
Collaborator

Hasn't this meant that our token was overwritten?

It would surprise me if that was true. I assumed that when I was prompted for a new token, R-hub was issuing me an additional token for use with that email, and that you could have multiple associated with the same email address.

If that's not true and we find out that the token has been overwritten, then I apologize and I'll put up a PR to fix the comment-triggered jobs.

@jameslamb
Copy link
Collaborator

Job fails even before submitting the LightGBM package to RHub.

ooooh I see! I was wondering where @shiyu1994 saw failures related to {Matrix} when I didn't see that in the logs.

I guess then, based on #5159 (comment), we need to install gfortran into that job's environment prior to building the package.

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!

@StrikerRUS StrikerRUS requested a review from jameslamb May 21, 2022 22:12
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for the fix!

@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.

Troubles installing lightgbm GPU on windows
4 participants