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

[R-package] suppress Wcast-function-type warning in CMake-based gcc and MinGW builds (fixes #4273) #4274

Merged
merged 2 commits into from
May 15, 2021

Conversation

jameslamb
Copy link
Collaborator

This PR fixes #4273. It proposes suppressing this warning on CMake-based builds of the R package using gcc or MinGW:

warning: cast between incompatible function types from 'SEXPREC* (*)(LGBM_SE, SEXP, SEXP)' {aka 'SEXPREC* (*)(LGBM_SER*, SEXPREC*, SEXPREC*)'} to 'DL_FUNC' {aka 'void* (*)()'} [-Wcast-function-type]

This PR proposes suppressing this warning specifically for the portion of lightgbm_R.cpp that handles routine registration. The pattern used in {lightgbm} is exactly the same one used in many other R packages on CRAN (https://github.com/search?q=R_CallMethodDef+org%3Acran+filename%3A*.cpp), and the extensive unit tests covering the package give me confidence that this isn't causing any issues.

I think that removing this warning is useful because it remove unnecessary noise from users' logs and might prevent users who are trying to debug installations issues from spending time focused on warnings that are not actually problematic.

Notes for reviewers

See #4273 for more details on this warning.

See https://cran.r-project.org/doc/manuals/r-release/R-exts.html#useDynLib and the description in #4155 for more background information on how routine registration in R packages works.

CRAN does have a rule preventing the of pragmas that suppress warnings, but the pragmas introduced in this PR will be removed when preparing a CRAN package.

-e 's/^.*#pragma GCC diagnostic.*$//' \

@jameslamb jameslamb requested review from shiyu1994 and StrikerRUS May 10, 2021 22:23
@jameslamb jameslamb requested a review from Laurae2 as a code owner May 10, 2021 22:23
@jameslamb jameslamb marked this pull request as draft May 10, 2021 23:44
@jameslamb jameslamb changed the title [R-package] suppress Wcast-function-type warning in CMake-based gcc and MinGW builds (fixes #4273) WIP: [R-package] suppress Wcast-function-type warning in CMake-based gcc and MinGW builds (fixes #4273) May 10, 2021
@jameslamb
Copy link
Collaborator Author

I've converted this back to a draft. It seems R CMD check is unhappy with the use of this pragma.

All CMake-based builds from https://github.com/microsoft/LightGBM/pull/4274/checks?check_run_id=2550215720 are failing with this warning.

* checking pragmas in C/C++ headers and code ... WARNING

File which contains non-portable pragma(s)

  'src/src/lightgbm_R.cpp'

File which contains pragma(s) suppressing diagnostics:

  'src/src/lightgbm_R.cpp'

The MinGW + R 3.6 build (but not the MinGW + R 4.0 build) also has this warning:

D:\a\LightGBM\LightGBM\lightgbm.Rcheck\00_pkg_src\lightgbm\src\src\lightgbm_R.cpp:684:32: warning: unknown option after '#pragma GCC diagnostic' kind [-Wpragmas]

 #pragma GCC diagnostic ignored "-Wcast-function-type"

link: https://github.com/microsoft/LightGBM/pull/4274/checks?check_run_id=2550215720

and clang builds have this

/home/runner/work/LightGBM/LightGBM/lightgbm.Rcheck/00_pkg_src/lightgbm/src/src/lightgbm_R.cpp:684:32: warning: unknown warning group '-Wcast-function-type', ignored [-Wunknown-warning-option]
#pragma GCC diagnostic ignored "-Wcast-function-type"
                               ^
1 warning generated.

link: https://github.com/microsoft/LightGBM/pull/4274/checks?check_run_id=2550215599

So now I think that maybe it would be better to suppress this from the CMake side (but only for the R package).

@jameslamb
Copy link
Collaborator Author

Ok I think this has successfully suppressed this warning, and this PR is ready for review.

@jameslamb jameslamb marked this pull request as ready for review May 11, 2021 00:35
@jameslamb jameslamb changed the title WIP: [R-package] suppress Wcast-function-type warning in CMake-based gcc and MinGW builds (fixes #4273) [R-package] suppress Wcast-function-type warning in CMake-based gcc and MinGW builds (fixes #4273) May 11, 2021
@StrikerRUS
Copy link
Collaborator

@jameslamb
Is this PR ready for review or supposed to be revised after the discussion with @david-cortes in #4273?

@jameslamb
Copy link
Collaborator Author

I think it can be reviewed. I interpreted the most recent comment in that discussion, #4273 (comment) , as saying "ok actually it's not related to use or missing use of extern C", so I can't think of other things to try.

Given the wide range of operating system + R version + compiler versions the R package is tested against, I'm fairly confident that this warning can be ignored and that it would improve the user experience to suppress it.

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.

I've checked all CI logs to make sure that proposed if condition doesn't leave uncovered cases.
And I'm OK to ignore this warning.

@jameslamb
Copy link
Collaborator Author

I've checked all CI logs to make sure that proposed if condition doesn't leave uncovered cases.
And I'm OK to ignore this warning.

ok thanks very much!

@jameslamb jameslamb merged commit db2e976 into microsoft:master May 15, 2021
@jameslamb jameslamb deleted the fix/r-gcc-warning branch May 15, 2021 02:31
@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.

[R-package] gcc warning on CMake-based builds: 'cast between incompatible function types'
2 participants