Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible package.skeleton issue affecting Rcpp, RcppArmadillo, RStudio (!!), ... #1087

Closed
eddelbuettel opened this issue Jun 5, 2020 · 6 comments

Comments

@eddelbuettel
Copy link
Member

So I just did the live webinar on Rcpp and got to show live how package generation now fails (from RStudio) for both Rcpp and RcppArmadillo 馃槩

It looks the NAMESPACE file now gets an 'echo' from a temp function we create but which is no longer there when the package is finished. And misses an actual exportPatter().

So a simple first fix for RcppArmadillo could be (done and tested)

modified   R/RcppArmadillo.package.skeleton.R
@@ -78,6 +78,8 @@ RcppArmadillo.package.skeleton <- function(name="anRpackage", list=character(),
     ## add a useDynLib to NAMESPACE,
     NAMESPACE <- file.path( root, "NAMESPACE")
     lines <- readLines( NAMESPACE )
+    lines <- lines[!grepl("^export.*fake", lines)]
+    lines <- c(lines, "exportPattern(\"^[[:alpha:]]+\")")
     if (! grepl("useDynLib", lines)) {
         lines <- c(sprintf("useDynLib(%s, .registration=TRUE)", name),
                    "importFrom(Rcpp, evalCpp)",        ## ensures Rcpp instantiation

do not print the fake one but add the pattern. Is my fear justified that the RStudio generator may need this too? Paging @kevinushey ....

@eddelbuettel
Copy link
Member Author

I just pushed a simpler / better fix in a new branch. In essence we seem to need to keep a fake symbol around, and already cleaned up after it at the end. Now, it seems, R changed and somehow that symbol ends up in the NAMESPACE. So ... we clean that too.

That, plus ensuring an exportPattern(...) is set may be all we need here.

@kevinushey Can you remind how it works from RStudio when we create a package? Does it call the Rcpp function?

@kevinushey
Copy link
Contributor

kevinushey commented Jun 6, 2020

Hmm, it looks like the behavior in package.skeleton() has actually changed in R 4.0.0. Before, it would insert an export pattern; now, it just exports any objects in the global environment when package.skeleton() is called. For example, I see:

$ R --vanilla --slave -e "x <- 1; package.skeleton()" && cat anRpackage/NAMESPACE
Creating directories ...
Creating DESCRIPTION ...
Creating NAMESPACE ...
Creating Read-and-delete-me ...
Saving functions and data ...
Making help files ...
Done.
Further steps are described in './anRpackage/Read-and-delete-me'.
export("x")

Compare with R 3.6:

kevinushey@Kevins-MBP:~/scratch/opkasda
$ R --vanilla --slave -e "x <- 1; package.skeleton()" && cat anRpackage/NAMESPACE
Creating directories ...
Creating DESCRIPTION ...
Creating NAMESPACE ...
Creating Read-and-delete-me ...
Saving functions and data ...
Making help files ...
Done.
Further steps are described in './anRpackage/Read-and-delete-me'.
exportPattern("^[[:alpha:]]+")

So this might warrant a closer look at what package.skeleton() is doing.

@kevinushey
Copy link
Contributor

Looks like this was changed here: wch/r-source@36b27d9#diff-a1b94fcea8b8818d4371aabf04dc2b3c

@eddelbuettel
Copy link
Member Author

eddelbuettel commented Jun 6, 2020

Yes, and for the narrow case of an empty package I now have it fixed by

  • removing the faux global symbol
  • adding the now missing exportPattern()

Question is what, if anything, to do with the more complicated usage patterns the skeleton helpers support "in theory" and which (as far as I can tell) nobody uses...

@eddelbuettel
Copy link
Member Author

Good catch on the commit to R. We should probably stick our heads together, take another look on the weekend or Monday, or report to them. Looks like a bug, doesn't it?

@eddelbuettel
Copy link
Member Author

With @dmurdoch kindly waving the cluebat at me we can assume the beahvior will stay so I think the simple PR here can be merged too.

eddelbuettel added a commit that referenced this issue Jun 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants