Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change win program resolution to match macOS/Linux #165

Merged
merged 6 commits into from
Jul 26, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
27 changes: 26 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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
Expand Down
72 changes: 54 additions & 18 deletions bb.edn
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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]
Expand Down Expand Up @@ -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"
Expand Down
8 changes: 7 additions & 1 deletion deps.edn
Original file line number Diff line number Diff line change
Expand Up @@ -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"]}}}
33 changes: 22 additions & 11 deletions src/babashka/process.cljc
Original file line number Diff line number Diff line change
Expand Up @@ -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))
borkdude marked this conversation as resolved.
Show resolved Hide resolved
program))
borkdude marked this conversation as resolved.
Show resolved Hide resolved

:else
(fs/which program))]
(-> resolved fs/absolutize fs/normalize str)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change necessary? It wasn't there before?

Copy link
Contributor Author

@lread lread Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did absolutize before but only for relative programs with a dir.
(This was a change I made a while back f2c8d93, I am now wondering if it is absolutely necessary? 🥁)

It could be I am over-absolutizing here. The thing I liked about it is that it makes it very clear exactly what we are resolving to. I think I added normalize to make it clearer what we were absolutizing to, and that also made some test comparisons easier.

But I also see the value of not absolutizing/normalizing unless we need to. Can explore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not processing the program name unless we have to as this may also affect the name of the program that is seen in exceptions etc. (same reason I don't prefer adding an extra cwd above)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to me, I'll make the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I committed the first suggested (because all the tests still worked, I felt it was permissible) but this other change fails a bunch of tests. To diagnose via CI I reversed the bb and JVM tests as the JVM tests give better error messages. Then when I changed the resolved return value to (str resolved) half the CI tests started working, except Windows. FWIW.

(throw (ex-info (str "Cannot resolve program: " program) {}))))
borkdude marked this conversation as resolved.
Show resolved Hide resolved

(defn ^:no-doc default-program-resolver
[{:keys [program] :as opts}]
Expand Down
6 changes: 3 additions & 3 deletions test-native/src/babashka/test_native/run_exec.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"))
Expand Down
69 changes: 69 additions & 0 deletions test-resources/README.md
Original file line number Diff line number Diff line change
@@ -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.
3 changes: 3 additions & 0 deletions test-resources/print-dirs.bat
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
@echo off
echo exepath: %~dpnx0
echo workdir: %cd%
3 changes: 3 additions & 0 deletions test-resources/print-dirs.cmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
@echo off
echo exepath: %~dpnx0
echo workdir: %cd%
Binary file added test-resources/print-dirs.exe
Binary file not shown.
13 changes: 13 additions & 0 deletions test-resources/print-dirs.go
Original file line number Diff line number Diff line change
@@ -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)
}
2 changes: 2 additions & 0 deletions test-resources/print-dirs.ps1
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)"
3 changes: 3 additions & 0 deletions test-resources/print-dirs.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#!/usr/bin/env sh
echo "exepath: $(readlink -f "$0")"
echo "workdir: $(pwd)"
Loading