Do not require deps.edn when using local bbin install

[publicimageltd]

I have been working on a babashka project with the following bb.edn (the script's name is secrets):

{:paths ["src"] ;; library
 :deps {com.taoensso/timbre {:mvn/version "6.4.0"}}
 :bbin/bin {secrets {:main-opts ["-m" "secrets.core/-main"]}}
}

Installation via bbin install . works, but when I call the resulting script, a classpath not found error is raised. So I added an empty deps.edn:

{:paths ["src"]}

Now the reinstalled script works. But why is that necessary? The deps.edn is useless, since I am already declaring the dependencies in the bb.edn file and do not call clj. So I expected bbin install to work autonomously.

So if I did not miss something, I think it would be better to drop this dependency. Not least because jacking into the script with CIDER causes unnecessary confusion (I have to choose "clojure or babashka?"). If that will not be changed, I would recommend at least to document that a deps.edn is also necessary.

[borkdude]

cc @rads

[NoahTheDuke]

I've run into this issue another way: I have a deps.edn that has a dependency that doesn't work with babashka, so in my bb.edn, I don't depend on the local deps.edn, I just specify :paths and use deps/add-lib in my script to bring in a babashka compatible version:

#?(:bb (do (require '[babashka.deps :as deps])
           (deps/add-deps '{:deps {io.github.babashka/tools.bbuild
                                   {:git/sha "f5a4acaf25ec2bc5582853758ba81383fff5e86b"}}}))

If I run the script with babashka, this works, but if I run it with bbin's generated script, it doesn't because it pulls in the incompatible version through the deps.edn first.

[teodorlu]

I had a look at this, made a reproduction, and wrote a very long summary of what I learned:

https://github.com/teodorlu/bbin-testdata/tree/72934325b97ce431c57ef0c581b4ed1aeedf70af/fail1/README.md

Summary of what I learned:

1. The present version of bbin uses the --deps-root argument when calling bb, which crashes if deps.edn is not present in the script root folder
2. From limited testing, --deps-root does not appear to be necessary, setting --config to the script's deps.edn file or bb.edn file may be sufficient.
3. A modified bbin that uses bb --config SCRIPT_DEPS_FILE will crash if the deps.edn file or bb.edn file does not set :paths explicitly, :paths ["src"] is not inferred.

For 3 I see two options:

1. Change babashka to infer :paths ["src"] when bb is invoked with --config.
2. Require bbin script authors to set :paths explicitly if they want to use bbin to ship their scripts.

(See heading in linked document: https://github.com/teodorlu/bbin-testdata/blob/72934325b97ce431c57ef0c581b4ed1aeedf70af/fail1/README.md#question-for-borkdude-should-classpath-be-inferred-for-bb---config-some-deps-edn-or-bb-edn-file)

[borkdude]

With bb :paths is always explicit, as to not mix JVM clojure sources with bb sources, so explicit seems best to me.

[rads]

@teodorlu: It's been a while since I looked at this code in detail so I appreciate the overview. You've already started moving towards the right direction in your PR, but I'll summarize my suggested plan here for clarity:

• The following changes are only relevant to the GitDir and LocalDir script types:
  • If deps.edn exists, continue using --deps-root and --config.
    • This ensures that all existing projects that are already working maintain exactly the same behavior as before, even if reinstalled with the new verison of bbin.
  • If deps.edn does not exist, look for bb.edn and use only --config.
    • This case was broken anyways which means it's safe to add new behavior here.
    • The :paths in the bb.edn must be set explicitly, otherwise throw an exception. (see comment below)
    • bbin should use the bb.edn file as-is and not add any inference for :paths.
    • Validation of the bb.edn file is not required.
  • If neither deps.edn nor bb.edn exist, throw an exception.
• It would be ideal for bbin to provide friendly error messages when it detects these cases rather than letting the bb command fail.

If the above points look good to you and @borkdude, then we can move forward with finalizing the implementation.

[borkdude]

Looks good, except maybe for this one?

The :paths in the bb.edn must be set explicitly, otherwise throw an exception.

Why throw an exception in this case, one could have a valid bb.edn without :paths, e.g. with a local/root dep or whatever

[rads]

Good point. I'll fix that to say that we don't want to add inference rather than adding a strict check.

[borkdude]

@teodorlu

If the above points look good to you and @borkdude, then we can move forward with finalizing the implementation.

I think it all looks good, except the throwing.

[teodorlu]

I think that it might be a good idea to move away from string-based selmer templates for the shims to code as data.

1. With code as data, we can get structural editing and syntax highlighting from the editor
2. With code as data, we can also avoid duplicating code, not have to solve the same problem in two places.

To get pretty-printed code, we could use something like clojure.pprint/code-dispatch (example).

@rads / @borkdude thoughts on this?

(Just saying this now, because I felt it made the last "chunk" of work harder than necessary)

[borkdude]

I'm fine with this, but it seems like a different issue to me, let's not try to do too many changes at once, unless it makes your life easier in the PR?

[borkdude]

@teodorlu What's necessary to wrap this issue up? I think it was mentioned in the bbin channel on Clojurians slack once again

[teodorlu]

Hey!

It's been a while since I last looked at this. I lost a bit of steam working on the Clojure code we generate by interpolating values into strings. I also remember feeling slightly uneasy changing the generated Clojure code fearing Windows regressions. Maybe first refactoring to generate Clojure code as Clojure data is easier.

If you want to take a stab (possibly you'll just steam through in an hour as you sometimes do), feel free!

Otherwise I'll give it another shot in a day or two.