From 16a412b1080290d798b5f903e8b5cb2df1e957d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= <matma.rex@gmail.com>
Date: Sat, 19 Aug 2023 23:45:37 +0200
Subject: [PATCH] Fix ArcanistExternalLinter on Windows

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.

Reviewers: O1 Blessed Committers, avivey

Reviewed By: O1 Blessed Committers, avivey

Subscribers: aklapper, avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno

Maniphest Tasks: T15544

Differential Revision: https://we.phorge.it/D25341
---
 src/filesystem/Filesystem.php              | 17 ++++++++++++++---
 src/lint/linter/ArcanistExternalLinter.php | 14 +++++++++++++-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/src/filesystem/Filesystem.php b/src/filesystem/Filesystem.php
index 9cd52268..981d4feb 100644
--- a/src/filesystem/Filesystem.php
+++ b/src/filesystem/Filesystem.php
@@ -1103,12 +1103,23 @@ final class Filesystem extends Phobject {
         }
         return null;
       }
-      $stdout = head($stdout);
+
+      // These are the only file extensions that can be executed directly
+      // when using proc_open() with 'bypass_shell'.
+      $executable_extensions = ['exe', 'bat', 'cmd', 'com'];
+
+      foreach ($stdout as $line) {
+        $path = trim($line);
+        $ext = pathinfo($path, PATHINFO_EXTENSION);
+        if (in_array($ext, $executable_extensions)) {
+          return $path;
+        }
+      }
+      return null;
     } else {
       list($err, $stdout) = exec_manual('which %s', $binary);
+      return $err === 0 ? trim($stdout) : null;
     }
-
-    return $err === 0 ? trim($stdout) : null;
   }
 
 
diff --git a/src/lint/linter/ArcanistExternalLinter.php b/src/lint/linter/ArcanistExternalLinter.php
index a394528f..8c35e440 100644
--- a/src/lint/linter/ArcanistExternalLinter.php
+++ b/src/lint/linter/ArcanistExternalLinter.php
@@ -133,7 +133,19 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter {
    * @task bin
    */
   final public function getBinary() {
-    return coalesce($this->bin, $this->getDefaultBinary());
+    $bin = coalesce($this->bin, $this->getDefaultBinary());
+    if (phutil_is_windows()) {
+      // On Windows, we use proc_open with 'bypass_shell' option, which will
+      // resolve %PATH%, but not %PATHEXT% (unless the extension is .exe).
+      // Therefore find the right binary ourselves.
+      // If we can't find it, leave it unresolved, as this string will be
+      // used in some error messages elsewhere.
+      $resolved = Filesystem::resolveBinary($bin);
+      if ($resolved) {
+        return $resolved;
+      }
+    }
+    return $bin;
   }
 
   /**