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

Automatically register init functions for RcppModules (closes #704) #705

Merged
merged 2 commits into from Jun 2, 2017

Conversation

Projects
None yet
4 participants
@jjallaire
Member

jjallaire commented Jun 2, 2017

Addresses #704

RcppModules include a native routine "boot" function of the form _rcpp_module_boot_*. These functions are not discovered by tools::package_native_routine_registration_skeleton so generally need to be hand-added to init.c.

This also implies that our current auto-generation of native routine registrations for RcppExports.cpp won't register these functions and will therefore fail to create a loadable package. 2 alternatives here:

  1. When we see an Rcpp module simply don't attempt to generate native routine registrations (sort of like we do now when we see init.c).

  2. Generate native routine registrations for the Rcpp modules.

Happily, we already parse for Rcpp module declarations (since sourceCpp supports exporting modules) so it's a very straightforward change to generate the _rcpp_module_boot_* functions, which is what we do in this PR.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 2, 2017

Member

Nicely done.

Member

eddelbuettel commented Jun 2, 2017

Nicely done.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 2, 2017

Member

Shame on me for not escalating this in March when I adjusted the example we unit test.

Member

eddelbuettel commented Jun 2, 2017

Shame on me for not escalating this in March when I adjusted the example we unit test.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 2, 2017

Codecov Report

Merging #705 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
+ Coverage   89.54%   89.57%   +0.03%     
==========================================
  Files          66       66              
  Lines        3587     3588       +1     
==========================================
+ Hits         3212     3214       +2     
+ Misses        375      374       -1
Impacted Files Coverage Δ
src/attributes.cpp 98.4% <100%> (ø) ⬆️
R/Rcpp.package.skeleton.R 84.43% <100%> (+0.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8d1e93...7f872ca. Read the comment docs.

codecov-io commented Jun 2, 2017

Codecov Report

Merging #705 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
+ Coverage   89.54%   89.57%   +0.03%     
==========================================
  Files          66       66              
  Lines        3587     3588       +1     
==========================================
+ Hits         3212     3214       +2     
+ Misses        375      374       -1
Impacted Files Coverage Δ
src/attributes.cpp 98.4% <100%> (ø) ⬆️
R/Rcpp.package.skeleton.R 84.43% <100%> (+0.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8d1e93...7f872ca. Read the comment docs.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 2, 2017

Member

Not quite there yet.

After fresh install from this branch:

/tmp$ Rscript -e "Rcpp::Rcpp.package.skeleton('foo', module = TRUE)"  
Creating directories ...
Creating DESCRIPTION ...
Creating NAMESPACE ...
Creating Read-and-delete-me ...
Saving functions and data ...
Making help files ...
Done.
Further steps are described in './foo/Read-and-delete-me'.

Adding Rcpp settings
 >> added Imports: Rcpp
 >> added LinkingTo: Rcpp
 >> added useDynLib directive to NAMESPACE
 >> added import(methods, Rcpp) directive to NAMESPACE
 >> added example src file using Rcpp attributes
 >> compiled Rcpp attributes
 >> do NOT modify by hand either RcppExports.cpp or RcppExports.R
 >> added Rd file for rcpp_hello_world
 >> copied the example module file 
/tmp$ R CMD INSTALL foo
* installing to library ‘/usr/local/lib/R/site-library’
* installing *source* package ‘foo’ ...
** libs
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused -c Num.cpp -o Num.o
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused -c RcppExports.cpp -o RcppExports.o
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused -c rcpp_hello_world.cpp -o rcpp_hello_world.o
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused -c rcpp_module.cpp -o rcpp_module.o
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused -c stdVector.cpp -o stdVector.o
g++ -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -Wl,-z,relro -o foo.so Num.o RcppExports.o rcpp_hello_world.o rcpp_module.o stdVector.o -L/usr/lib/R/lib -lR
installing to /usr/local/lib/R/site-library/foo/libs
** R
** preparing package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded
Error: package or namespace load failed for ‘foo’ in .doLoadActions(where, attach):
 error in load action .__A__.1 for package foo: loadModule(module = "NumEx", what = TRUE, env = ns, loadNow = TRUE): Unable to load module "NumEx": Failed to initialize module pointer: Error in FUN(X[[i]], ...): no such symbol _rcpp_module_boot_NumEx in package foo

Error: loading failed
Execution halted
ERROR: loading failed
* removing ‘/usr/local/lib/R/site-library/foo’
/tmp$ 
Member

eddelbuettel commented Jun 2, 2017

Not quite there yet.

After fresh install from this branch:

/tmp$ Rscript -e "Rcpp::Rcpp.package.skeleton('foo', module = TRUE)"  
Creating directories ...
Creating DESCRIPTION ...
Creating NAMESPACE ...
Creating Read-and-delete-me ...
Saving functions and data ...
Making help files ...
Done.
Further steps are described in './foo/Read-and-delete-me'.

Adding Rcpp settings
 >> added Imports: Rcpp
 >> added LinkingTo: Rcpp
 >> added useDynLib directive to NAMESPACE
 >> added import(methods, Rcpp) directive to NAMESPACE
 >> added example src file using Rcpp attributes
 >> compiled Rcpp attributes
 >> do NOT modify by hand either RcppExports.cpp or RcppExports.R
 >> added Rd file for rcpp_hello_world
 >> copied the example module file 
/tmp$ R CMD INSTALL foo
* installing to library ‘/usr/local/lib/R/site-library’
* installing *source* package ‘foo’ ...
** libs
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused -c Num.cpp -o Num.o
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused -c RcppExports.cpp -o RcppExports.o
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused -c rcpp_hello_world.cpp -o rcpp_hello_world.o
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused -c rcpp_module.cpp -o rcpp_module.o
ccache g++ -I/usr/share/R/include -DNDEBUG  -I"/usr/local/lib/R/site-library/Rcpp/include"    -fpic  -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused -c stdVector.cpp -o stdVector.o
g++ -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -Wl,-z,relro -o foo.so Num.o RcppExports.o rcpp_hello_world.o rcpp_module.o stdVector.o -L/usr/lib/R/lib -lR
installing to /usr/local/lib/R/site-library/foo/libs
** R
** preparing package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded
Error: package or namespace load failed for ‘foo’ in .doLoadActions(where, attach):
 error in load action .__A__.1 for package foo: loadModule(module = "NumEx", what = TRUE, env = ns, loadNow = TRUE): Unable to load module "NumEx": Failed to initialize module pointer: Error in FUN(X[[i]], ...): no such symbol _rcpp_module_boot_NumEx in package foo

Error: loading failed
Execution halted
ERROR: loading failed
* removing ‘/usr/local/lib/R/site-library/foo’
/tmp$ 
@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Jun 2, 2017

Member
Member

jjallaire commented Jun 2, 2017

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Jun 2, 2017

Member
Member

jjallaire commented Jun 2, 2017

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 2, 2017

Member

Works like a charm now.

Member

eddelbuettel commented Jun 2, 2017

Works like a charm now.

@eddelbuettel eddelbuettel changed the title from Automatically register init functions for RcppModules to Automatically register init functions for RcppModules (closes #704) Jun 2, 2017

compileAttributes(root)
message(" >> compiled Rcpp attributes ")
}

This comment has been minimized.

@eddelbuettel

eddelbuettel Jun 2, 2017

Member

Maybe I am short of coffee but if (attributes) { relies on attributes==TRUE. We could have a case of modules=TRUE (as in #704) and also attributes=FALSE, no? Needless complication?

@eddelbuettel

eddelbuettel Jun 2, 2017

Member

Maybe I am short of coffee but if (attributes) { relies on attributes==TRUE. We could have a case of modules=TRUE (as in #704) and also attributes=FALSE, no? Needless complication?

@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Jun 2, 2017

Member

I think it's a little more involved than first appears, if we do this:

Rcpp::Rcpp.package.skeleton('foo', module = TRUE, attributes = FALSE)

Then the attributes = FALSE causes us to generate init.c (which will in turn cause compileAttributes to do no registration). So calling compileAttributes in that case won't actually change the output, but rather will generate an RcppExports.cpp that has nothing in it.

I'd say if the user asks for modules and no attributes then they mean no auto-generation behavior and they'll futz with init.c manually themselves.

Member

jjallaire commented Jun 2, 2017

I think it's a little more involved than first appears, if we do this:

Rcpp::Rcpp.package.skeleton('foo', module = TRUE, attributes = FALSE)

Then the attributes = FALSE causes us to generate init.c (which will in turn cause compileAttributes to do no registration). So calling compileAttributes in that case won't actually change the output, but rather will generate an RcppExports.cpp that has nothing in it.

I'd say if the user asks for modules and no attributes then they mean no auto-generation behavior and they'll futz with init.c manually themselves.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 2, 2017

Member

Agreed. Let's go with this now and clean up spillage should it occur. Rcpp.package.skeleton() is already used less than it used to given the 'make me a package' feature in a certain IDE. And use of modules is even less common. And modules with attributes=FALSE ... may just never happen.

Member

eddelbuettel commented Jun 2, 2017

Agreed. Let's go with this now and clean up spillage should it occur. Rcpp.package.skeleton() is already used less than it used to given the 'make me a package' feature in a certain IDE. And use of modules is even less common. And modules with attributes=FALSE ... may just never happen.

@eddelbuettel eddelbuettel merged commit c73730a into master Jun 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@BenoitLondon

This comment has been minimized.

Show comment
Hide comment
@BenoitLondon

BenoitLondon Jun 7, 2017

Thanks for the fix! Do you know when you will release it? I wanted to know if I should use master or wait for the release.

BenoitLondon commented Jun 7, 2017

Thanks for the fix! Do you know when you will release it? I wanted to know if I should use master or wait for the release.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 7, 2017

Member

@BenoitLondon See my recent post to the rcpp-devel list with pointers to the drat repo. You can install Rcpp 0.12.11.2 from there. (Master works too but the git repo could be unstable.)

Member

eddelbuettel commented Jun 7, 2017

@BenoitLondon See my recent post to the rcpp-devel list with pointers to the drat repo. You can install Rcpp 0.12.11.2 from there. (Master works too but the git repo could be unstable.)

@BenoitLondon

This comment has been minimized.

Show comment
Hide comment
@BenoitLondon

BenoitLondon commented Jun 8, 2017

Thanks

@eddelbuettel eddelbuettel deleted the feature/module-routine-registraiton branch Jun 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment