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

Unable to reload babashka.process/exec #1557

Closed
lread opened this issue May 13, 2023 · 1 comment · Fixed by #1558
Closed

Unable to reload babashka.process/exec #1557

lread opened this issue May 13, 2023 · 1 comment · Fixed by #1558

Comments

@lread
Copy link
Contributor

lread commented May 13, 2023

version

❯ bb --version
babashka v1.3.179

platform
Repro on Linux, but should apply to all.

problem
Babashka comes bundled with babashka.processs built in.
Sometimes a newer version of babashka.process becomes available.

For example, at the time of this writing, a newer version of babashka.process is available with a :cmd opt feature.

A user can opt to use the newer version via a ns reload.
This works great in general, but not for the exec function.

repro
Given a bb.edn that specifies the new version:

{:deps {babashka/process {:mvn/version "0.5.19"}}}

Here's a REPL session that shows the behaviour.
We'll start off using built-in babashka.process:

❯ rlwrap bb
Babashka v1.3.179 REPL.
Use :repl/quit or :repl/exit to quit the REPL.
Clojure rocks, Bash reaches.

user=> (require '[babashka.process :as p])
nil

And now let's call a new feature that is not yet in bb (it will fail):

user=> (p/shell {:cmd ["ls" "-l"]})
java.lang.NullPointerException [at <repl>:2:1]

And we'll reload the ns to get the updated version specified in bb.edn and retry our command (it will work)

user=> (require '[babashka.process :as p] :reload)
nil
user=> (p/shell {:cmd ["ls" "-l"]})
total 4
-rw-rw-r-- 1 lee lee 51 May 13 12:39 bb.edn
{:proc #object[java.lang.ProcessImpl 0x6d1bac37 "Process[pid=66629, exitValue=0]"], :exit 0, :in #object[java.lang.ProcessBuilder$NullOutputStream 0x73925281 "java.lang.ProcessBuilder$NullOutputStream@73925281"], :out #object[java.lang.ProcessBuilder$NullInputStream 0x1affa17d "java.lang.ProcessBuilder$NullInputStream@1affa17d"], :err #object[java.lang.ProcessBuilder$NullInputStream 0x1affa17d "java.lang.ProcessBuilder$NullInputStream@1affa17d"], :prev nil, :cmd ["ls" "-l"]}

Now let's try a call to exec to show the issue:

user=> (p/exec "ls -l")
clojure.lang.ExceptionInfo: exec is not supported in non-GraalVM environments [at babashka/process.cljc:612:8]

expected behavior
exec should also work after ns reload.

thoughts
I am guessing that if bb exposed org.graalvm.nativeimage.ProcessProperties/exec this should work.

next steps
I'll experiment and follow up.

@borkdude
Copy link
Collaborator

right, making org.graalvm.nativeimage.ProcessProperties available in bb should solve that

Let's limit it to the "exec" method in the :custom map to save space

borkdude pushed a commit that referenced this issue May 13, 2023
* Support babashka.process reload for exec fn

Exposed GraalVM `ProcessProperties/exec` signature used by
babashka.process/exec.

Add new `graal?` feature (on by default) to allow folks to build/use
babashka without this specific Graal API.

On my linux dev box bb executable increased by 8kb.

Closes #1557

* Respond to PR review feedback

1. Allow all Graal ProcessProperties/exec signatures

2. Instead of a feature flag, simply check if Graal ProcessProperties
class is available before including ProcessProperties/exec.
I did not see the value of adding a has-graal-process-properties fn, so
left that part out. Lemme know if you want/need that.

* Respond to PR review feedback

Because resolves can bloat GraalVM native-image size, we like to keep
the together and obvious instead of buried and non-obvious.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants