Page MenuHomePhabricator

Windows quoting conventions still bad
Closed, DuplicatePublic

Description

Sadly, I think that T4395 is insufficient to fix the Windows quoting problems. It looks like cmd has a bug when invoked like this:

cmd /c ""jshint" "--version""

In this case, jshint is a batch file generated by npm's cmd-shim:

jshint.cmd
@IF EXIST "%~dp0\node.exe" (
  "%~dp0\node.exe"  "%~dp0\node_modules\jshint\bin\jshint" %*
) ELSE (
  node  "%~dp0\node_modules\jshint\bin\jshint" %*
)

When invoked the way arc lint does, %~dp0 does not get correctly set, so it refers to $CWD instead of the directory containing jshint.cmd. So, %~dp0\node_modules\jshint\bin\jshint doesn't resolve, and jshint fails to run.

Specifically, quoting the program name is what trips up cmd, so even invoking "jshint" directly in the shell fails. There's a corresponding npm bug filed, but I think it's worth fixing in phabricator to handle this edge case more robustly.

Recommendation: Not quoting the binary but still quoting the rest of the args fixes it (cmd /c "jshint "--version""). This solves the problem for binaries without spaces in the names, which should be 99.9% of binaries. If the binary does have a space in the name, nothing can save you from %~dp0 being wrong, as far as I can tell. I made a dummy "js hint.cmd" with the batch code above, and even invoking "js hint" directly at the shell screws things up.

Event Timeline

staticshock raised the priority of this task from to Needs Triage.
staticshock updated the task description. (Show Details)
staticshock added a project: Arcanist.
staticshock added a subscriber: staticshock.

The cmd /? help page says that the special characters that require quotes are:

<space>
&()[]{}^=;!'+,`~

So, this might be a reasonable solution:

--- a/src/lint/linter/ArcanistExternalLinter.php
+++ b/src/lint/linter/ArcanistExternalLinter.php
@@ -321,9 +321,9 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter {
     $binary = $this->getBinary();

     if ($interpreter) {
-      $bin = csprintf('%s %s', $interpreter, $binary);
+      $bin = csprintf('%R %s', $interpreter, $binary);
     } else {
-      $bin = csprintf('%s', $binary);
+      $bin = csprintf('%R', $binary);
     }

     return $bin;
ctuwuzida moved this task from In Progress to Backlog on the Arcanist board.
ctuwuzida moved this task from Backlog to Completed on the Arcanist board.
ctuwuzida moved this task from Completed to Backlog on the Arcanist board.

Summary of a conversation I had about this with @epriestley on IRC:

Critique 1: %R has the same behavior as %s in some cases. Offhand, the binary path may very reasonably have spaces in it, if it's entered as "C:/Program FIles (x86)/Node/bin/jshint" or whatever.
Response: When a path is specified, the quotes are fine. The only case in which the quotes are not fine is executable names that are looked up in $PATH, such as "jshint".

Critique 2: Since we haven't seen much in the way of reports, either no users are interested in doing that or it's working fine for many of them. If it's completely broken and no one cares, that makes it even less important to fix.
Response: To run into this, people have to be:

  1. Developing on Windows.
  2. Using Differential.
  3. Using arcanist for linting.
  4. Specifically using node.js-based linters, for which npm generates batch-based wrappers that use %~dp0.

Each one of those will weed some people out, so the impacted group is certainly a minority. And there are certainly workarounds, such as using full paths, or adding shims to the repo, but they're annoying and tiresome for something that could have easily "just worked".

The patch I provided makes it "just work", and, I believe, has no downsides.