sneumann / mzR

This is the git repository matching the Bioconductor package mzR: parser for netCDF, mzXML, mzData and mzML files (mass spectrometry data)
42 stars 26 forks source link

Your `configure` assumes `R --vanilla` to find Rccp #212

Open mmaechler opened 4 years ago

mmaechler commented 4 years ago

I cannot install mzR at all (well, I can if I start modifiyng its source before installing...). Unfortunately, at the very end, when all the long C++ compilations are finished, the installation fails with

** byte-compile and prepare package for lazy loading
Error : invalid version specification ‘’
Error: unable to load R code in package ‘mzR’
Execution halted
ERROR: lazy loading failed for package ‘mzR’
* removing ‘/usr/local64.sfs/app/R/Bioconductor/library_3.10_F30/mzR’
Warning message:
In install.packages(...) :
  installation of package ‘mzR’ had non-zero exit status
> 

and this was not so easy to debug.... till I had the idea to look into your mzR/configure file, and then saw the line

BUILT_RCPP_VERSION=`"${RBIN}" --vanilla --slave -q -e "cat(as.character(packageVersion('Rcpp')))"`

This of course can only work for people who install everything in the standard R library... which is actually not recommended by many experts, but maybe is typical for bioconductor users and setups ?
Here, we never install packages into R's own .Library but rather use an R_PROFILE setting which sets up a .libPaths() of several entries. I know that this is also true for all debian/ubuntu (packaged) installation of R, but there it does not happen via R_PROFILE and hence '--vanilla' may not suppress the finding of the extra .libPaths entries there ...

I think Dirk (Rcpp maintainer) @eddelbuettel will have better advice on what do here.

mmaechler commented 4 years ago

What I say above was too extreme: Of course, R --vanilla can also find Rcpp in another library (than R's .Library) if the users set R_LIBS correspondingly. But setting these is not at all the only way to setup an extended .libPaths() ...

eddelbuettel commented 4 years ago

I think Dirk (Rcpp maintainer) @eddelbuettel will have better advice on what do here.

No, sorry, I don't. I cam only put my extra $0.02 is that messing with ~/.Rprofile is generally a bad idea, as is assuming it would not print etc pp.

As for changing .libPaths / having system wide defaults: Yes. Debian/Ubuntu do that since 2004 (!!) when Kurt and Fritz convinced me of the merit of the particular scheme we still use. But that scheme does not depend on ~/.Rprofile:

edd@rob:~$ R --slave --vanilla -e 'print(.libPaths())'
[1] "/usr/local/lib/R/site-library" "/usr/lib/R/site-library"      
[3] "/usr/lib/R/library"           
edd@rob:~$ 

Edit: But to be clear, I fully agree with @mmaechler that relying on / messing with what what comes from R via

BUILT_RCPP_VERSION=`"${RBIN}" --vanilla --slave \
      -q -e "cat(as.character(packageVersion('Rcpp')))"`

seems like a bad idea. While you could ask the R selected by the current path about installed.packages() but even then answer is not clear. Rcpp itself does not "export" its version number or suggest or document any inference on building based on it. We just trust R, and generally assume the user knows when to rebuild as changes (in the installed compiler, say) are well beyond our control or knowledge.

mmaechler commented 4 years ago

Thank you, Dirk, for the info.
Note I did NOT say nor mean ~/.Rprofile but rather the R_PROFILE environment variable which you can easily set (to the same file) within an organization, and everybody in that organization gets the same setup (which BTW is much more flexible than e.g. using R_LIBS)

sneumann commented 4 years ago

Jez, that's ancient code. I think it came in to write the Rcpp version that was used when compiling/installing the package into https://github.com/sneumann/mzR/blob/master/R/zzz.R#L1 and subsequently emit a warning upon package loading in https://github.com/sneumann/mzR/blob/master/R/zzz.R.in#L8 if the user has installed a newer / incompatible Rcpp since that installation. IIRC that was catching some incompatibilities, but I am unsure if that is still required in 2019/2020 . Yours, Steffen

lgatto commented 4 years ago

Yes, the goal here was to catch the installed version on the Bioconductor server, and that line did the job on that system - apologies for missing/messing others.

@sneumann - we haven't seen these incompatibilities between Rcpp version on Windows in years. Maybe we can drop it?