Page MenuHomePhorge

Fix ArcanistExternalLinter on Windows
ClosedPublic

Authored by matmarex on Jul 9 2023, 22:53.

Details

Summary

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.

Changes:

  • 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

Repository
rARC Arcanist
Branch
arcpatch-D25341
Lint
Lint Passed
Unit
Test Failures
Build Status
Buildable 758
Build 758: arc lint + arc unit

Unit TestsFailed

TimeTest
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).

src/filesystem/Filesystem.php
1109

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?

src/lint/linter/ArcanistExternalLinter.php
145

return $resolved; here

src/filesystem/Filesystem.php
1109

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.)

src/filesystem/Filesystem.php
1111–1118

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".

src/filesystem/Filesystem.php
1109

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?

src/filesystem/Filesystem.php
1109

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
This revision was landed with ongoing or failed builds.Dec 4 2023, 21:44
This revision was automatically updated to reflect the committed changes.

I had no idea that I am supposed to "land" these changes. Surely it should be the responsibility of the maintainers to decide into which release a change will go and when?

Anyway, I have been waiting for several months for someone to stop ignoring me and accept my patch, until @aklapper explained it to me… This seems like an awful workflow.

I had no idea that I am supposed to "land" these changes. Surely it should be the responsibility of the maintainers to decide into which release a change will go and when?

Anyway, I have been waiting for several months for someone to stop ignoring me and accept my patch, until @aklapper explained it to me… This seems like an awful workflow.

It’s definitely not ideal, but all community members here have full time jobs and working on this project in spare time. Having authors land the changes avoids the community members being a bottleneck when they can’t get around to patching, checking, landing, etc.

I believe there’s a task for us to set up the server-side landing which would mean you or community members can land just by clicking a button, but we haven’t gotten around to it.