Preparing for Rcpp 1.0.13 and asking four packages for an adjustment

[eddelbuettel]

Rcpp aims at a six-months release cycle with releases in January and July, and has followed this pattern for a few years. The next release 1.0.13 is due next month, and with the ongoing changes in R(-devel) to more formally define the C API of R, we now merged a PR (#1310) prepared by Kevin to avoid the use of DATAPTR. (It also 'hardens' out of bounds access a little; the opt-out config variable that always existed is still there should you need it.)

The PR was tested diligently via nearly all 2800 reverse dependencies. It lead us to soften one stop() to a warning(). In the last run, four packages exhibited issues that are 'new relative to the CRAN version'. We recognise that we are imposing a small change on others, but we are doing so in the spirit of making R more solid using more well defined interfaces. We will that a change improving use and setup for over 2800 package is worth the small price of asking for a change in just four packages.

The packages in question, their relative versions. maintainers and repos are

• bayesianVARs (0.1.2) @luisgruber https://github.com/luisgruber/bayesianVARs -- passes with 0.1.3
• PKPDsim (1.3.0) @ronkeizer @roninsightrx https://github.com/InsightRX/PKPDsim
• reservr (0.0.2) @AshesITR https://github.com/AshesITR/reservr -- no longer an issue in 0.0.3 on CRAN as of 2024-06-24
• TreeDist (2.7.0) @ms609 https://github.com/ms609/TreeDist/ -- passes with rebuilt TreeTools

Maintainers (and of course everybody else) can access a release candidate for the upcoming Rcpp 1.0.13 in the current version from its repo¹, the rcpp drat², or r-universe³⁴.

I plan to follow-up in email too but encourage everybody to follow-up here (or on the rcpp-devel list, if you are subscribed already; else consider subscribing and 'lurking').

So if there are any questions, just ask.

PS As noted below, I prepared 1.0.12.4 and eg shipped to the drat repo but failed to then push here so r-universe was behind in it build. The Rcpp repo will have had the correct code merged but not at the right version. This should now be fine with 1.0.12.4 at sha1 5189611. The key merged was PR #1310 merged at 5614a8b so if you installed from source (eg via remotes) or from r-universe) within the last day or so after i opened this issue you will have gotten, it just did not yet call itself 1.0.12.4 as it does now. Sorry for any confusion on that.

Footnotes

1. Use remotes::install("RcppCore/Rcpp") from the default branch which will fetch it ↩

2. Use install.packages(): install.packages("Rcpp", repos=c("https://RcppCore.github.io/drat", "https://cloud.r-project.org")) and see https://rcppcore.github.io/drat/ for more ↩

3. Use install.packages('Rcpp', repos = c('https://rcppcore.r-universe.dev', 'https://cloud.r-project.org')) and see https://rcppcore.r-universe.dev/Rcpp ↩

4. As I type this, r-universe is still at 1.0.12.3 but should catch up to 1.0.12.4 within the hour. ↩

[JanMarvin]

So if there are any questions, just ask.

Do you want to approach the note regarding STRING_PTR and VECTOR_PRT in this release? Given that R-core is currently changing what is non-public API and what not, it might be a good idea to wait until the dust has settled, but I somehow fear that CRAN will ask about these notes, if they appear during package submission. (The STRING_PTR could probably avoided using something like const_cast<SEXP*>(STRING_PTR_RO(x)) in get_string_ptr() and the VECTOR_PTR could probably be disabled for R >= 4.5.0).

[eddelbuettel]

Release 1.0.13 should only cover what is in the repo right now, the rest will have to wait for 1.0.14. As you say, lots of moving parts still. (We could work on changes in a branch but the next release is so close that I do not plan on anything major or potentially disruptive.)

[ms609]

Acknowledged, and certainly in favour of the motivation behind these changes.

I've downloaded Rcpp 1.0.12.4 and am looking to see what changes I need to make in TreeDist. However, as an inexperienced C++ user, I'm struggling to work out what action I need to take. I can't find any use of the string DATAPTR in the TreeDist codebase, and when I try to build or load_all TreeDist, the R session crashes with no error message evident. Any pointers would be gratefully received.

[AshesITR]

@eddelbuettel I see no issues with devtools::check() when installing the GitHub version of Rcpp and checking my current master branch.

[eddelbuettel]

@ms609 Maybe start with your tests? I see

  > library("testthat", warn.conflicts = FALSE)
  > library("TreeDist")
  >
  > test_check("TreeDist")
  Loading required package: ape

   *** caught segfault ***
  address 0x55b19059fec8, cause 'memory not mapped'

But it could of course be my end? Maybe ape needs to be rebuilt. If you cannot reproduce it may be a non-issue?

@AshesITR I see this at the end of the tests which may be the new out-of-bounds warning?

  ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure ('test-dist_erlangmix.R:16:3'): erlang mixture distribution works ───
  `fit(dist, x, init = "shapes", shapes = as.numeric(params$shapes))` produced warnings.
  ── Failure ('test-dist_erlangmix.R:18:3'): erlang mixture distribution works ───
  `fit(dist, x, init = "fan", spread = 3L)` produced warnings.
  ── Failure ('test-dist_erlangmix.R:20:3'): erlang mixture distribution works ───
  `fit(dist, x, init = "kmeans")` produced warnings.
  ── Failure ('test-dist_erlangmix.R:21:3'): erlang mixture distribution works ───
  `fit(dist, x, init = "cmm")` produced warnings.

  [ FAIL 4 | WARN 0 | SKIP 29 | PASS 1203 ]
  Error: Test failures
  Execution halted

[AshesITR]

@eddelbuettel This is curious; tests pass for me.
Could you show me the warnings generated when you run the tests?

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 2444 ]
> packageVersion("Rcpp")
[1] ‘1.0.12.3’

[eddelbuettel]

@AshesITR Sorry that may be partially my fault. I had prepared 1.0.12.4 and eg pusged it to the rcpp-drat (see eg here in its src/contrib) but had failed to actually commit the DESCRIPTION and ChangeLog change. That said, if you installed from the main branch you would have had the changes. In any event, my bad and let's try to refer to that tarball as 1.0.12.4. It will also be at the r-universe site soon.

With it, and on the Debian testing machine with current version of everything I see what is under 'details' below. I see you also have a pending new version incoming at CRAN, so that may need retesting.

* using log directory ‘/home/dirk/tmp/prrd/reservr.Rcheck’
* using R version 4.4.0 (2024-04-24)
* using platform: x86_64-pc-linux-gnu
* R was compiled by
    gcc (Debian 13.2.0-23) 13.2.0
    GNU Fortran (Debian 13.2.0-23) 13.2.0
* running under: Debian GNU/Linux trixie/sid
* using session charset: UTF-8
* using options ‘--no-manual --no-vignettes’
* checking for file ‘reservr/DESCRIPTION’ ... OK
* this is package ‘reservr’ version ‘0.0.2’
* package encoding: UTF-8
* checking package namespace information ... OK
* checking package dependencies ... NOTE
Package suggested but not available for checking: ‘formattable’
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking for sufficient/correct file permissions ... OK
* checking whether package ‘reservr’ can be installed ... [56s/76s] OK
* used C++ compiler: ‘g++ (Debian 13.2.0-25) 13.2.0’
* checking installed package size ... OK
* checking package directory ... OK
* checking ‘build’ directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking code files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking loading without being on the library search path ... OK
* checking whether startup messages can be suppressed ... OK
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... [20s/24s] OK
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... OK
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... OK
* checking line endings in C/C++/Fortran sources/headers ... OK
* checking line endings in Makefiles ... OK
* checking compilation flags in Makevars ... OK
* checking for GNU extensions in Makefiles ... NOTE
GNU make is a SystemRequirements.
* checking for portable use of $(BLAS_LIBS) and $(LAPACK_LIBS) ... OK
* checking use of PKG_*FLAGS in Makefiles ... OK
* checking compilation flags used ... OK
* checking compiled code ... OK
* checking installed files from ‘inst/doc’ ... OK
* checking files in ‘vignettes’ ... OK
* checking examples ... [42s/45s] OK
* checking for unstated dependencies in ‘tests’ ... OK
* checking tests ... [53s/56s] ERROR
  Running ‘testthat.R’ [52s/56s]
Running the tests in ‘tests/testthat.R’ failed.
Complete output:
  > library(testthat)
  > library(reservr)
  > 
  > test_check("reservr")
  ── Skip ('/home/dirk/tmp/prrd/reservr.Rcheck/tests/testthat/helpers.R:13:3'): set floatx to 64-bit ──
  Reason: TensorFlow not available for testing
  
  [ FAIL 4 | WARN 0 | SKIP 29 | PASS 1203 ]
  
  ══ Skipped tests (29) ══════════════════════════════════════════════════════════
  • TensorFlow not available for testing (29): 'test-dist_beta.R:16:3',
    'test-dist_binomial.R:26:3', 'test-dist_blended.R:108:3',
    'test-dist_dirac.R:25:3', 'test-dist_discrete.R:39:3',
    'test-dist_erlangmix.R:67:3', 'test-dist_erlangmix.R:112:3',
    'test-dist_exponential.R:17:3', 'test-dist_gamma.R:17:3',
    'test-dist_genpareto.R:42:5', 'test-dist_lognormal.R:17:3',
    'test-dist_mixture.R:60:3', 'test-dist_negbinomial.R:18:3',
    'test-dist_normal.R:17:3', 'test-dist_pareto.R:17:3',
    'test-dist_poisson.R:18:3', 'test-dist_translate.R:56:3',
    'test-dist_trunc.R:42:3', 'test-dist_uniform.R:21:3',
    'test-dist_weibull.R:17:3', 'test-interval.R:159:3', 'test-tf_fit.R:2:3',
    'test-tf_fit.R:74:3', 'test-tf_fit.R:127:3', 'test-tf_fit.R:179:3',
    'test-tf_fit.R:239:3', 'test-tf_fit.R:281:3', 'test-tf_fit.R:332:3',
    'test-tf_initialise.R:2:3'
  
  ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure ('test-dist_erlangmix.R:16:3'): erlang mixture distribution works ───
  `fit(dist, x, init = "shapes", shapes = as.numeric(params$shapes))` produced warnings.
  ── Failure ('test-dist_erlangmix.R:18:3'): erlang mixture distribution works ───
  `fit(dist, x, init = "fan", spread = 3L)` produced warnings.
  ── Failure ('test-dist_erlangmix.R:20:3'): erlang mixture distribution works ───
  `fit(dist, x, init = "kmeans")` produced warnings.
  ── Failure ('test-dist_erlangmix.R:21:3'): erlang mixture distribution works ───
  `fit(dist, x, init = "cmm")` produced warnings.
  
  [ FAIL 4 | WARN 0 | SKIP 29 | PASS 1203 ]
  Error: Test failures
  Execution halted
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes ... OK
* checking running R code from vignettes ... SKIPPED
* checking re-building of vignette outputs ... SKIPPED
* DONE
Status: 1 ERROR, 2 NOTEs

[AshesITR]

I did test using remotes::install_github("RcppCore/Rcpp") and the currently pending for release version of reservr (labelled 0.0.3).
Unfortunately, the detail output of R CMD check doesn't contain the messages.

0.0.2 has a bunch of issues regarding length-0 input to the C++ routines, so if you tested that version, it is probably worth testing my current master branch instead.

If you want, you could try running this snippet to see if we have a problem in the 0.0.3 version and help me find it

# remotes::install_github("AshesITR/reservr")
library(reservr)
set.seed(1337L)
dist <- dist_erlangmix(list(NULL, NULL, NULL))
params <- list(
  shapes = list(1L, 4L, 12L),
  scale = 2.0,
  probs = list(0.5, 0.3, 0.2)
)
alt_params <- list(
  shapes = list(2L, 6L, 100L),
  scale = 0.1,
  probs = list(0.7, 0.2, 0.1)
)
x <- dist$sample(100L, with_params = params)

str(fit(dist, x, init = "shapes", shapes = as.numeric(params$shapes)))

[eddelbuettel]

Of course, if you refer to a newer version than what is released at CRAN we will be talking about likely different outcomes based on different inputs. Reverse dependency checks commonly deploy the (then-current) released version, that is how CRAN tests acceptance. I can and will catch up to reservr 0.0.3 if it makes it to CRAN to assert if it does address the issues I saw with 0.0.2.

[ms609]

@eddelbuettel I've been able to reproduce your result in a GitHub actions run.
Again I would see this error as occurring when the TreeDist package is loaded (which would also happen at the completion of load_all() as I mentioned above). So I can't get to the point of running the tests. What would help me to identify the bits of code I need to look carefully at would be a better picture of how the latest updates have changed the behaviour of Rcpp; i.e. which bits of code are likely to have changed their behaviour as a result of the update?

[eddelbuettel]

@ms609 Strange that it covers all platforms. That is a bit of head scratcher -- I don't think I have seen that.

Your TreeDist package has quite a dependency tail which may make it tricky to prune this to a more minimal example:

> db <- tools::CRAN_package_db()
> tools::package_dependencies("TreeDist", recursive=TRUE, db=db)[[1]]
 [1] "stats"       "ape"         "cli"         "colorspace"  "memoise"     "phangorn"    "Rdpack"     
 [8] "shiny"       "shinyjs"     "TreeTools"   "Rcpp"        "nlme"        "lattice"     "graphics"   
[15] "methods"     "utils"       "parallel"    "digest"      "grDevices"   "rlang"       "cachem"     
[22] "fastmatch"   "generics"    "igraph"      "Matrix"      "quadprog"    "tools"       "rbibutils"  
[29] "httpuv"      "mime"        "jsonlite"    "xtable"      "fontawesome" "htmltools"   "R6"         
[36] "sourcetools" "later"       "promises"    "crayon"      "fastmap"     "withr"       "commonmark" 
[43] "glue"        "bslib"       "lifecycle"   "bit64"       "PlotTools"   "RCurl"       "R.cache"    
[50] "bit"         "base64enc"   "jquerylib"   "sass"        "magrittr"    "pkgconfig"   "vctrs"      
[57] "cpp11"       "grid"        "R.methodsS3" "R.oo"        "R.utils"     "bitops"      "fs"         
[64] "rappdirs"   
> 

One possible approach may be start at the other end with a 'fake' new empty package and add one of the core dependencies at a time to see what happens where. It is alwasy possible that we engineered a binary interface incompatibility here, but I also think that is unlikely as we have not affected some 2800 oother packages.

[roninsightrx]

thanks @eddelbuettel, found the issue in PKPDsim, and made a fix.

[eddelbuettel]

@AshesITR Nice -- version 0.0.3 now on CRAN tests cleanly. So that one is taken care of as you surmised -- yay!!

[SermetPekin]

@eddelbuettel I've been able to reproduce your result in a GitHub actions run. Again I would see this error as occurring when the TreeDist package is loaded (which would also happen at the completion of load_all() as I mentioned above). So I can't get to the point of running the tests. What would help me to identify the bits of code I need to look carefully at would be a better picture of how the latest updates have changed the behaviour of Rcpp; i.e. which bits of code are likely to have changed their behaviour as a result of the update?

I wanted to give it a try to fix the dependency resolution issue and I hope this helps. I also added a Dockerfile to streamline the setup and testing process.

The fGarch package was causing the resolution issue due to its dependency on Matrix (>= 1.5-0), which required a newer version of R. By explicitly installing Matrix and other critical dependencies in the correct order, we can prevent issues related to missing or conflicting dependencies.

PR

[luisgruber]

@eddelbuettel I see no issue after installing the GitHub version of Rcpp and checking my current master branch (Version 0.1.2.9000 which contains a bugfix to the released version on CRAN). I will submit to CRAN. Hope this will solve the issue.

[ms609]

After rebuilding the dependency "TreeTools" (included in the LinkingTo DESCRIPTION field), TreeDist seems to operate correctly. In case there's another underlying issue I haven't detected, is it possible to test whether this action also clears up your reverse dependency check?

[eddelbuettel]

@ms609 "Maybe" -- sadly I don't know which of the packages in the long list I may need to rebuild on my box. I will try to take a look (once back from some traveling).

[ms609]

Thanks – I think you should only need to rebuild TreeTools. The issue seems (I think) to arise where the C++ code uses an Rcpp::XPtr to a TreeTools::ClusterTable object (included using #include <TreeTools/ClusterTable.h>). My guess is that if TreeTools was built using a different version of Rcpp to TreeDist, this might cause undefined behaviour.

(NB I've been part-way through a project to remove a dependency on one package that contributes heavily to this list of dependencies; this has given me fresh enthusiasm to finish that job!)

[eddelbuettel]

My guess is that if TreeTools was built using a different version of Rcpp to TreeDist, this might cause undefined behaviour.

Luckily that is generally not the case (we use a lot of XPtr constructs in some projects) but I can try a rebuild. Could it maybe be something related to an old version of Matrix? I think rebuilt everything that compiles against its headers and see no other 'sudden deaths'.

[ms609]

I'd be surprised if Matrix was related; it's a dependency of another package (phangorn) that's only tangentially used; I don't think any Matrix-dependent functions are directly or indirectly called by TreeDist. The segmentation faults I'd been seeing occur within a TreeDist C++ function that calls TreeTools::ClusterTable (but no Matrix functions), and disappeared when I re-installed TreeTools and TreeDist.

[eddelbuettel]

Noted. I will give rebuilding it a try when I get a moment (am traveling) and report back.

[eddelbuettel]

@luisgruber Thank you -- the new version passes as expected. Very nice.

@ms609 As you suggested, a rebuild of TreeTools solved it here too. Strange as it had been built on on June 10, but I take it.

[eddelbuettel]

@roninsightrx Do you plan to roll a new (minor) release to CRAN? I will probably try to upload a new Rcpp within a week. It is good enough to be able to explain to CRAN that an issue they may see is already taken care of, but it is of course even easier not to have the conversation :)

[eddelbuettel]

This release is now on CRAN, with breakage at PKPDsim (for which @roninsightrx has a fix, albeit has not uploaded a fixed version) and another package (for which I could not corrobate the breakge, CRAN eventually agreed to proceed.)

So closing the issue. Big thanks again to everybody for the extremely fast and impressive fixes (even if one did not make it to the repo yet). Your cooperation really helps with maintaining the package.