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

new R warnings re: registration of symbols #636

Closed
kevinushey opened this issue Jan 17, 2017 · 34 comments
Closed

new R warnings re: registration of symbols #636

kevinushey opened this issue Jan 17, 2017 · 34 comments

Comments

@kevinushey
Copy link
Contributor

cc: wch/r-source@2f9cf02

R will now warn during R CMD check when symbols are not explicitly registered with dynamic lookup disabled. This implies that any R package using Rcpp::compileAttributes() will be getting these warnings. From anytime:

* checking compiled code ... NOTE
File ‘anytime/libs/anytime.so’:
  Found no calls to: ‘R_registerRoutines’, ‘R_useDynamicSymbols’

It is good practice to use registered native symbols and to disable
symbol search.

See ‘Writing portable packages’ in the ‘Writing R Extensions’ manual.

It looks like the expectation is that this will be on for CRAN submissions, so we might have to take the time to update compileAttributes() to handle manually generating the registration code.

@eddelbuettel
Copy link
Member

eddelbuettel commented Jan 17, 2017

Ewww.

I noticed that, but then I also noticed this today: wch/r-source@db321a9

Did you use r-devel as of yesterday / earlier or current? With

edd@max:~$ RD --version | head -1
R Under development (unstable) (2017-01-16 r71994) -- "Unsuffered Consequences" 
edd@max:~$

I get a clean check -- no issues for anytime_0.2.0.tar.gz under r-devel. Pheww

We may want to keep an eye on it. Then again, we may want to make registration better anyway (time permitting...)

@kevinushey
Copy link
Contributor Author

It's disabled temporarily; effectively until the documentation necessary is available in the R manuals:

wch/r-source@ea54ad8

So I think this will likely be affecting us (and client Rcpp packages) soon.

@kevinushey
Copy link
Contributor Author

I have a demo implementation (pretty much standalone) here:

https://github.com/kevinushey/sourcetools/blob/master/R/register.R

We might want to cannibalize some of this code and integrate it with compileAttributes().

@eddelbuettel
Copy link
Member

Lovely stuff. I keep noticing the various incremental commit which BDR put into R-devel, but have not had time to look at this. Automating the registration via compileAttributes() would appear to be the thing to do.

@jjallaire
Copy link
Member

jjallaire commented Feb 6, 2017 via email

@eddelbuettel
Copy link
Member

I am set up for reverse depends checks over CRAN. Given other lists of packages (i.e. if we bothered to systematically search GitHub, say) we could do that too.

And it's all for a good cause. Omelettes and breaking eggs comes to mind.

@jjallaire
Copy link
Member

If there were only a couple of packages we end up breaking we could to this:

  1. Generate the code as in Kevin's prototype

  2. Asking the users whose packages are broken to provide some "by convention" means of providing an additional initialization function which we would run from our generated code.

@eddelbuettel
Copy link
Member

Mind you, normal package builds do not tickle new runs of compileAttributes(). So we'd have to rig up a new best bed anyway. No hurry.

We could first test this with our own packages.

@kevinushey
Copy link
Contributor Author

kevinushey commented Feb 6, 2017

I agree that we want to make sure we never break packages (especially for packages that call compileAttributes() on every build since that will be exceedingly annoying for developers)

Perhaps we could compromise with a solution like the following:

  1. Emit the registration code to the tail end of RcppExports.cpp -- that is, implement a function there (with C linkage) called e.g. Rcpp_init_pkgname. This function can return an array of CallMethodDefs, or something similar.

  2. Emit an R_init_pkg.c file if none already exists / we don't detect a call to R_init_pkg somewhere in the package sources. This file will take care of calling Rcpp_init_pkgname and passing the R call method definitions to R's registration routines. If that file does exist and doesn't call our Rcpp initialization routine, then we'll notify the user with a message or something similar.

I do think that it will be relatively rare for Rcpp users to also have their own unique registration code. I imagine most Rcpp users who are registering routines are just doing something less automated to accomplish what we're setting out to do right now; it's unlikely many users are mixing e.g. .C, .External and .Fortran interface calling functions. That said, we should be prepared to accommodate those users as well.

@EricArcher
Copy link

I have just received this NOTE on a submission to CRAN although it did not show up when my package was checked with win-builder (this has been changed and does now).

* checking compiled code ... NOTE
File 'strataG/libs/i386/strataG.dll':
  Found no calls to: 'R_registerRoutines', 'R_useDynamicSymbols'
File 'strataG/libs/x64/strataG.dll':
  Found no calls to: 'R_registerRoutines', 'R_useDynamicSymbols'

It is good practice to register native routines and to disable symbol
search.

See 'Writing portable packages' in the 'Writing R Extensions' manual.

Unfortunately, after reading the section referenced in "Writing R Extensions" and the thread here, I am no more enlightened as to what is triggering this NOTE or how to repair it for my package to be accepted.

Is there something else I can read that would help or is someone able to guide me as to what I should include?

Thanks in advance.

@eddelbuettel
Copy link
Member

eddelbuettel commented Feb 16, 2017

You need to read the documentation from "r-devel", ie R 3.4.0 -- which become "r-release" around April (though no date has been set). Try eg here.

@andyyao95
Copy link

Hi kevin @kevinushey. My package has the same problem (i.e. Found no calls to: 'R_registerRoutines', 'R_useDynamicSymbols' ). I didn't understand how to fix this from the r-devel documentation. Could you let me know specifically what should be done to bypass this NOTE?

Thanks a ton,
Andy

@eddelbuettel
Copy link
Member

What part of the previous message (ie: read "r-devel" documentation) was unclear?

@andyyao95
Copy link

@eddelbuettel As I said, "I didn't understand how to fix this from the r-devel documentation.", so I was hoping if Rcpp could provide some guidance. I'm only a user of Rcpp so I don't know what is causing the NOTE at the C level.

@eddelbuettel
Copy link
Member

eddelbuettel commented Feb 20, 2017

What makes you think our role is tutoring you to read documentation that other people have written? Complaining to us is like complaining to the makers of your editor, or OS, or whatnot. Misplaced.

You wrote an R package. R updated its requirements. Read the R docs.

We're all volunteers here too, and asking us to read them aloud to you is ... a little weird.

@eddelbuettel
Copy link
Member

We will get to this eventually. But we're all busy, and we do this as side-jobs and favours to the world at large. So have them patience.

Or, ideally, do some work to actually help us.

@EricArcher
Copy link

As both a user of RCpp and a developer of packages that get used by people with a variety of backgrounds, I deeply appreciate the effort and work you and your co-authors put into maintaining it and helping out such a large community.

I did not get the impression that @andyyao95 was asking you to read the r-devel documentation to him. He said that he had read it and did not understand it. I too have read it and have not fully understood what I need to do to be in compliance with the CRAN requirements. Because I have not found anyone to explain it to me, I am googling and testing and learning by trial and error. Perhaps I will narrow it down and find a resolution.

We are all overworked and do much of this community coding as a labour of love. I try to remember that none of us were born with knowledge and we all had to learn some way. Sometimes reading clarifies things. When it doesn't, some of us are lucky enough to have knowledgeable people around who can help explain difficult concepts in a way that the text cannot. I am sure you too have had this experience in your career and been on our side of the ignorance wall.

I sincerely hope this doesn't offend you, however being in the same place as @andyyao95, I thought it might be helpful to try to explain the perspective of one who tries to solve a problem as much as possible before taking others time to seek help, but still needs to seek help from time to time.

@kevinushey
Copy link
Contributor Author

kevinushey commented Feb 20, 2017

The tl;dr version of this -- you must now define a routine in your package called R_init_<pkg> which takes care of registering symbols, as described in https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#Registering-native-routines.

Since examples are much easier to digest than R's documentation, the idea is that you (at least, right now) need to manually generate a file similar to this: https://github.com/kevinushey/sourcetools/blob/master/src/sourcetools_init.c

FWIW, it's likely sufficient to just add this to your package somewhere:

void R_init_<pkg>(DllInfo* info) {
	R_registerRoutines(info, NULL, NULL, NULL, NULL);
	R_useDynamicSymbols(info, TRUE);
}

replacing <pkg> with the name of your package. (That would, in theory, silence the R CMD check warnings, although I have not yet tested.) Note that for now dynamics symbols are required as the R-level interface generated by Rcpp relies on this.

It would also be necessary to add .registration = TRUE in the useDynLib() call in the NAMESPACE file, e.g. as in https://github.com/kevinushey/sourcetools/blob/master/NAMESPACE#L13.

In the (hopefully near-ish) future, Rcpp will automatically take care of generating this code for you as part of the compileAttributes() step; unfortunately, the Rcpp team hasn't had time to take on this work just yet.

@kevinushey
Copy link
Contributor Author

kevinushey commented Feb 20, 2017

It's also worth examining how Rcpp registers its own routines:

https://github.com/RcppCore/Rcpp/blob/master/src/Rcpp_init.cpp

although the logic there is somewhat more complicated as Rcpp does a bit more work.

@spedygiorgio
Copy link

Hi All, I tried to follow @kevinushey suggestion by:

  1. creating a C file in my scr/ section of my package markovchain, named registerDynamicSymbols.c
  2. pushing the following code:
// RegisteringDynamic Symbols

#include <R.h>
#include <Rinternals.h>
#include <R_ext/Rdynload.h>

void R_init_markovchain(DllInfo* info) {
  R_registerRoutines(info, NULL, NULL, NULL, NULL);
  R_useDynamicSymbols(info, TRUE);
}

I then tested it with winbuilder but still NOTES are raised:

File 'markovchain/libs/i386/markovchain.dll':
Found no calls to: 'R_registerRoutines', 'R_useDynamicSymbols'
File 'markovchain/libs/x64/markovchain.dll':
Found no calls to: 'R_registerRoutines', 'R_useDynamicSymbols'

I hope that forthoming version of Rcpp could help on this.

@eddelbuettel
Copy link
Member

eddelbuettel commented Feb 20, 2017

@spedygiorgio It really is not all that complicated. Here are the steps:

  1. Get yourself r-devel. Just build it, or use a Docker instance, or ...
  2. Run it on a package. Eg I just did for (the GitHub version of) RcppArmadillo as I knew I had not (yet) registered its fastLm() function. And surely, RD CMD check (using r-devel) warns.
  3. The key: be lazy and just call tools::package_native_routine_registration_skeleton(".") from r-devel as has been suggested before. In my case it identifies four entry points, and sets up the file. I copied that to src/init.c.
  4. Rebuild, re-check. Worst case you need to also adapt NAMESPACE.

@kevinushey
Copy link
Contributor Author

@spedygiorgio FWIW I recall reading on R-devel that winbuilder has had some problems related to this check; if things look okay locally but fail on winbuilder in this specific case you may be fine.

@EricArcher
Copy link

I brought the fact that winbuilder does not catch this check to the attention of Uwe Ligges last week and he modified it. It now throws the NOTE.

@eddelbuettel
Copy link
Member

For what it is worth my now-updated RcppArmadillo passes cleanly here (with a just-updated build of R-devel) and also on win-builder (which took a jolly long time to reply, well over an hour).

@spedygiorgio
Copy link

Seems that on my packages things worked worse. As suggested by @eddelbuettel I moved to the R Devel, then I ran tools::package_native_routine_registration_skeleton(".") and pasted the output into a init.c. And also, I included the , .registration = TRUE within my useDynLib(markovchain).

Not only the error " Found no calls to: 'R_registerRoutines', 'R_useDynamicSymbols'" keep to be shown but I also receive a bunch of "undocumented code object".

@eddelbuettel
Copy link
Member

Sorry, but that just seems to indicate that you need to work on your package. Maybe the r-help or r-package-devel lists can help you with that,

There never was an Rcpp issue here, so could we maybe stop this off-topic thread here?

@digEmAll
Copy link

digEmAll commented Feb 20, 2017

I had the same problems of @spedygiorgio, here's how I fixed :

  1. I did what Dirk suggested 2 posts above (using tools::package_native... etc..)
  2. at this point all my Rcpp functions became visible and consequently "undocumented"
  3. I changed NAMESPACE file by deleting the line exportPattern("^[[:alpha:]]+") line (added by Rcpp skeleton function) and replaced by explicit export directives : export(functionToMakeVisible1,functionToMakeVisible2,...) (this solves the undocumented issue)
  4. CMD check --as-cran using R-devel on Windows still gave the Found no calls to: 'R_registerRoutines' error but only on x64 architecture
  5. I tested on https://win-builder.r-project.org/ and went OK, so I suspect there's something wrong/different with local and win-builder builds

@spedygiorgio
Copy link

spedygiorgio commented Feb 20, 2017

@digEmAll thanks for the suggestion, I will try it!
I would also to add that a part of this problem is that tools::package_native_routine_registration_skeleton(".") could not work well when in Rcpp export directives are "." (to make the function hidden)...
@eddelbuettel , certainly I have to work on my package but consider that all my external code has been always written in Rcpp... And before R 3.4 all worked properly... So I believe it is an Rcpp issue... Not for Rcpp team faults, but for CRAN architecture revisions...

@eddelbuettel
Copy link
Member

@digEmAll: The registration only covers the R-to-C(++) call (as far as I understand it) and should have no bearing on what is exported, and what needs documentation.

@kevinushey
Copy link
Contributor Author

kevinushey commented Feb 20, 2017

The issue is likely the presence of e.g.

exportPattern(<everything>)

in the NAMESPACE file -- this will inadvertently export the native symbol objects, which is likely not intended by most users.

And it's definitely not Rcpp's fault if we don't work with new R-devel routines that nobody told us were coming ...

@digEmAll
Copy link

digEmAll commented Feb 20, 2017

@kevinushey :
yes, as far as I understood from -> here, you can replace the .Call("c_function",PACKAGE="package",arg1,arg2) with .Call(c_function,arg1,arg2) (note the c_function with no quotes) so probably the symbol c_function is made available in the package and exportPattern(<permissiveRegexp>) will make it available for the rest of the world as well.

BTW, it seems that modification can give some speed improvements. Maybe it could be added in the next Rcpp compileAttributes version :)

@openpaul
Copy link

openpaul commented Feb 22, 2017

I managed to compile my package and removing the warning under ubuntu, which does not have R-devel binaries, by just downloading R-devel source and compiling it (takes around 30 minutes, maybe less. I am on an fairly old PC).

Download here: https://cran.r-project.org/sources.html and unpack.

cd R-devel
./configure
make

Then doing this (quick and dirty):

cd 'package-dir'
~/R-devel/bin/R 

in the R console paste

tools::package_native_routine_registration_skeleton(".") 

and copy the resulting c ouput in a file under src. Also modify your NAMESPACE file to include this line useDynLib(packagename, .registration = TRUE).

Then build your package as your used to. This fixed the problem for my package (see commit hildebra/Rarefaction@33896d9).

@eddelbuettel
Copy link
Member

eddelbuettel commented Feb 22, 2017

  1. You made your life too difficult.
  2. You don't need to build R-devel to access the package_native_routine_registration_skeleton() helper. As discussed on the mailing lists, you can just fetch the function from the R-devel sources.
  3. We do provide Docker containers with r-devel pre-built. Running Docker on Ubuntu is trivial.
  4. I have explained numerous times how to build r-devel well; the Dockerfiles are one example.
  5. I am glad the issue is fixed for you too; it is now still not an Rcpp issue so I am now locking this thread.

@RcppCore RcppCore locked and limited conversation to collaborators Feb 22, 2017
@eddelbuettel
Copy link
Member

Closing this as it turned into a "R packaging help" issue for which this is not the right forum.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants