Fix Process.find_executable once and for all (crystal-lang#9365)

* Fix `Process.find_executable` once and for all

Make it work on Windows.
Make it actually check that the found path is an *executable* *file*.
Fix a lot of edge cases.
Add specs and also verify that these expectations exactly match what Process.new would do.

* fixup! Fix `Process.find_executable` once and for all

* Expand the comment in the manual spec

* Factor out a function listing possibilities of a path to check

* Address review comments + refactor

* Simplify compilation of test executables, doesn't have to be parallel

* Add spec involving `..`

* Resolve review comments

spec/manual/find_executable_spec.cr

@@ -0,0 +1,58 @@
+# Verifies that find_executable's specs match the behavior of Process.run.
+# This doesn't actually test find_executable, only takes all the test cases
+# directly from spec/std/process/find_executable_spec.cr and checks that
+# *they* match what the OS actually does when finding an executable for the
+# purpose of running it.
+
+require "spec"
+require "digest/sha1"
+require "../support/env"
+require "../support/tempfile"
+require "../std/process/find_executable_spec"
+
+describe "Process.run" do
+  test_dir = Path[SPEC_TEMPFILE_PATH] / "manual_find_executable"
+  base_dir = Path[test_dir] / "base"
+  path_dir = Path[test_dir] / "path"
+
+  around_all do |all|
+    Dir.mkdir_p(test_dir)
+
+    exe_names, non_exe_names = FIND_EXECUTABLE_TEST_FILES
+    exe_names.each do |name|
+      src_fn = test_dir / "self_printer.cr"
+      exe_fn = test_dir / "self_printer.exe"
+      File.write(src_fn, "print #{name.inspect}")
+      Process.run("bin/crystal", ["build", "-o", exe_fn.to_s, src_fn.to_s])
+      Dir.mkdir_p((base_dir / name).parent)
+      File.rename(exe_fn, base_dir / name)
+    end
+    non_exe_names.each do |name|
+      File.write(base_dir / name, "")
+    end
+
+    with_env "PATH": {ENV["PATH"], path_dir}.join(Process::PATH_DELIMITER) do
+      Dir.cd(base_dir) do
+        all.run
+      end
+    end
+
+    FileUtils.rm_r(test_dir.to_s)
+  end
+
+  find_executable_test_cases(base_dir).each do |(command, exp)|
+    if exp
+      it "runs '#{command}' as '#{exp}'" do
+        output = Process.run command, &.output.gets_to_end
+        $?.success?.should be_true
+        output.should eq exp
+      end
+    else
+      it "fails to run '#{command}'" do
+        expect_raises IO::Error do
+          Process.run(command)
+        end
+      end
+    end
+  end
+end

spec/std/process/find_executable_spec.cr

@@ -0,0 +1,141 @@
+require "spec"
+require "../../support/tempfile"
+
+{% if flag?(:win32) %}
+  FIND_EXECUTABLE_TEST_FILES = {
+    [
+      "inbase.exe",
+      "not_exe",
+      ".exe",
+      "inboth.exe",
+
+      "inbasebat.bat",
+      "inbase.foo.exe",
+      ".inbase.exe",
+
+      "sub/insub.exe",
+      "sub/not_exe",
+      "sub/.exe",
+
+      "../path/inpath.exe",
+      "../path/not_exe",
+      "../path/.exe",
+      "../path/inboth.exe",
+    ], [] of String,
+  }
+
+  def find_executable_test_cases(pwd)
+    pwd_nodrive = "\\#{pwd.relative_to(pwd.anchor.not_nil!)}"
+    {
+      "inbase.exe"     => "inbase.exe",
+      "inbase"         => "inbase.exe",
+      "sub\\insub.exe" => "sub/insub.exe",
+      "sub/insub"      => "sub/insub.exe",
+      "inpath.exe"     => "../path/inpath.exe",
+      "inpath"         => "../path/inpath.exe",
+      "sub/.exe"       => "sub/.exe",
+      "sub\\"          => "sub/.exe",
+      "sub/"           => "sub/.exe",
+      ".exe"           => ".exe",
+      "not_exe"        => nil,
+      "sub\\not_exe"   => nil,
+      "inbasebat"      => nil,
+      "inbase.foo.exe" => "inbase.foo.exe",
+      "inbase.foo"     => nil,
+      ".inbase.exe"    => ".inbase.exe",
+      ".inbase"        => nil,
+      ""               => nil,
+      "."              => nil,
+      "inboth.exe"     => "inboth.exe",
+      "inboth"         => "inboth.exe",
+      "./inbase"       => "inbase.exe",
+      "../base/inbase" => "inbase.exe",
+      "./inpath"       => nil,
+      "sub"            => nil,
+      "#{pwd}\\sub"    => nil,
+      "#{pwd}\\sub\\"  => nil,
+      # 'C:\Temp\base\inbase', 'C:\Temp\base\.exe', 'C:\Temp\base\'
+      "#{pwd}\\inbase" => "inbase.exe",
+      "#{pwd}\\.exe"   => ".exe",
+      "#{pwd}\\"       => nil,
+      # 'C:inbase', 'C:.exe', 'C:'
+      "#{pwd.drive}inbase" => "inbase.exe",
+      "#{pwd.drive}.exe"   => ".exe",
+      "#{pwd.drive}"       => nil,
+      "#{pwd.drive}sub\\"  => nil,
+      # '\Temp\base\inbase', '\Temp\base\.exe', '\Temp\base\'
+      "#{pwd_nodrive}\\inbase" => "inbase.exe",
+      "#{pwd_nodrive}\\.exe"   => ".exe",
+      "#{pwd_nodrive}\\"       => nil,
+    }
+  end
+{% else %}
+  FIND_EXECUTABLE_TEST_FILES = {
+    [
+      "inbase",
+      "sub/insub",
+      "../path/inpath",
+    ], [
+      "not_exe",
+      "sub/not_exe",
+      "../path/not_exe",
+    ],
+  }
+
+  def find_executable_test_cases(pwd)
+    {
+      "./inbase"       => "inbase",
+      "../base/inbase" => "inbase",
+      "inbase"         => nil,
+      "sub/insub"      => "sub/insub",
+      "inpath"         => "../path/inpath",
+      "./inpath"       => nil,
+      "inbase/"        => nil,
+      "sub/insub/"     => nil,
+      "./not_exe"      => nil,
+      "not_exe"        => nil,
+      "sub/not_exe"    => nil,
+      ""               => nil,
+      "."              => nil,
+      "#{pwd}/inbase"  => "inbase",
+      "#{pwd}/inbase/" => nil,
+      "#{pwd}/sub"     => nil,
+      "./sub"          => nil,
+      "sub"            => nil,
+    }
+  end
+{% end %}
+
+describe "Process.find_executable" do
+  test_dir = Path[SPEC_TEMPFILE_PATH] / "find_executable"
+  base_dir = Path[test_dir] / "base"
+  path_dir = Path[test_dir] / "path"
+
+  around_all do |all|
+    exe_names, non_exe_names = FIND_EXECUTABLE_TEST_FILES
+    (exe_names + non_exe_names).each do |name|
+      Dir.mkdir_p((base_dir / name).parent)
+      File.write(base_dir / name, "")
+    end
+    exe_names.each do |name|
+      File.chmod(base_dir / name, 0o755)
+    end
+
+    all.run
+
+    FileUtils.rm_r(test_dir.to_s)
+  end
+
+  find_executable_test_cases(base_dir).each do |(command, exp)|
+    if exp
+      exp_path = File.expand_path(exp, base_dir)
+      it "finds '#{command}' as '#{exp}'" do
+        Process.find_executable(command, path: path_dir.to_s, pwd: base_dir).should eq exp_path
+      end
+    else
+      it "fails to find '#{command}'" do
+        Process.find_executable(command, path: path_dir.to_s, pwd: base_dir).should be_nil
+      end
+    end
+  end
+end

spec/std/process_spec.cr

@@ -402,27 +402,6 @@ describe Process do
     end
   end
 
-  describe "find_executable" do
-    pwd = Process::INITIAL_PWD
-    crystal_path = Path.new(pwd, "bin", "crystal").to_s
-
-    pending_win32 "resolves absolute executable" do
-      Process.find_executable(Path.new(pwd, "bin", "crystal")).should eq(crystal_path)
-    end
-
-    pending_win32 "resolves relative executable" do
-      Process.find_executable(Path.new("bin", "crystal")).should eq(crystal_path)
-      Process.find_executable(Path.new("..", File.basename(pwd), "bin", "crystal")).should eq(crystal_path)
-    end
-
-    pending_win32 "searches within PATH" do
-      (path = Process.find_executable("ls")).should_not be_nil
-      path.not_nil!.should match(/#{File::SEPARATOR}ls$/)
-
-      Process.find_executable("some_very_unlikely_file_to_exist").should be_nil
-    end
-  end
-
   describe "quote_posix" do
     it { Process.quote_posix("").should eq "''" }
     it { Process.quote_posix(" ").should eq "' '" }

src/process/executable_path.cr

@@ -28,13 +28,47 @@ class Process
     end
   end
 
+  private def self.is_executable_file?(path)
+    unless File.info?(path, follow_symlinks: true).try &.file?
+      return false
+    end
+    {% if flag?(:win32) %}
+      # This is *not* a temporary stub.
+      # Windows doesn't have "executable" metadata for files, so it also doesn't have files that are "not executable".
+      true
+    {% else %}
+      File.executable?(path)
+    {% end %}
+  end
+
   # Searches an executable, checking for an absolute path, a path relative to
   # *pwd* or absolute path, then eventually searching in directories declared
   # in *path*.
   def self.find_executable(name : Path | String, path : String? = ENV["PATH"]?, pwd : Path | String = Dir.current) : String?
-    name = Path.new(name)
+    find_executable_possibilities(Path.new(name), path, pwd) do |p|
+      if is_executable_file?(p)
+        return p.to_s
+      end
+    end
+    nil
+  end
+
+  private def self.find_executable_possibilities(name, path, pwd)
+    return if name.to_s.empty?
+
+    {% if flag?(:win32) %}
+      # https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw#parameters
+      # > If the file name does not contain an extension, .exe is appended.
+      # See find_executable_spec.cr for cases this needs to match, based on CreateProcessW behavior.
+      basename = name.ends_with_separator? ? "" : name.basename
+      basename = "" if basename == name.anchor.to_s
+      if (basename.empty? ? !name.anchor : !basename.includes?("."))
+        name = Path.new("#{name}.exe")
+      end
+    {% end %}
+
     if name.absolute?
-      return name.to_s
+      yield name
     end
 
     # check if the name includes a separator
@@ -43,19 +77,17 @@ class Process
       count_parts += 1
       break if count_parts > 1
     end
+    has_separator = (count_parts > 1)
 
-    if count_parts > 1
-      return name.expand(pwd).to_s
+    if {{ flag?(:win32) }} || has_separator
+      yield name.expand(pwd)
     end
 
-    return unless path
-
-    path.split(PATH_DELIMITER).each do |path_entry|
-      executable = Path.new(path_entry, name)
-      return executable.to_s if File.exists?(executable)
+    if path && !has_separator
+      path.split(PATH_DELIMITER).each do |path_entry|
+        yield Path.new(path_entry, name)
+      end
     end
-
-    nil
   end
 end