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

Ad-hoc fix for testthat's C entrypoint #1274

Merged
merged 2 commits into from Aug 31, 2023
Merged

Ad-hoc fix for testthat's C entrypoint #1274

merged 2 commits into from Aug 31, 2023

Conversation

Enchufa2
Copy link
Member

@Enchufa2 Enchufa2 commented Aug 29, 2023

Background here. It all comes to this call that relies on tools::package_native_routine_registration_skeleton for the generation of registering code, but such function does not make any effort to detect the types of the extern declarations here. It's just void * for every argument, which works without LTO, but generates a warning with LTO enabled.

So in this patch we just substitute void * with SEXP for the particular case of testthat's C entrypoint, to make the life of these users easier. We probably could apply this to every declaration, but this should be further investigated. We can revisit this if new reports come out with other functions.

To test this:

Rcpp::Rcpp.package.skeleton()
#> Creating directories ...
#> Creating DESCRIPTION ...
#> Creating NAMESPACE ...
#> Creating Read-and-delete-me ...
#> Saving functions and data ...
#> Making help files ...
#> Done.
#> Further steps are described in './anRpackage/Read-and-delete-me'.
#> 
#> Adding Rcpp settings
#>  >> added Imports: Rcpp
#>  >> added LinkingTo: Rcpp
#>  >> added useDynLib directive to NAMESPACE
#>  >> added importFrom(Rcpp, evalCpp) directive to NAMESPACE
#>  >> added example src file using Rcpp attributes
#>  >> added Rd file for rcpp_hello_world
#>  >> compiled Rcpp attributes
setwd("anRpackage/")
testthat::use_catch()
#> > Added C++ unit testing infrastructure.
#> > Please ensure you have 'LinkingTo: testthat' in your DESCRIPTION.
#> > Please ensure you have 'Suggests: xml2' in your DESCRIPTION.
#> > Please ensure you have 'useDynLib(anRpackage, .registration = TRUE)' in your NAMESPACE.
desc <- paste(readLines("DESCRIPTION"), collapse="\n")
cat(paste0(desc, ", testthat\nSuggests: xml2\n"), file="DESCRIPTION")
Rcpp::compileAttributes()
readLines("src/RcppExports.cpp")[24]
#> [1] "RcppExport SEXP run_testthat_tests(SEXP);"

and then compilation succeeds without warning.

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests
  • Preferably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog

Copy link
Member

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

On behalf of the testthat + catch users: thank you!

Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

LGTM; the workaround is greatly appreciated!

@osorensen
Copy link

Fantastic! Thank you @Enchufa2!

@eddelbuettel
Copy link
Member

No issues whatsoever in reverse-dependency checks, so merging. Will also increment to micro version so there will be a dev version 1.0.11.2 at the Rcpp drat and at r-universe.

@eddelbuettel eddelbuettel merged commit a2b0ce5 into master Aug 31, 2023
14 checks passed
@eddelbuettel eddelbuettel deleted the fix/testthat branch August 31, 2023 13:16
doccstat added a commit to doccstat/fastcpd that referenced this pull request Sep 20, 2023
*   Remove C++ unit tests using catch and commented out the code since the new
    version of development version of Rcpp is not yet available on CRAN.
    Related pull request: RcppCore/Rcpp#1274.
*   Add more documentation for `fastcpd` method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants