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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import packages, don't depend on them #176

Closed
hadley opened this issue Jul 23, 2015 · 7 comments
Closed

Import packages, don't depend on them #176

hadley opened this issue Jul 23, 2015 · 7 comments

Comments

@hadley
Copy link

hadley commented Jul 23, 2015

Currently you have Depends: Rcpp (>= 0.11.0), utils, inline, methods, but these should all be moved into imports. It's bad practice (and not needed) to attach other packages to the search path when loading a package.

@bgoodri
Copy link
Contributor

bgoodri commented Jul 23, 2015

Thanks Hadley.

  • I'm not sure how utils got in there so it probably shouldn't be
  • For methods the manual ( https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Namespaces-with-S4-classes-and-methods ) said to put it in Depends but then footnote 53 suggests maybe it is not necessary any more now that we require R >= 3.0.2.
  • Rcpp is needed in Depends because if Rcpp is not loaded the user's model will fail with the message Error in .Object$initialize(...) : could not find function "cpp_object_initializer" which is unexported by Rcpp
  • I thought inline had a similar issue as Rcpp but now I think I'm wrong about that.

@hadley
Copy link
Author

hadley commented Jul 24, 2015

Are you using Rcpp modules? You shouldn't need to depend on Rcpp in most cases.

@bgoodri
Copy link
Contributor

bgoodri commented Jul 24, 2015

Yeah, it is this issue RcppCore/Rcpp#168

@hadley
Copy link
Author

hadley commented Jul 24, 2015

I'll try and find some time to look into this in more detail. Would be nice if we could get a fix into Rcpp. I bet me and @wch can work it out between us.

@lionel-
Copy link
Contributor

lionel- commented Aug 12, 2015

a temporary solution would be to import and reexport cpp_object_initializer(), this way Rcpp can be moved to Imports.

@bgoodri
Copy link
Contributor

bgoodri commented Aug 13, 2015

I've made this change in d531b6e and it seems to work in every situation except compiling the vignette automatically when multiple cores are used, whereupon it says

Error: processing vignette 'rstan_vignette.Rnw' failed with diagnostics:
 chunk 5 (label = callstan) 
Error in unserialize(node$con) : error reading from connection
Execution halted

But that doesn't happen if cores = 1, or if I install the package without the vignettes and run a model manually with multiple cores, or even if I explicitly call tools::buildVignette(). Any ideas? I'm currently only using 1 core in the vignette.

@bgoodri
Copy link
Contributor

bgoodri commented Jan 9, 2016

This was largely fixed for rstan 2.9.0 or maybe 2.8.2, but I believe the "fix" entails that downstream packages of rstan that come with stanmodel objects have to put Rcpp in Depends. FYI, see the rstanarm package for how this can be done.

@bgoodri bgoodri closed this as completed Jan 9, 2016
JohnReid added a commit to JohnReid/DeLorean that referenced this issue Feb 12, 2016
mm3509 added a commit to alan-turing-institute/PosteriorBootstrap that referenced this issue Mar 15, 2019
mm3509 added a commit to alan-turing-institute/PosteriorBootstrap that referenced this issue Mar 21, 2019
dkahle added a commit to dkahle/algstat that referenced this issue Mar 25, 2019
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

3 participants