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

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

teodorlu
Copy link
Contributor

@teodorlu teodorlu commented Jun 19, 2024

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

@teodorlu
Copy link
Contributor Author

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
Copy link
Contributor

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

@rads
Copy link
Collaborator

rads commented Jun 19, 2024

Thanks for the PR, taking a closer look now.

@teodorlu
Copy link
Contributor Author

@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
Copy link
Contributor Author

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.

  • I only changed the behavior for installing script from a local directory
  • I haven't added any tests yet

Rationale: `bb.edn` is _meant_ to control babashka-things, and may
_optionally refer_ to `deps.edn` for additional dependencies.
@rads
Copy link
Collaborator

rads commented Jun 19, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

teodorlu commented Jun 19, 2024

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
Copy link
Collaborator

rads commented Jun 19, 2024

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

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 this pull request may close these issues.

3 participants