avoid including empty string in R CMD SHLIB call

[kevinushey]

Pull Request Template for Rcpp

Closes #1061; ensures that we only include non-empty elements when constructing the command string.

In particular, shQuote(character()) does return an empty quoted string on Windows:

> shQuote(character())
[1] "\"\""

so we need to check the length of the option 'dependencies' vector before including it in the generated command.

Checklist

• Code compiles correctly
• R CMD check still passes all tests
• Prefereably, new tests were added which fail without the change
• Document the changes by file in ChangeLog

[codecov-io]

Codecov Report

Merging #1062 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1062   +/-   ##
=======================================
  Coverage   95.60%   95.61%           
=======================================
  Files          64       64           
  Lines        2778     2782    +4     
=======================================
+ Hits         2656     2660    +4     
  Misses        122      122           

Impacted Files	Coverage Δ
R/Attributes.R	99.66% <100.00%> (+<0.01%)	⬆️

[kevinushey]

R CMD check had a couple hiccups locally, but seem unrelated:

** running tests for arch 'i386' ...
  Running 'tinytest.R'
 ERROR
Running the tests in 'tests/tinytest.R' failed.
Last 13 lines of output:
   call| expect_equal(Datetime_format(d, "%Y/%m/%d %H:%M:%S"), format(d,
   call| "%Y/%m/%d %H:%M:%OS"), info = "Datetime.formating.given.format")
   diff| Expected '2016/12/13 14:15:16.123456', got '2016/12/13 22:15:16.123456'
   info| Datetime.formating.given.format
  ----- FAILED[data]: test_date.R<199--201>
   call| expect_equal(Datetime_ostream(d), format(d, "%Y-%m-%d %H:%M:%OS"),
   call| info = "Datetime.formating.ostream")
   diff| Expected '2016-12-13 14:15:16.123456', got '2016-12-13 22:15:16.123456'
   info| Datetime.formating.ostream
  ----- FAILED[data]: test_exceptions.R<50--50>
   call| expect_true(!is.null(condition$cppstack))
   diff| Expected TRUE, got
  Execution halted
  Warning message:
  In dir.create("templib") : 'templib' already exists
** running tests for arch 'x64' ...
  Running 'tinytest.R'
 ERROR
Running the tests in 'tests/tinytest.R' failed.
Last 13 lines of output:
   call| expect_equal(Datetime_format(d, "%Y/%m/%d %H:%M:%S"), format(d,
   call| "%Y/%m/%d %H:%M:%OS"), info = "Datetime.formating.given.format")
   diff| Expected '2016/12/13 14:15:16.123456', got '2016/12/13 22:15:16.123456'
   info| Datetime.formating.given.format
  ----- FAILED[data]: test_date.R<199--201>
   call| expect_equal(Datetime_ostream(d), format(d, "%Y-%m-%d %H:%M:%OS"),
   call| info = "Datetime.formating.ostream")
   diff| Expected '2016-12-13 14:15:16.123456', got '2016-12-13 22:15:16.123456'
   info| Datetime.formating.ostream
  ----- FAILED[data]: test_exceptions.R<50--50>
   call| expect_true(!is.null(condition$cppstack))
   diff| Expected TRUE, got
  Execution halted
  Warning message:
  In dir.create("templib") : 'templib' already exists

Let me know if it's worth digging into these. The Datetime errors appear to be timezone-related; the cppstack bit should likely be ignored on Windows (?)

[eddelbuettel]

Had you ever had TZ issues before? That seems weird, and unrelated. Ditto for the exceptions issue though could that be related to the recent cleanup for messy PR that got us into the spot of bother that Rcpp 1.0.4 is in?

[jeroen]

I can confirm that this patch fixes the unit tests for both RcppArmadillo and RcppParallel on R 4.0. Without this test, both packages fail on R 4.0 on Windows, regardless of which toolchain is used.

Highly recommend a Rcpp patch release to CRAN.

ps: there are some known timezone issues in R on Windows if the server is set to UTC which R does not recognize. On appveyor and anywhere else we specifically set the timezone to something else: https://github.com/krlmlr/r-appveyor/blob/master/scripts/appveyor-tool.ps1#L160-L165

Proof of output (with the above Rcpp patch)

* using log directory 'C:/Users/jeroen/Documents/checks/RcppArmadillo.Rcheck'
* using R version 4.0.0 alpha (Rtools40) (2020-03-29 r78107)
* using platform: x86_64-w64-mingw32 (64-bit)
* using session charset: ISO8859-1
* checking for file 'RcppArmadillo/DESCRIPTION' ... OK
* checking extension type ... Package
* this is package 'RcppArmadillo' version '0.9.850.1.0'
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking whether package 'RcppArmadillo' can be installed ... OK
* checking installed package size ... NOTE
  installed size is  9.3Mb
  sub-directories of 1Mb or more:
    include   6.0Mb
    libs      2.3Mb
* checking package directory ... OK
* checking 'build' directory ... OK
* checking DESCRIPTION meta-information ... OK
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking R files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* loading checks for arch 'i386'
** checking whether the package can be loaded ... OK
** checking whether the package can be loaded with stated dependencies ... OK
** checking whether the package can be unloaded cleanly ... OK
** checking whether the namespace can be loaded with stated dependencies ... OK
** checking whether the namespace can be unloaded cleanly ... OK
** checking loading without being on the library search path ... OK
* loading checks for arch 'x64'
** checking whether the package can be loaded ... OK
** checking whether the package can be loaded with stated dependencies ... OK
** checking whether the package can be unloaded cleanly ... OK
** checking whether the namespace can be loaded with stated dependencies ... OK
** checking whether the namespace can be unloaded cleanly ... OK
** checking loading without being on the library search path ... OK
* checking dependencies in R code ... OK
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... OK
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... OK
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... OK
* checking line endings in shell scripts ... OK
* checking line endings in C/C++/Fortran sources/headers ... OK
* checking line endings in Makefiles ... OK
* checking compilation flags in Makevars ... OK
* checking for GNU extensions in Makefiles ... OK
* checking for portable use of $(BLAS_LIBS) and $(LAPACK_LIBS) ... OK
* checking use of PKG_*FLAGS in Makefiles ... OK
* checking compiled code ... NOTE
Warning in read_symbols_from_dll(so, rarch) :
  this requires 'objdump.exe' to be on the PATH
Warning in read_symbols_from_dll(so, rarch) :
  this requires 'objdump.exe' to be on the PATH
Warning in read_symbols_from_dll(so, rarch) :
  this requires 'objdump.exe' to be on the PATH
Warning in read_symbols_from_dll(so, rarch) :
  this requires 'objdump.exe' to be on the PATH


See 'Writing portable packages' in the 'Writing R Extensions' manual.
* checking installed files from 'inst/doc' ... OK
* checking files in 'vignettes' ... OK
* checking examples ...
** running examples for arch 'i386' ... OK
** running examples for arch 'x64' ... OK
* checking for unstated dependencies in 'tests' ... OK
* checking tests ...
** running tests for arch 'i386' ... OK
  Running 'tinytest.R'
** running tests for arch 'x64' ... OK
  Running 'tinytest.R'
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes in 'inst/doc' ... OK
* checking running R code from vignettes ... OK
  'RcppArmadillo-sparseMatrix.Rmd'... OK
  'RcppArmadillo-intro.Rnw'... OK
* checking re-building of vignette outputs ... OK
* checking PDF version of manual ... OK
* DONE
Status: 2 NOTEs

[eddelbuettel]

I can confirm that this patch fixes the unit tests for both RcppArmadillo and RcppParallel on R 4.0.[0]

Awesome, most helpful!

I did actually file a bug report against base R about RcppArmadillo failing to link -- which I attributed (wrongly) to LAPACK regressions and will report it there as well.

That makes it two reasons this should go in.

[jeroen]

Can you please close the r-base bug report, such that they don't use it as a blocker for the rtools40 toolchain? The bug really is in Rcpp, unfortunately.

[eddelbuettel]

I just commented there. Can you pipe in as well?

(And yes, thanks to you confirmation it very much looks like "us" (Rcpp) rather than "them" (base R). My bad in not seeing it earlier in the RcppArmadillo errors. But it is very likely the interaction of Rcpp with R 4.* that needs an updated -- as provided right here.)