Fix #163, #164 Change win program resolution to match macOS/Linux (#165)

Co-authored-by: Michiel Borkent <michielborkent@gmail.com>
babashka · Jul 26, 2024 · 264e950 · 264e950
1 parent ae1656b
commit 264e950
Show file tree

Hide file tree

Showing 16 changed files with 619 additions and 121 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,30 @@
       (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)
+                      (if dir
+                        ;; we need to absolutize here to overcome a bug in Windows ProcessBuilder
+                        (some-> (fs/which (fs/file dir program)) fs/absolutize)
+                        (fs/which program))
+
+                      :else
+                      (fs/which program))]
+    (str resolved)
+    (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)"