Path quoting in CxxFlags with bash

[llaniewski]

Hi. In relation to the discussion in #1188 I just wanted to mention an issue introduced there.

Problem

Adding quotes around path in #1189 broke compilation for anybody who was using CxxFlags in simple compilation script like:

CXXFLAGS="$(R CMD config --cppflags)"
CXXFLAGS="$CXXFLAGS $(R --vanilla --slave -e "Rcpp:::CxxFlags()")"
g++ $CXXFLAGS -c main.cpp

This is because bash will use exactly what is returned by CxxFlags as argument for the compiler (g++). This means that the compiler gets -I"/some/path" and searches for headers in "/some/path" rather then /some/path.

Other use cases

• You can resolve the quotes in bash by doing eval g++ ... in the script, but one cannot modify the all the compiler calls to use "eval" in eg. ./configure scripts from autoconf.
• If one uses the CxxFlags() in GNU make, it works. This is because make resolves the quotes. For example, the compilation of RInside examples (example/standard) works fine.
• On the other hand, it seems to break cmake-based compilation. When I try to compile the same examples using cmake 3.16.3 for configuration, it fails with the error. Apparently passing path with quotes to include_directories() through a variable causes some weird behaviour.

Questions

• If anybody has a good idea how to fix this without hurting windows users and their beloved spaces, I would be happy to implement it and make a pull request. The only half-decent workaround I can think of now is quoting path if it has a space.
• Is there a better way to do get include paths from Rcpp, which would work with eg. autoconf?

Notes

We run into this problem, as we use RInside in our C++ code. We currently use a workaround of stripping quotes.

The main.cpp file can be as simple as:

#include <Rcpp.h>
int main () {
    return 0;
}

I'm not attaching sessionInfo() as this issue is not about R.

@eddelbuettel Do you think it is an issue at all and would you expect such a bash script to work? If not, I you can close the issue, and we'll stick with our workaround.

[eddelbuettel]

This is such a well-reasoned and written issue! We should definitely help, and it once again shows the risk of touching / changing old statements and code 😢

I like your 'change only if space found'. So the idea is that we would wrap a function around that will in most cases do nothing but if on that forlorn OS and a space is found makes the change? Sounds good to me.

[llaniewski]

Hi. After some investigation I ended up with the following regexp pattern to catch paths that do not need quoting:

quoteNonStandard = function(path, quoteAll=FALSE) {
        quoted = shQuote(path)
        if (quoteAll) {
                quoted
        } else {
                sel = grepl("^[[:alnum:]/._~+@%-]*$", path)
                ifelse(sel, path, quoted)
        }
}

I used shQuote in place of just paste0('"',path,'"') to make it more consistent with the rest of the code. I'll try to test this with stuff described in #1188 and test if everything works in Windows and Linux (don't own a Mac). If this looks good I'll submit a pull request.

[eddelbuettel]

(You could do ifelse( !sel || quoteAll, quoted, path) to make it bit more compact..)

That would need some code comments to make it comprehensible for humans 😄 Why the regexp? And is that stable across windows versions?

(Lastly, for style consistency in the repo, please use <- for assignment.)

Otherwise, all good. The January update just shipped as Rcpp 1.0.10, so we have quite some time til July to test this ...

[llaniewski]

I opened the pull request #1243

Why the regexp? I think matching a reasonable set of "legal" characters is safer than trying to make an exhaustive list of "illegal" characters (like spaces). This way the code will fall back to quoting for any non-standard characters in path.

As per your previous suggestion, I changed it to only kick in on unix-type systems.

[eddelbuettel]

Yes by glancing at it a little longer I started to understand the regexp. It's good! I made a small suggestion for the function. We could also leave the argument empty and test if missing(arg)) arg = sys != unix (I paraphrase) but it's all details.