Additional file path normalization to protect a windows path

[eddelbuettel]

Motivation

Issue #1234 demonstrated that we had a left-over non-normalized path, this PR addresses this.

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

[eddelbuettel]

While at it, do we need more mustWork = FALSE? I see recurrning warnings as the one in the #1234 discussion but that may not come from us.

[kevinushey]

I don't think this is the right fix. Here's what I see on my Windows VM:

> R.home("bin")
[1] "C:/PROGRA~2/R/R-42~1.1/bin/x64"
> normalizePath(R.home("bin"))
[1] "C:\\Program Files (x86)\\R\\R-4.2.1\\bin\\x64"

In other words, normalizePath() could convert a short path (which would be handled okay via system()) to a long path (which might have spaces and hence would not).

A cross-platform fix would be to use shQuote() on the executable path, or utils::shortPathName() on Windows to convert long paths to short paths. Or, use system2() and rely on R to properly quote the command / executable name.

[eddelbuettel]

Very good. Can you recurse into the discussion in #1234 and see with OP? What I suggested works for him there ...

[eddelbuettel]

"Empirically speaking" this is a rare case anyway as we had a bazillion uses of sourceCpp() on Windows since that line was written so it is a little puzzling anyway.

[eddelbuettel]

Here is the whole block (as I currently have i.e. with the PR). There is a shQuote already for the path, so what you suggest is good -- we can do it for binary too. Just puzzled it 'worked for him' over in #1234.

        # grab components we need to build command
        r <- normalizePath(file.path(R.home("bin"), "R"), winslash = "/", mustWork = FALSE)
        lib  <- context$dynlibFilename
        deps <- context$cppDependencySourcePaths
        src  <- context$cppSourceFilename

        # prepare the command (output if we are in showOutput mode)
        args <- c(
            r, "CMD", "SHLIB",
            if (windowsDebugDLL) "-d",
            if (rebuild) "--preclean",
            if (dryRun) "--dry-run",
            "-o", shQuote(lib),
            if (length(deps))
                paste(shQuote(deps), collapse = " "),
            shQuote(src)
        )

        cmd <- paste(args, collapse = " ")
        if (showOutput)
            cat(cmd, "\n")										# #nocov

        # execute the build -- suppressWarnings b/c when showOutput = FALSE
        # we are going to explicitly check for an error and print the output
        result <- suppressWarnings(system(cmd, intern = !showOutput))

[kevinushey]

I'm surprised too, to be honest. That said, if there really was an issue affecting Windows users en-masse we surely would've heard a lot more about it by now!

[eddelbuettel]

Yep! All I can think of is that OP's use case of sourceCpp(code="...") is a little less standard than pointing to a file.

Given the above I have less appetite for switching wholesale to system2() and its different argument signature / arrangement. Maybe as you said wrapping shQuote(), and maybe only on Windows is the way to go.

Lunch time here in flyover country. Will ponder ...

[kirimaru-jp]

Here is the whole block (as I currently have i.e. with the PR). There is a shQuote already for the path, so what you suggest is good -- we can do it for binary too. Just puzzled it 'worked for him' over in #1234.

Really sorry, it was my mistake. I changed some code after that to debug, and forgot to restore the original code.
I can confirm that r <- normalizePath(file.path(R.home("bin"), "R"), mustWork = FALSE) did not work either:

> sourceCpp(code = code2)
 Error in system(cmd, intern = !showOutput) : 'd:\Program' not found

shQuote() would be a good choice, as I see on my Win 10 machine:

> shQuote(R.home("bin"))
[1] "\"d:/Program Files/R/R-4.2.1/bin/x64\""

[eddelbuettel]

Ah, no worries, and extra brownie points to @kevinushey for catching this.

I will amend and (conditionally on being on Windows) wrap shQuote() around then.

[eddelbuettel]

Can you please try with these two lines

        r <- file.path(R.home("bin"), "R")
        if (.Platform$OS.type == "windows") r <- shQuote(r)     

in lieu of the faulty one you indentified?

[kirimaru-jp]

r <- file.path(R.home("bin"), "R")
if (.Platform$OS.type == "windows") r <- shQuote(r)

It worked perfectly!

> sourceCpp(code = code2)
> fibonacci(10)
[1] 55

[kirimaru-jp]

I don't think this is the right fix. Here's what I see on my Windows VM:

> R.home("bin")
[1] "C:/PROGRA~2/R/R-42~1.1/bin/x64"

I did not see the short path like yours, here is mine:

> R.home("bin")
[1] "d:/Program Files/R/R-4.2.1/bin/x64"

I checked the help page of R.home, and it writes

On Windows the values of R.home() and R_HOME are switched to the 8.3 short form of path elements if required and if the Windows service to do that is enabled.

I followed this link, and run this command

C:\WINDOWS\system32>fsutil 8dot3name query
The registry state is: 2 (Per volume setting - the default).

It means that (at least on my Windows) the 8.3 short form is enabled only "per volume on the system", which I guess it means it is only enabled for the system drive C by default, while I installed R in D.
So possibly, we do not heard much about that because almost all R users install R in C on Windows, so it works perfectly.

[eddelbuettel]

It's brilliant because the (underappreciated) 'R on Windows FAQ' has recommended for 20-some years to not do that: install in a path with space. Yet R always did, up until the 4.2.* series which (IIRC) finally converted that. I have not worked on Windows in many years so I am a little removed from that.

And agreed on the D:/ vs C:/ difference here -- and that is likely why we generally have not seen this issue bubble up.

[eddelbuettel]

We (informally) had a review here as @kevinushey spotted a thinko on my part but if anyone has a moment for a more 'formal' review and tick-off I'd appreciate it -- cleaner than me pushing through with a merge.

[Enchufa2]

LGTM!