[R] Add an inline plugin for R #3626
Conversation
|
Zongs! I thought we long had one? That absolutely belongs! |
eddelbuettel
left a comment
eddelbuettel
left a comment
There was a problem hiding this comment.
I think I may make one change.
rcurtin
left a comment
rcurtin
left a comment
There was a problem hiding this comment.
Looks great to me (but, of course, my R knowledge is very limited); do you want to add a change to HISTORY.md too? 👍
| Rcpp::Rcpp.plugin.maker( | ||
| include.before = "#include <mlpack.h>", | ||
| libs = paste(openmp_flag, "$(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS)"), | ||
| LinkingTo = c("RcppArmadillo", "Rcpp", "RcppEnsmallen", "mlpack"), |
There was a problem hiding this comment.
Does RcppEnsmallen have any shared objects to link against? ensmallen is header only, so at least in C++-land there's nothing to link against. Anyway, I certainly don't know Rcpp plugins, so maybe this is needed just to indicate the dependency, I dunno. 😄
There was a problem hiding this comment.
LinkingTo justs points to the header`s location. So we need it.
There was a problem hiding this comment.
@rcurtin It's all R's funky language. LinkingTo does not link because heck why would you think it does? 😜
RcppArmadillo has been header-only for R forever and ever. Armadillo usually needs linking to get LAPACK and BLAS but when loaded from R we already have that. RcppEnsmallen (and many others) do the same, and the big big actually Yuge achievement of MLPACK 4 was to get to the same point. Header-only compiles from R.
I think I blogged a little about it when it happened and thought I had added such a helper. Clearly not. This will be good have (on CRAN in the next release) and we then spin some more wheels for awareness.
There was a problem hiding this comment.
Much appreciate the explanation, thank you both! 👍
There was a problem hiding this comment.
@eddelbuettel speaking of awareness , here the demo package (template) I had created as part of #3613. Of course it works only if there is a plug-in helper but someone can follow the instructions in the issue if they can't wait for the CRAN release.
Co-authored-by: Dirk Eddelbuettel <[email protected]>
|
@rcurtin added a note inside of |
rcurtin
left a comment
rcurtin
left a comment
There was a problem hiding this comment.
Nice, whenever you're happy with it, I say merge away! 🚀
|
Okidokie, merging in. Thanks for the fast review @eddelbuettel + @rcurtin. Also, thanks goes out to @cgiachalis for bringing it to our attention. Sorry it took 3 weeks(!) to get it rectified. If you notice anything else amiss, please create an issue and tag either myself (@coatless) or @eddelbuettel so we can work on the problem. |
4 participants
This adds an inline plugin for the R extension.
Close #3613
/cc & h/t @cgiachalis