Dplyr: Warn if installed Rcpp is different from Rcpp at compile time

Created on 18 Dec 2016  路  15Comments  路  Source: tidyverse/dplyr

...or at least give a message.

This came up in debugging a segmentation fault, which could be attributed to Rcpp, but didn't go away until actually recompiling dplyr (https://github.com/hadley/dplyr/issues/2308#issuecomment-267495075). Dirk says he's very careful about keeping the ABI stable, but I think we're on the safer side if we remind the user that he needs to keep both in sync. Duncan Murdoch suggested we should handle this at the package level.

@hadley @kevinushey: Thoughts?

feature

Most helpful comment

We really want to do this in every package that uses Rcpp so a helper would nice:

check_Rcpp_on_load <- function(package) {
  Rcpp_version <- packageVersion("Rcpp")

  setHook(
    packageEvent(package, "onLoad"),
    function() {
      if (Rcpp_version != packageVersion("Rcpp")) {
        message(
          "Current Rcpp version, ", packageVersion("Rcpp"), 
          ", different from version ", Rcpp_version, 
          " installed at build time of ", package, 
          " please reinstall ", package
        )
      }
    }
  )

  invisible(TRUE)
}

All 15 comments

Could Rcpp provide a function to make this easier?

I think it's as simple as

Rcpp_version <- packageVersion("Rcpp")

.onLoad <- function(...) {
  if (Rcpp_version != packageVersion("Rcpp")) {
    message(
      "Current Rcpp version ", packageVersion("Rcpp"), " different from version ",
      Rcpp_version, " installed at build time of dplyr, please reinstall dplyr")
    )
  }
}

We really want to do this in every package that uses Rcpp so a helper would nice:

check_Rcpp_on_load <- function(package) {
  Rcpp_version <- packageVersion("Rcpp")

  setHook(
    packageEvent(package, "onLoad"),
    function() {
      if (Rcpp_version != packageVersion("Rcpp")) {
        message(
          "Current Rcpp version, ", packageVersion("Rcpp"), 
          ", different from version ", Rcpp_version, 
          " installed at build time of ", package, 
          " please reinstall ", package
        )
      }
    }
  )

  invisible(TRUE)
}

Or we could go all-out and record the versions of _all_ packages available / used when the package was built (that is, all dependent packages as seen in the DESCRIPTION file). That would be very useful for e.g. Packrat as well.

@kevinushey: In Rcpp, or for all packages? How?

I was thinking that a mechanism for packages to record all of their build-time dependencies would be useful. In other words, the package could contain some code that:

  1. Parses its own DESCRIPTION file,
  2. Lists all packages in 'Imports', 'Depends' etc.,
  3. Reads those package's DESCRIPTION files,
  4. Records information about e.g. the package version and so on.

This could be stored as an R list in the package's namespace, e.g. .build_info or something. The code would be run at build time, and could potentially be used if necessary during load or otherwise. (This would normally only be relevant for e.g. packages that supply C / C++ headers, but sometimes this can matter when e.g. cached closures are shared between packages)

Step 4 could potentially be done recursively.

A super simple example of what I'm thinking of -- the R package could contain an R file, e.g. R/build.R, with something like:

.build_info <- new.env(parent = emptyenv())
(function() {
  DESCRIPTION <- read.dcf("DESCRIPTION", all = TRUE)
  imports <- strsplit(DESCRIPTION$Imports, "\\s*,\\s*")[[1]]
  imports <- gsub("^\\s*|\\s*$", "", imports)
  .build_info$Imports <- lapply(imports, function(import) {
    file <- system.file("DESCRIPTION", package = import)
    if (!file.exists(file))
      return(NA)
    read.dcf(file, all = TRUE)
  })
})()

Obviously with cleanup / helper functions for actually reading + parsing the DESCRIPTION files, and probably with a nicer structure for the output than what's returned naively by read.dcf().

Load hooks then could just use that .build_info object when needed for the specific Rcpp case.

That would be nice to have, of course best if it was built into R, perhaps as part of the __NAMESPACE__ environment. The R-devel discussion I started has starved, though.

Let's just do this for Rcpp for now

Any package providing a compiled library should have some notion of an ABI version declared in a header that is checked by dependent packages on initialization. That's what we (DTL) did with ggobi and rggobi, although it was based on structure sizes, which is obviously not completely safe. It's not clear whether R itself should have anything to do with this, but it would be nice to have some conventions defined somewhere.

@lawremi if R understood the notion of a build-time dependency, it could automatically (e.g.) automatically rebuild dplyr when you updated Rcpp (if needed).

Could just have a BinaryVersion field in the DESCRIPTION. When a package is intalled or built as a binary, it could record the binary version of all packages in the LinkingTo field. Those would then be checked on load (to avoid segfaults) and upon installation of any package (to rebuild). I'd welcome a contribution along these lines.

Edit: would be nice if the reinstalled packages were installed at their current, rather than latest, version to be least disruptive, but that might not be practical due to potential breakage of source compatibility.

As in other thread, would you also consider a BuildDepends field to describe other packages that are needed at build time? (i.e. for packages that are used to generate R code at build time)

I wonder if we can just declare e.g. LinkingTo: R6 .

I'm not sure what thread you are referring to, and this sounds like an orthogonal issue? @gmbecker has done some thinking along the lines of a more flexible, staged package build process (for e.g. compiling some DSL to R on the fly).

Was this page helpful?
0 / 5 - 0 ratings

Related issues

hadley picture hadley  路  18Comments

hadley picture hadley  路  44Comments

Malarkey73 picture Malarkey73  路  17Comments

edwindj picture edwindj  路  35Comments

stephlocke picture stephlocke  路  21Comments