Change win program resolution to match macOS/Linux

After [some careful study](https://github.com/lread/info-process-builder/blob/main/README.adoc#program-resolution), I now understand the underlying behaviour of ProcessBuilder on Windows. It has some nuances: - it resolves from the current dir before PATH (which CMD Shell does too - but PowerShell opted not to do for security reasons) - it only resolves `a` to `a.exe` - and has what seem to be bugs wrt to resolving when a `directory` working dir is specified These oddities were sometimes leaking through to babashka process when run from Windows. We now entirely take over the program resolution for Windows in our default `:program-resolver` which now matches the behaviour of PowerShell (with the exception of launching `.ps1` files, we do not do that), macOS and Linux. Note that the default `:program-resolver` is also employed for `exec` on all OSes. Because it matches macOS/Linux behaviour this does not present any problems. Tests now thoroughly exercise program resolution via explicitly constructed scenarios instead of awkwardly relying on what happens to be on the PATH. See new test-resources dir README for some details. Bb tasks adjusted accordingly to setup the `PATH` for tests. New `dev:bb` and `dev:jvm` tasks added to launch nREPLs with this same setup. README is updated with new section on Program Resolution. Closes #163 Closes #164
babashka · Jul 25, 2024 · ded3bc9 · ded3bc9
1 parent ae1656b
commit ded3bc9
Show file tree

Hide file tree

Showing 16 changed files with 618 additions and 122 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,10 @@
 [Babashka process](https://github.com/babashka/process)
 Clojure library for shelling out / spawning sub-processes
 
+## Unreleased 
+
+- [#163](https://github.com/babashka/process/issues/163), [#164](https://github.com/babashka/process/issues/164): Program resolution strategy for `exec` and Windows now matches macOS/Linux/PowerShell ([@lread](https://github.com/lread))
+
 ## 0.5.22 (2024-02-29)
 
 - [#123](https://github.com/babashka/process/issues/123): `exec` now converts `:env` and `:extra-env` keywords ([@lread](https://github.com/lread))

diff --git a/README.md b/README.md
@@ -96,7 +96,7 @@ ls: nothing: No such file or directory
 Execution error (ExceptionInfo) at babashka.process/check (process.cljc:113).
 ```
 
-To avoid throwing when the command's exit code is non-zero, use `:continue true`. 
+To avoid throwing when the command's exit code is non-zero, use `:continue true`.
 You will still see the error printed to stderr, but no exception will be thrown. This is convenient
 when you want to handle the `:exit` code yourself:
 
@@ -490,6 +490,31 @@ nil
 
 Another solution is to let bash handle the pipes by shelling out with `bash -c`.
 
+## Program Resolution
+
+### macOS & Linux
+On macOS & Linux, programs are resolved the way you expect:
+
+- `a` resolves against the system `PATH`
+- `./a` resolves against `:dir` if specified, otherwise the current working directory
+- `/some/absolute/a` resolves absolutely
+
+In all cases, the working directory for `a` is `:dir`, if specified, otherwise your current working directory.
+
+### Windows
+
+Windows executable files have extensions, which, if not specified, are resolved in order: `.com`,`.exe`,`.bat`,`.cmd`.
+Programs are resolved in directories using the same rules as macOS, Linux, and Windows PowerShell.
+
+> **Windows .ps1 TIP**: Babashka process will never resolve to, and cannot launch, `.ps1` scripts directly.
+To launch a `.ps1` script, you must do so through PowerShell.
+Example:
+> ```Clojure
+> (p/shell "powershell.exe -File .\\a.ps1")
+> ```
+
+> **Windows TIP**: If you prefer a more CMD Shell-like experience where programs are resolved first in the current working directory, then on the `PATH`, and are OK with the security implications of doing so, you can override the default `:program-resolver` with your own.
+
 ## Differences with `clojure.java.shell/sh`
 
 If `clojure.java.shell` works for your purposes, keep using it. But there are

diff --git a/bb.edn b/bb.edn
@@ -3,13 +3,58 @@
         #_{:local/root "../../quickdoc"}
         {:git/url "https://github.com/borkdude/quickdoc"
          :git/sha "32e726cd6d785d00e49d4e614a05f7436d3831c0"}
-        org.clj-commons/digest {:mvn/version "1.4.100"} }
+        org.clj-commons/digest {:mvn/version "1.4.100"}}
+
  :tasks
- {:requires ([babashka.fs :as fs])
+ {:requires ([babashka.cli :as cli]
+             [babashka.fs :as fs])
+  :init (do
+          (def shell-opts {:extra-env
+                           {(if (fs/windows?) "Path" "PATH")
+                            (str (fs/canonicalize "target/test/on-path") fs/path-separator (System/getenv "PATH"))}})
+          (defn parse-repl-args [args]
+            (let [cli-spec {:spec
+                            {:host {:desc "Bind to host (use 0.0.0.0 to allow anyone to connect)"
+                                    :alias :h
+                                    :default "localhost"}}
+                            :restrict true}]
+                (cli/parse-opts args cli-spec))))
   clean {:doc "Delete build work"
          :task (do
                  (fs/delete-tree "target")
                  (shell {:dir "test-native"} "bb clean"))}
+
+  dev:jvm {:doc "Start a jvm nREPL server with PATH appropriatly setup for tests"
+           :task (let [opts (parse-repl-args *command-line-args*)
+                       host (:host opts)]
+                   (shell shell-opts
+                          "clj" "-M:clj-1.11:test:nrepl/jvm" "-h" host "-b" host))}
+
+  -bb {:doc "Internal - launched by dev:bb, test:bb"
+            :extra-paths ["test"]
+            :extra-deps  {;; inherit base deps from deps.edn
+                          babashka/process {:local/root "."}
+                          ;; repeat necessary :test deps from deps.edn
+                          io.github.cognitect-labs/test-runner {:git/tag "v0.5.1" :git/sha "dfb30dd"}}
+            :requires ([cognitect.test-runner])
+       :task (let [target (cli/coerce (first *command-line-args*) :keyword) ;; should be either :test or :dev
+                   args (rest *command-line-args*)]
+               ;; flag: sub-process should reload babashka.process
+               (System/setProperty "babashka.process.test.reload" "true")
+               ;; flag: force use of bb even if natively compiled version of run-exec exists
+               (System/setProperty "babashka.process.test.run-exec" "bb")
+               ;; run from babashka.process sources, not built-in babaska.process
+               (require '[babashka.process] :reload)
+               (case target
+                 :test (apply cognitect.test-runner.-main args)
+                 :dev (let [opts (parse-repl-args args)]
+                        (babashka.nrepl.server/start-server! opts)
+                        (deref (promise)))))}
+
+  dev:bb {:doc "Start a bb nREPL server with PATH appropriately setup for tests"
+          :task (apply shell shell-opts
+                       "bb -bb :dev" *command-line-args*)}
+
   quickdoc {:doc "Invoke quickdoc"
             :requires ([quickdoc.api :as api])
             :task (api/quickdoc {:git/branch "master"
@@ -22,23 +67,12 @@
 
   test:native {:doc "Run exec tests with native runner (requires GraalVM compilation)."
                :depends [-prep-native-exec]
-               :task (apply clojure "-M:test:clj-1.11" "--namespace" "babashka.process-exec-test" *command-line-args*)}
+               :task (apply clojure shell-opts
+                            "-M:test:clj-1.11" "--namespace" "babashka.process-exec-test" *command-line-args*)}
 
   test:bb {:doc "Run all tests under bb"
-           :extra-paths ["test"]
-           :extra-deps  {;; inherit base deps from deps.edn
-                         babashka/process {:local/root "."}
-                         ;; repeat necessary :test deps from deps.edn
-                         io.github.cognitect-labs/test-runner {:git/tag "v0.5.1" :git/sha "dfb30dd"}}
-           :requires ([cognitect.test-runner])
-           :task (do
-                   ;; flag: sub-process should reload babashka.process
-                   (System/setProperty "babashka.process.test.reload" "true")
-                   ;; flag: force use of bb even if natively compiled version of run-exec exists
-                   (System/setProperty "babashka.process.test.run-exec" "bb")
-                   ;; run from babashka.process sources, not built-in babaska.process
-                   (require '[babashka.process] :reload)
-                   (apply cognitect.test-runner.-main *command-line-args*))}
+           :task (apply shell shell-opts
+                        "bb -bb :test" *command-line-args*)}
 
   test:jvm {:doc "Run jvm tests, optionally specify clj-version (ex. :clj-1.10 :clj-1.11(default) or :clj-all)"
             :requires ([clojure.string :as str]
@@ -72,7 +106,9 @@
                     (doseq [alias aliases]
                       (do
                         (println (format "-[Running jvm tests for %s]-" alias))
-                        (apply clojure (str "-M:test" alias) "--namespace" "babashka.process-test" args))))}
+                        (apply clojure
+                               shell-opts
+                               (str "-M:test" alias) "--namespace" "babashka.process-test" args))))}
 
   ;; hidden CI support tasks
   -ci-install-jdk {:doc "Helper to download and install jdk under ~/tools on circleci"

diff --git a/deps.edn b/deps.edn
@@ -5,4 +5,10 @@
            :clj-1.9 {:extra-deps {org.clojure/clojure {:mvn/version "1.9.0"}}}
            :clj-1.10 {:extra-deps {org.clojure/clojure {:mvn/version "1.10.3"}}}
            :clj-1.11 {:extra-deps {org.clojure/clojure {:mvn/version "1.11.3"}}}
-           :clj-1.12 {:extra-deps {org.clojure/clojure {:mvn/version "1.12.0-beta1"}}}}}
+           :clj-1.12 {:extra-deps {org.clojure/clojure {:mvn/version "1.12.0-beta1"}}}
+           :nrepl/jvm {:extra-deps {nrepl/nrepl                {:mvn/version "1.2.0"}
+                                    cider/cider-nrepl          {:mvn/version "0.49.1"}}
+                       :jvm-opts ["-XX:-OmitStackTraceInFastThrow"]
+                       :main-opts  ["-m" "nrepl.cmdline"
+                                    "--middleware" "[cider.nrepl/cider-middleware]"
+                                    "-i"]}}}
diff --git a/src/babashka/process.cljc b/src/babashka/process.cljc
@@ -178,17 +178,28 @@
       (str/includes? "windows")))
 
 (defn- -program-resolver [{:keys [program dir]}]
-  ;; this should make life easier and not cause any bugs that weren't there previously
-  ;; on exception we just return the program as is
-  (try
-    (if (fs/relative? program)
-      (if-let [f (fs/which (if dir
-                             (-> (fs/file dir program) fs/absolutize)
-                             program))]
-        (str f)
-        program)
-      program)
-    (catch Throwable _ program)))
+  ;; Used by default:
+  ;; - for all program resolution when running on Windows
+  ;; - by `exec` on all OSes
+  ;; Default ProcessBuilder behaviour on macOS/Linux does this work naturally
+  ;; and is therefore unneeded.
+  ;;
+  ;; ProcessBuilder program resolution on Windows does not entirely match
+  ;; behaviour of CMD Shell nor PowerShell.
+  ;; Here we adapt resolution to match PowerShell (with the exclusion of resolving
+  ;; .ps1 scripts) which follows the same strategy as macOS/Linux.
+  (if-let [resolved (cond
+                      (fs/absolute? program)
+                      (fs/which program) ;; to resolve any extensions
+
+                      (fs/parent program)
+                      (fs/which (fs/file (or dir (fs/cwd))
+                                         program))
+
+                      :else
+                      (fs/which program))]
+    (-> resolved fs/absolutize fs/normalize str)
+    (throw (ex-info (str "Cannot resolve program: " program) {}))))
 
 (defn ^:no-doc default-program-resolver
   [{:keys [program] :as opts}]

diff --git a/test-native/src/babashka/test_native/run_exec.clj b/test-native/src/babashka/test_native/run_exec.clj
@@ -45,10 +45,10 @@
   "call with a list of `args` for `babashka.process/exec`
 
   for adhoc (under bash, Windows shell will have different escaping rules):
-  bb exec-run.clj \"({:arg0 'new-arg0'} bb wd.clj :out foo :exit 3)\"
+  bb run-exec.clj \"({:arg0 'new-arg0'} bb wd.clj :out foo :exit 3)\"
 
   to avoid shell command escaping hell can also read args from edn file
-  bb exec-run.clj --file some/path/here.edn"
+  bb run-exec.clj --file some/path/here.edn"
   [& args]
   (let [exec-args (parse-args args)]
     (try
@@ -59,7 +59,7 @@
       (System/exit 42)
       (catch Exception ex
         ;; an error occurred launching exec
-        (println (pr-str (Throwable->map ex)))))))
+        (println (pr-str {:bbp-test-run-exec-exception (Throwable->map ex)}))))))
 
 ;; support invocation from babashka when run as script
 (when (= *file* (System/getProperty "babashka.file"))

diff --git a/test-resources/README.md b/test-resources/README.md
@@ -0,0 +1,69 @@
+# Babashka Process Test Resources
+
+For test coverage, we need executables that emit their path and their current working directory.
+For example, on Windows:
+```
+> .\print-dirs.bat
+exepath: Z:\babashka\process\test-resources\print-dirs.bat
+workdir: Z:\babashka\process\test-resources
+> cd ..
+>test-resources\print-dirs
+exepath: Z:\babashka\process\test-resources\print-dirs.exe
+workdir: Z:\babashka\process
+```
+
+This is straightforward for Linux/macOS `.sh`, Windows `.cmd` and Windows `.bat` variants, but for the Windows `.exe` variant, we need to create a binary.
+
+## Generating a Windows .exe
+
+It doesn't matter how `print-dirs.exe` is created; it won't need to be recreated often (we commit it to source control).
+
+Since it generates relatively small binaries very quickly for any architecture, we've built a `print-dirs.exe` using Go.
+Source is in [print-dirs.go](print-dirs.go)
+
+### Minimal Build
+Presuming you have [Go](https://go.dev/) installed, from this dir:
+
+```shell
+GOOS=windows GOARCH=amd64 go build -o print-dirs.exe print-dirs.go
+```
+
+For me, this generated a 1.9mb `print-dirs.exe`.
+
+### Build a Smaller Exe
+Since this `.exe` is checked into version control, I opted to create a smaller binary by adding the following `-ldflags` to strip debug info:
+
+```shell
+GOOS=windows GOARCH=amd64 go build -ldflags "-s -w" -o print-dirs.exe print-dirs.go
+```
+
+For me, the generated `print-dirs.exe` is now 1.3mb.
+To further shrink down the binary, you can use [UPX](https://upx.github.io/) (which is also cross-platform):
+
+```
+upx --ultra-brute print-dirs.exe
+```
+
+And now `print-dirs.exe` is 457kb.
+
+### Building With Docker
+If you don't want to install Go and UPX on your dev box but have docker installed, you can build from a docker image like so:
+
+``` shell
+docker run --rm \
+  -v "$PWD":/src \
+  -w /src \
+  devopsworks/golang-upx:latest \
+  bash -c 'GOOS=windows GOARCH=amd64 \
+    go build -ldflags "-s -w" -o print-dirs.exe print-dirs.go &&
+    upx --ultra-brute print-dirs.exe'
+```
+
+This is ultimately the command I ran to create our Windows .exe.
+
+## What about Windows .com?
+
+These days, `.com` executables are rare.
+
+I could not find an easy way to create one.
+Our tests use `print-dirs.exe` copied to `print-dirs.com` to support `.com` test cases.
diff --git a/test-resources/print-dirs.bat b/test-resources/print-dirs.bat
@@ -0,0 +1,3 @@
+@echo off
+echo exepath: %~dpnx0
+echo workdir: %cd%
diff --git a/test-resources/print-dirs.cmd b/test-resources/print-dirs.cmd
@@ -0,0 +1,3 @@
+@echo off
+echo exepath: %~dpnx0
+echo workdir: %cd%
diff --git a/test-resources/print-dirs.exe b/test-resources/print-dirs.exe
diff --git a/test-resources/print-dirs.go b/test-resources/print-dirs.go
@@ -0,0 +1,13 @@
+package main
+
+import (
+    "fmt"
+    "os"
+)
+
+func main() {
+    executable, _ := os.Executable()
+    fmt.Fprintln(os.Stdout,"exepath:", executable)
+    cwd, _ := os.Getwd()
+    fmt.Fprintln(os.Stdout,"workdir:", cwd)
+}
diff --git a/test-resources/print-dirs.ps1 b/test-resources/print-dirs.ps1
@@ -0,0 +1,2 @@
+Write-Output "exepath: $($MyInvocation.MyCommand.Path)"
+Write-Output "workdir: $(Get-Location)"
diff --git a/test-resources/print-dirs.sh b/test-resources/print-dirs.sh
@@ -0,0 +1,3 @@
+#!/usr/bin/env sh
+echo "exepath: $(readlink -f "$0")"
+echo "workdir: $(pwd)"
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		Write-Output "exepath: $($MyInvocation.MyCommand.Path)"
		Write-Output "workdir: $(Get-Location)"