CRAN packaging #64
Comments
|
I think you've done most everything correctly.
A few points, and if @eddelbuettel has a few minutes it would be great to get his second opinion as he will be the expert.
// [[Rcpp::depends(xtensor)]]
// [[Rcpp::plugins(cpp14)]]
#include <xtensor-r/rarray.hpp>
#include <Rcpp.h>
// [[Rcpp::export]]
SEXP rray_add_cpp(const xt::rarray<double>& x, const xt::rarray<double>& y) {
const xt::rarray<double>& res = x + y;
return res;
}all good, no issues. // [[Rcpp::depends(xtensor)]]
// [[Rcpp::plugins(cpp14)]]
#include <Rcpp.h>
#include <xtensor-r/rarray.hpp>
// [[Rcpp::export]]
SEXP rray_add_cpp(const xt::rarray<double>& x, const xt::rarray<double>& y) {
const xt::rarray<double>& res = x + y;
return res;
}There is some interesting interaction of the |
|
@wolfv: You may recall that I tried to help you good folks a while back. In essence there are two major usage modes you are conflating here: ad-hoc ( |
|
@eddelbuettel My goal is that R-users can just do We will (forever) have a problem with installing with devtools from github since we're not going to add the xtensor headers to this repository. I'll try to iron that out in a container since I actually do have xtensor headers installed in my system so there are interactions. |
Yeah, we build the CRAN tarball as a cmake target, and no generated file is versioned in the repository. |
|
@wolfv, @SylvainCorlay : you are mixing two conversations. It;s better to separate them. Wolf describe a usage outcome. We can talk about that. Sylvain (needlessly, if I may add) reminds everyone that he likes @wolfv: "Does that sound doable, or not?" It does to me, and the devil is in the detail. But Rcpp facilitates dozens of packages doing that -- providing their headers for other packages, and for use by |
|
This works "out of the box" so I'm fairly certain the only issue is with the install.packages("xtensor")
#>
#> The downloaded binary packages are in
#> /var/folders/41/qx_9ygp112nfysdfgxcssgwc0000gn/T//RtmpCPDFdY/downloaded_packages
Rcpp::sourceCpp(
code = "
// [[Rcpp::depends(xtensor)]]
// [[Rcpp::plugins(cpp14)]]
#include <xtensor-r/rarray.hpp>
#include <Rcpp.h>
// [[Rcpp::export]]
SEXP rray_add_cpp(const xt::rarray<double>& x, const xt::rarray<double>& y) {
const xt::rarray<double>& res = x + y;
return res;
}
")
x <- matrix(as.double(1:10), ncol = 2)
y <- matrix(as.double(1:5))
rray_add_cpp(x, y)
#> [,1] [,2]
#> [1,] 2 7
#> [2,] 4 9
#> [3,] 6 11
#> [4,] 8 13
#> [5,] 10 15 |
@DavisVaughan could you try applying the following patch locally and tell me if this fixes the issue? The R headers are defining a |
|
It doesn't seem to work ( // [[Rcpp::depends(xtensorrr)]]
// [[Rcpp::plugins(cpp14)]]
#include <Rcpp.h>
#include <xtensor-r/rarray.hpp>
// [[Rcpp::export]]
SEXP rray_add_cpp(const xt::rarray<double>& x, const xt::rarray<double>& y) {
const xt::rarray<double>& res = x + y;
return res;
}switching the order of the includes again works. |
|
Ok, too bad .. the only other option is to store the macro in a temp macro, use |
|
Would defining There's an issue for this over in Rcpp |
|
Sure, that would definitely help. I have this code right now: |
|
I think Rcpp would have to use |
|
You can do #define STRICT_R_HEADERS
#include <Rcpp.h>and/or define/undefine as needed. It happens with other codebases at times. |
|
We will need to protect against cases where R headers or Rcpp are included without STRICT_R_HEADERS so we will probably end up locally undefining the poluting macros in xtensor. |
|
And you still can't use |
|
Easy. Just It's an R thing we too try to protect against as well but when we tried to turn it on too many things fell over so we are trying to more gradually bring it in. |
|
Even if we define that macro, client code may have included RCPP (or the R headers themselves) unfortunately. |
|
Well if you define it there won't be spillage when folks include R headers after your. And for cases where people include R headers before your (maybe via Rcpp [btw: we spell it Capital-R lowercase cpp ]) you have the option to test for their headers. |
|
Unfortunately, the undefining / redefining of macros does not really allow redefining
|
Might point is not about cmake, that is applicable regardless of the build system: We don't vendor dependencies in git repositories, and try to avoid checking in generated content. The tarball generated by the |
|
@Sylvain Yes, I got that lecture from you a few times, whether I asked for it or not :) I didn't say anything about repo or modules or whatever. Do what you want or need to do. Your repo. Now, I look after Rcpp. I check against all (by now 1500+) revdeps prior to releases. I do not think we have a single one biting with |
|
Dirk, no worries, I think we went a bit on a tangent here. The CMake stuff is just an implementation detail for us to make our package look like a R package before uploading. It could be a Python or R script as well. Totally understood that CMake is not the tool of choice for the R community. If we had something better that was as widely adopted in the C++ community we would have switched yesterday, it's that bad. Of course the In the meantime we need to decide wether we want to workaround this by putting some ifdef/undef guards in xtensor (the above example from @SylvainCorlay contains a typo, that's why it doesn't work) or wether we just want to clearly document the behavior and tell everyone to either switch the order of includes or use the strict header macro. Yep! Looking forward to xtensor + Rcpp uses as well! If there is something I am not getting from the R world I'll head over to the Rcpp channel. Thanks for your time! |
|
@wolfv I think that even with the fixed typo it doesn't work. I tried but get different errors. Maybe I have a typo too.
Two things I was able to get working are:
and
But they are rather ugly... |
|
I've created a branch in my fork called That makes it possible to get started quickly with devtools:
https://github.com/wolfv/xtensor-r/tree/package @SylvainCorlay @DavisVaughan thoughts? |
|
This is essentially the idea behind why I have |
|
@DavisVaughan we have made some progress on this. You can check out the
Let me know what you think! |
|
Now the Xtensor.R repository is
while the xtensor-r only holds the pure C++ header-only library. Besides, the R package for xtensor has been packaged for conda-forge, and properly depends on xtensor, xtensor-r, xtl instead of vendoring them. Closing the issue! |
|
This is great progress. Its on my calendar to give it a try tomorrow. I think this is a really important step. Thanks for the hard work here! |
@DavisVaughan since you made a new package, this is basically a question for you!
We have a process in our CMake file that generates a CRAN package and we have uploaded versions of this package to CRAN.
Inside the CRAN package we have copies of xtensor and xtl (we should add xsimd soon, as well).
Are we installing them in the wrong locations? I think it would be good if "our" CRAN package works out of the box – and stuff like
Rcpp::sourceCpp(...)just works.The text was updated successfully, but these errors were encountered: