RcppCore / Rcpp

Seamless R and C++ Integration
https://www.rcpp.org
GNU General Public License v2.0
741 stars 213 forks source link

Remove a package from RcppExports with compileAttributes? #808

Closed mattfidler closed 6 years ago

mattfidler commented 6 years ago

Hi,

I found that RcppArmadillo and/or eigen headers interact to cause assertion errors at compile time in Linux. If the headers are included in different files, then the assertion errors disappear. However, if you are linking to both in your package, then RcppExports.cpp puts them both packages into the c++ file. Is there anyway to tell Rcpp not to include RcppArmadillo in it's included headers?

I have a work-around for now, but it is very hacky, and was wondering if I missed something in the documentation to fix this without a dirty hack.

My hack (for anyone else with the problem) is:

assignInNamespace(".parseLinkingTo",function(linkingTo) {

    if (is.null(linkingTo))
        return (character())

    linkingTo <- strsplit(linkingTo, "\\s*\\,")[[1]]
    result <- gsub("\\s", "", linkingTo)
    ret <- gsub("\\(.*", "", result);
    return(ret[ret != "RcppArmadillo"]);}
   ,"Rcpp")
eddelbuettel commented 6 years ago

Minimally reproducible example?

Methinks 439 CRAN packages with RcppArmadillo and 132 with RcppEigen show that clearly not every use leads to assert() going belly up. So for me to understand what you are actually trying to get at an example might be helpful.

Sometimes just changing order of #include statements is all it takes.

mattfidler commented 6 years ago

I tried to work out a minimal example, and could not. I have a colleague with a mimimal example, so I will try to get one from him.

mattfidler commented 6 years ago

A simple, reproducable example is below

#include <RcppArmadillo.h>
#include <RcppEigen.h>
using namespace Rcpp;
#include <stan/math.hpp>

//[[Rcpp::export]]
SEXP foo(SEXP oralSEXP) {
  const int oral = as<int>(oralSEXP);
  return Rcpp::List::create(Rcpp::Named("J") = wrap(oral));
}

It seems to be the interaction of Armadillo with STAN and Eigen. Thanks to my colleague @wwang-at-github for the simple example.

Regardless, there may be times you may not want to have some header included in the RcppExports C file.

mattfidler commented 6 years ago

Interestingly, this does not occur in the mac os nor the windows oses (I'm unsure about solaris)

eddelbuettel commented 6 years ago

That is a random snippet but not yet a complete example:

We are all busy people. Help us help you.

eddelbuettel commented 6 years ago

Now:

  1. If you add the line missing from your example
    // [[Rcpp::depends(RcppArmadillo,RcppEigen,StanHeaders)]]
    then it builds, but ends in error.

  2. If you remove / comment out #include <stan/math.hpp> it compiles fine.

I intend to close this. Feel free to file a bug with our esteemed colleague at the Stan project.

mattfidler commented 6 years ago

That is fine if you wish to close it. At the same time I think it would be helpful to allow certain headers to be removed if they are included in the LinkingTo: directive.

mattfidler commented 6 years ago

If you want me to work out a more complete example instead, I could also do that for you.

mattfidler commented 6 years ago

The thing that is missing I guess is a description file

Package: pkg
Title: What the Package Does (one line, title case)
Version: 0.0.0.9000
Authors@R: c(person("Matthew","Fidler",
       role = c("aut","cre"), email = "matthew.fidler@gmail.com"))
Description: What the package does (one paragraph).
Depends: R (>= 3.4.0)
License: GPL (>= 2)
Encoding: UTF-8
LazyData: true
LinkingTo: RcppArmadillo (>= 0.5.600.2.0), Rcpp (>= 0.12.3), StanHeaders, RcppEigen (>= 0.3.3.3.0)
RoxygenNote: 6.0.1
mattfidler commented 6 years ago

If you place that in the root of the directory with the cpp example in the src, it will throw an assert in the code. This shows up in a R CMD CHECK

eddelbuettel commented 6 years ago

But is it the fault of Rcpp when three other packages have header files that bite each other?

I do not think so. You are always free to rearrange your source code over different compilation units if you do need all three external packages.

mattfidler commented 6 years ago

No. But it would be useful if Rcpp allowed developers to work around these packages until a work-around is found...

eddelbuettel commented 6 years ago

With all due respect, no.

mattfidler commented 6 years ago

Thank you for your time. As I said I do have a work around for now.

eddelbuettel commented 4 years ago

This issue has now bitten someone else too: https://github.com/coatless/rcppensmallen/issues/19

Maybe we should revisit your workaround. How are you handling it these days?

mattfidler commented 3 years ago

Hi @eddelbuettel

I didn't see your comment earlier this year I must have overlooked it. It seems you have a work-around, but I couldn't figure out what it is.

For me, I used them in Imports instead of LinkingTo and then used R to generate the include path in the compile directory.

What was the work-around that we can use too. It would be great to use LinkingTo instead of Imports

mattfidler commented 3 years ago

I believe we got comments back that we shouldn't import RcppArmadillo from CRAN. I guess the only other option is to edit RcppExports by hand afterwards

mattfidler commented 3 years ago

Here is the current Makevars:

https://github.com/nlmixrdevelopment/RxODE/blob/49367a0e0c2ae4b792cfa622c8f9f36e18a2d6e1/src/Makevars.in#L1-L41

eddelbuettel commented 3 years ago

Yes. This seems like a overly complicated and also fairly non-standard issue.

Recall that LinkingTo: etc are all provided by R as helpers; we do not add or remove anything. They help in a great many normal situations. Yet that does not include that is somewhat unusual situations conflicts may arise---that has happened before with header files, and will like happen again.

I still see nothing actionable here for Rcpp as general remedy to a general problem. As painful as it may sound, I think the only good fix is to work locally -- maybe via configure and more custom includes. The ticket was also opened many months ago and several releases happened since. And a coauthor of mine and I also have a package where for a a reason I no longer recall right now we also have to do small post-compileAttributes() edits to RcppExports.cpp.

mattfidler commented 3 years ago

I have asked my collaborator to ask if CRAN is OK with our work-around; If not, we will be forced to use something like a post-install configure. Do you have a link to what you used; If not I can google the repository to try to see what you have done.

As always, thanks for your help.

eddelbuettel commented 3 years ago

I just tried to build the example you posted 23 months ago along with the DESCRIPTION file. It no longer builds for me as Stan and Eigen don't seem to agree. All packages at current CRAN versions.

I am sorry that I cannot be of more help. I cannot only suggest to, if possibly, simplify. Maybe it is simpler with just one of RcppArmadillo and RcppEigen rather than both, and the latter is often used with Stan. Sorry!

eddelbuettel commented 3 years ago

Actually I take that back. All it takes (and I guess well have seen that game before...) is reordering header files. (And, while I was at it, removing the flattened NAMESPACE). The files

#include <stan/math.hpp>
#include <RcppArmadillo.h>
#include <RcppEigen.h>

//[[Rcpp::export]]
SEXP foo(SEXP oralSEXP) {
  const int oral = Rcpp::as<int>(oralSEXP);
  return Rcpp::List::create(Rcpp::Named("J") = Rcpp::wrap(oral));
}

as the sole file in src/ for a baby package, updated via compileAttribute and using your DESCRIPTION above then builds, installs and runs. I use /tmp/lib to keep the test package out of my normal path:

edd@rob:/tmp/matt/pkg$ Rscript -e '.libPaths("/tmp/lib"); library(pkg); foo(42L)'
$J
[1] 42

edd@rob:/tmp/matt/pkg$ 
mattfidler commented 3 years ago

For now, I added a line to the configure to simply remove RcppArmadillo from the RcppExport generated files.

mattfidler commented 3 years ago

I think that this will only occur for edge cases and you can refer to what you did previously or this issue again, I think.