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

WIP: [ci] use R 4.2 in Windows R-package CI jobs (fixes #4881) #5274

Closed
wants to merge 53 commits into from

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Jun 7, 2022

Fixes #4881.

Attempts to add support for R 4.2 in most of the R package's CI jobs.

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.

References for some concerns about Windows builds:

References

r-lib/actions#574 (comment)

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.

Related fix in r-lib/actions: r-lib/actions@7d98418

@jameslamb jameslamb changed the title [ci] use R 4.2 in most R-package CI jobs WIP: [ci] use R 4.2 in most R-package CI jobs Jun 8, 2022
@StrikerRUS
Copy link
Collaborator

I'd like to propose checking passed R version to the FindLibR.cmake module from the logs

message(STATUS "R version passed into FindLibR.cmake: ${CMAKE_R_VERSION}")

similarly to
# this check makes sure that CI builds of the package
# actually use MM_PREFETCH preprocessor definition
if [[ $R_BUILD_TYPE == "cran" ]]; then
mm_prefetch_working=$(
cat $BUILD_LOG_FILE \
| grep --count -E "checking whether MM_PREFETCH work.*yes"
)
else
mm_prefetch_working=$(
cat $BUILD_LOG_FILE \
| grep --count -E ".*Performing Test MM_PREFETCH - Success"
)
fi
if [[ $mm_prefetch_working -ne 1 ]]; then
echo "MM_PREFETCH test was not passed"
exit -1
fi

That will prevent situations like #5129 (comment).

@jameslamb jameslamb changed the title WIP: [ci] use R 4.2 in most R-package CI jobs WIP: [ci] use R 4.2 in most R-package CI jobs (fixes #4881) Jul 22, 2022
@jameslamb jameslamb changed the title WIP: [ci] use R 4.2 in most R-package CI jobs (fixes #4881) WIP: [ci] use R 4.2 in Windows R-package CI jobs (fixes #4881) Sep 20, 2022
@jameslamb
Copy link
Collaborator Author

ooooo, I think that the inet_pton detection check in configure.win might be part of the problem!

I tried running the following shell script in the CI builds for the Windows jobs.

echo "------ testing test program ----"

cat > conftest.cpp <<EOL
#include <ws2tcpip.h>
int main() {
  void* p = inet_pton;
  return 0;
}
EOL

g++ -std=gnu++11 -I"c:/rtools42/x86_64-w64-mingw32.static.posix/include" -o conftest conftest.cpp || exit 123
echo "------ done compiling ----"

./conftest || exit 123
rm -f ./conftest
rm -f ./conftest.cpp
echo "------ done running ----"

And got the following

------ testing test program ----
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*)}
Building R package

To me, that looks like inet_pton was FOUND, but was incorrectly detected as NOT FOUND because this code can't be compiled.

@jameslamb
Copy link
Collaborator Author

This also seems relevant for the CMake side: https://cmake.org/cmake/help/latest/module/CheckCXXSymbolExists.html

CheckCXXSymbolExists command is unreliable when is (potentially) an overloaded function. Since there is no reliable way to predict whether a given function in the system environment may be defined as an overloaded function or may be an overloaded function on other systems or will become so in the future, it is generally advised to use the CheckCXXSourceCompiles module for checking any function symbol (unless somehow you surely know the checked function is not overloaded on other systems or will not be so in the future).

@jameslamb
Copy link
Collaborator Author

Closing this in favor of #5503 , which includes the fixes discovered through this investigation.

@jameslamb jameslamb closed this Sep 24, 2022
@jameslamb jameslamb deleted the ci/r-4.2.0 branch September 24, 2022 05:40
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[R-package] Support R 4.2
2 participants