Page MenuHomePhorge

Fix ArcanistExternalLinter on Windows

Authored by matmarex on Jul 9 2023, 22:53.
Referenced Files
Unknown Object (File)
Thu, Sep 21, 09:01
Unknown Object (File)
Sun, Sep 3, 16:27
Unknown Object (File)
Tue, Aug 29, 09:25
Unknown Object (File)
Sat, Aug 26, 04:19
Unknown Object (File)
Fri, Aug 25, 16:47
Unknown Object (File)
Aug 9 2023, 00:51
Unknown Object (File)
Aug 7 2023, 07:28
Unknown Object (File)
Aug 6 2023, 23:02



When using proc_open() with 'bypass_shell' => true on Windows,
file extensions other than .exe will not be resolved. Various linters
therefore don't work, such as jshint, which is actually jshint.cmd.

The problem was already observed and fixed in some places (e.g.
ArcanistGitAPI trying to run git), but not in ArcanistExternalLinter.


  • Fix Filesystem::resolveBinary() to actually only resolve executable files on Windows, and not any other files with no extension or with an extension listed in %PATHEXT%. Those files can be executed by typing their name in the cmd.exe shell, but not directly by low-level Windows functions, and we're using 'bypass_shell' to bypass the shell.
  • Fix ArcanistExternalLinter::getBinary() to call Filesystem::resolveBinary() on Windows.
Test Plan

Run arc lint on the Phorge repository while on Windows.
Observe no errors related to jshint.

Diff Detail

rARC Arcanist
Lint Passed
Test Failures
Build Status
Buildable 758
Build 758: arc lint + arc unit

Unit TestsFailed

5 msArcanistChmodLinterTestCase::testLinter
Lint emitted an unexpected set of messages for file "empty_executable.lint-test". MISSING MESSAGES Message with severity "warning" at "<null>:<null>" (CHMOD1)
4,659 msArcanistJSHintLinterTestCase::testLinter
Lint emitted an unexpected set of messages for file "too-many-errors.lint-test". MISSING MESSAGES Message with severity "disabled" at "2:22" (E043)
1,989 msArcanistJscsLinterTestCase::testLinter
Lint emitted an unexpected set of messages for file "curly-brace.lint-test". MISSING MESSAGES (No messages.)
14,665 msArcanistRuboCopLinterTestCase::testLinter
Lint emitted an unexpected set of messages for file "no_errors.lint-test". MISSING MESSAGES (No messages.)
1,285 msArcanistRuboCopLinterTestCase::testVersion
Assertion failed, expected 'true' (at ArcanistExternalLinterTestCase.php:10): Failed to parse version from command. ACTUAL VALUE
View Full Test Results (13 Failed · 43 Passed · 26 Skipped)

Event Timeline

matmarex requested review of this revision.Jul 9 2023, 22:53

I am not sure if these build failures are real? I guess they are from the run on my own machine, which indeed failed on a bunch of tests, but that might be just because I don't have every linter installed? If they are real, I'd appreciate some help investigating them.

Improve error messages when a binary is missing

Okay, that's better, some of the failures were real-ish. The remaining ones look like either incompatibilities with Windows in other code that I am not touching here, or issues due to having a different version of a linter installed than expected.

Looks good - I'll try to figure out the failed tests issue(s).


I'm not sure how, but on my machine, where docker returns C:\Program Files\Docker\Docker\resources\bin\docker, and running this actually does work (it's a shell script. starts with #!/usr/bin/env. I don't have WSL, AFAIK).

So probably add empty string to the list of options?


return $resolved; here


Does where only return that, or are there multiple results? And do you mean running it from the command prompt, or using proc_open() with 'bypass_shell'? I suspect it might actually be executing a docker.exe in the same directory, or something like that.

But if you really can do that with an extension-less file, then I guess I'll need to install Docker and figure out how it does it. If I just allowed files with no extension here, that would break my jshint problem again, so I'd need to find a different solution. (There's an extension-less C:\Users\a\AppData\Roaming\npm\jshint in my case, which is a shell script as well, but it can't be executed.)


It looks like pathinfo(..., PATHINFO_EXTENSION) can be used to get the file's extension. It's not used often in the phorge codebase but it's probably reasonable to use.

I've patched the branch and ran the tests on Linux.
Of the 6 failed test files (13 tests):

  • 1 fails in master (ArcanistJSHintLinterTestCase)
  • 2 are skipped locally because I don't have the external binaries (ArcanistJscsLinterTestCase, ArcanistRuboCopLinterTestCase)
  • and the rest pass with these changes

so I think we can write off all of these failed tests as either "broken" (the jshint one) or "broken on Windows".


It returns both C:\Program Files\Docker\Docker\resources\bin\docker and C:\Program Files\Docker\Docker\resources\bin\docker.exe, when running from command line.

But it's also possible that when I think I'm running C:\Program Files\Docker\Docker\resources\bin\docker, it actually implicitly runs the exe file with the same name - I'll check that more tomorrow (it's the work machine).

does where return the extension-less jshint for you?


Checked on the Windows machine again, and I think where is lying.

I copied docker to docker2, and where docker2 does show it, but trying to execute it days "not recognized as ....", so I it probably not executing the docker file as well.

I ran where /?, and it looks like where isn't even pretending to return "executables" - it "Displays the location of files that match the search pattern. By default, the search is done along the current directory and in the paths specified by the PATH environment variable."
and the examples explicitly show "*.dll" as a reasonable use-case.

So there's probably no reason to add '' to the extensions list.

This revision is now accepted and ready to land.Aug 16 2023, 06:57