babashka / bbin

Install any Babashka script or project with one command
MIT License
139 stars 9 forks source link

Don't require `deps.edn` for local scripts #85

Open teodorlu opened 5 months ago

teodorlu commented 5 months ago

Please answer the following questions and leave the below in as part of your PR.

teodorlu commented 5 months ago

With the bbin changes in 64025cc, I can install the fail1 script from https://github.com/teodorlu/bbin-testdata/tree/e8c5a94f042a73076010d04948139c2590a313ab/fail1/ without issues:

$ git rev-parse HEAD
e8c5a94f042a73076010d04948139c2590a313ab
$ pwd
/Users/teodorlu/dev/teodorlu/bbin-testdata/fail1
$ bbin-dev install .

Starting install...

bin    location                                               
─────  ───────────────────────────────────────────────────────
fail1  file:///Users/teodorlu/dev/teodorlu/bbin-testdata/fail1

Install complete.

$ fail1
{:script "fail1", :args nil}
borkdude commented 5 months ago

@teodorlu Cool. Perhaps bb.edn should be prioritized over deps.edn, what do you think?

rads commented 5 months ago

Thanks for the PR, taking a closer look now.

teodorlu commented 5 months ago

@teodorlu Cool. Perhaps bb.edn should be prioritized over deps.edn, what do you think?

Yeah, that's probably the right choice! I'll make the change.

teodorlu commented 5 months ago

Thanks for the PR, taking a closer look now.

Note: currently, this is just a start! But I wanted to put some code up so that we can discuss real code rather than hypothetical code.

rads commented 5 months ago

Note: currently, this is just a start! But I wanted to put some code up so that we can discuss real code rather than hypothetical code.

Makes sense. The main thing I'm thinking about is backwards-compatibility. Whatever change we make, I want to ensure we're only adding behavior and existing users don't have to do anything differently.

teodorlu commented 5 months ago

Makes sense. The main thing I'm thinking about is backwards-compatibility. Whatever change we make, I want to ensure we're only adding behavior and existing users don't have to do anything differently.

100 % agreed! Let's not break userspace.

teodorlu commented 5 months ago

Some observations while working on this code, probably out of scope for this PR:

  1. I wonder if creating the tmp-edn file is actually required. It seems it's not required when a deps.edn file or a bb.edn file exists.

    (def tmp-edn
      (doto (fs/file (fs/temp-dir) (str (gensym \"bbin\")))
        (spit (str \"{:deps {\" script-lib script-coords \"}}\"))
        (fs/delete-on-exit)))
  2. I wonder if script shims would be easier to work with as Clojure data rather than string template literals, getting the Clojure code right inside the string literals can be a bit hard.

(I may be missing things, as I've only focused on scripts installed from local directories)

rads commented 5 months ago

@teodorlu: I agree that those are out-of-scope, but you're welcome to create issues for those and anything else that comes up.