CxxFlags (and friends) should quote their library paths on Unix

[kevinushey]

Originally filed from here: rstudio/renv#862

The ultimate issue is that Rcpp:::CxxFlags() doesn't quote the include path used, so packages installed into a path containing spaces (I know, I know...) won't be propagated to the compiler correctly. For example:

# use library path with spaces
libpath <- tempfile("Has Spaces")
dir.create(libpath)
.libPaths(libpath)

# install Rcpp
install.packages("Rcpp")

# ask it for CxxFlags
Rcpp:::CxxFlags()

gives me, on macOS:

> Rcpp:::CxxFlags()
-I/private/var/folders/b4/2422hswx71z8mgwtv4rhxchr0000gn/T/RtmpyPtnPl/Has Spaces82462dc1ebe6/Rcpp/include

Note that the include path is not quoted, which implies that installing other packages from sources which try to use Rcpp:::CxxFlags() will fail. For example, with install.packages("scrypt", type = "source"):

< ... >
clang++ -mmacosx-version-min=10.13 -std=gnu++14 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG  -I'/private/var/folders/b4/2422hswx71z8mgwtv4rhxchr0000gn/T/RtmpyPtnPl/Has Spaces82462dc1ebe6/Rcpp/include' -I/usr/local/include  `/Library/Frameworks/R.framework/Resources/bin/Rscript -e "Rcpp:::CxxFlags()"` -I./scrypt-1.1.6/ -I./scrypt-1.1.6/lib -I./scrypt-1.1.6/lib/util -DHAVE_CONFIG_H -fPIC  -g -O3 -pipe -Wall -pedantic -c RcppExports.cpp -o RcppExports.o
clang: error: no such file or directory: 'Spaces82462dc1ebe6/Rcpp/include'
make: *** [RcppExports.o] Error 1
ERROR: compilation failed for package ‘scrypt’
* removing ‘/private/var/folders/b4/2422hswx71z8mgwtv4rhxchr0000gn/T/RtmpyPtnPl/Has Spaces82462dc1ebe6/scrypt’
Warning in install.packages :
  installation of package 'scrypt' had non-zero exit status

The scrypt package has this in its Makevars:

PKG_CXXFLAGS = `$(R_HOME)/bin/Rscript -e "Rcpp:::CxxFlags()"` $(SCRYPT_FLAGS)

I can't see where Rcpp officially documents how CxxFlags() should be used, other than some examples via environment variables in e.g.

inst/examples/ConvolveBenchmarks/overhead.sh
10:export PKG_CPPFLAGS=`Rscript -e "Rcpp:::CxxFlags()"`

inst/examples/ConvolveBenchmarks/buildAndRun.sh
11:export PKG_CPPFLAGS=`Rscript -e "Rcpp:::CxxFlags()"`

but I think this is the correct way to invoke the function (that is, the onus is not on the user to try and quote whatever is returned by Rcpp:::CxxFlags()).

Rcpp has a function called asBuildPath(), which is intended to create paths that are more easily consumed by the compiler / linker. It is defined here:

Rcpp/R/tools.R

Lines 35 to 50 in 2868f38

	# Transform a path for passing to the build system on the command line.
	# Leave paths alone for posix. For Windows, mirror the behavior of the
	# R package build system by starting with the fully resolved absolute path,
	# transforming it to a short path name if it contains spaces, and then
	# converting backslashes to forward slashes
	asBuildPath <- function(path) {
	if (.Platform$OS.type == "windows") {
	path <- normalizePath(path) # #nocov start
	if (grepl(' ', path, fixed=TRUE))
	path <- utils::shortPathName(path)
	path <- gsub("\\\\", "/", path) # #nocov end
	}
	return(path)
	}

We use that, and quote the returned path in a number of spots, e.g. (note the use of " around the path)

Rcpp/R/Attributes.R

Lines 869 to 880 in 2868f38

	# Build CLINK_CPPFLAGS from include directories of LinkingTo packages
	.buildClinkCppFlags <- function(linkingToPackages) {
	pkgCxxFlags <- NULL
	for (package in linkingToPackages) {
	packagePath <- find.package(package, NULL, quiet=TRUE)
	packagePath <- asBuildPath(packagePath)
	pkgCxxFlags <- paste(pkgCxxFlags,
	paste0('-I"', packagePath, '/include"'),
	collapse=" ")
	}
	return (pkgCxxFlags)
	}

But we don't quote it here:

Rcpp/R/RcppLdpath.R

Lines 48 to 56 in 2868f38

	## Provide compiler flags -- i.e. -I/path/to/Rcpp.h
	RcppCxxFlags <- function(cxx0x=FALSE) {
	# path <- RcppLdPath()
	path <- Rcpp.system.file( "include" )
	if (.Platform$OS.type=="windows") {
	path <- asBuildPath(path) # #nocov
	}
	paste("-I", path, if (cxx0x && canUseCXX0X()) " -std=c++0x" else "", sep="")
	}

My proposal is that we quote the path in the above routine.

[eddelbuettel]

I was ready to bet that we protected just about every possible path (because we relied so heavily on this with inline and when we still needed linking). And we fought this many times in the past years, this got added / modded / ...

So I would strongly urge packages like scrypt to get their act together and join the second decade of the 21st century and use just LinkingTo:. It has been supported since 2013 or so -- and effectively been the default that long too.

We somewhat actively discourage the LDFLAGS equivalent to the one you dug up.

I think I once looked into putting a .deprecated tag into these but it made too much noise.

[kevinushey]

So I would strongly urge packages like scrypt to get their act together and join the second decade of the 21st century and use just LinkingTo:. It has been supported since 2013 or so -- and effectively been the default that long too.

That's a good point -- and rscrypt already has LinkingTo: Rcpp anyway, so the use of Rcpp:::CxxFlags() seems redundant.

[eddelbuettel]

I have no problem with us being good samaritans and offer a helping hand so the PR idea is still good ... but some packages should also face reality. It is 2021. We have not needed CxxFlags since 2013. It persists because it got copied out of skeleton packages and of course older tutorials so we're not turning it off or removing it (unlike <cough, cough> some other frameworks) but it really should be changed at their end.

Now, your renv pain is real too and there is no need for added failures so we can (and probably should...) fix this.

PS Also, I probably meant third decade. Programmers and one-off errors and all that...