Open publicimageltd opened 7 months ago
cc @rads
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.
I had a look at this, made a reproduction, and wrote a very long summary of what I learned:
Summary of what I learned:
--deps-root
argument when calling bb
, which crashes if deps.edn
is not present in the script root folder--deps-root
does not appear to be necessary, setting --config
to the script's deps.edn
file or bb.edn
file may be sufficient.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:
:paths ["src"]
when bb
is invoked with --config
.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)
With bb :paths
is always explicit, as to not mix JVM clojure sources with bb sources, so explicit seems best to me.
@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:
GitDir
and LocalDir
script types:
deps.edn
exists, continue using --deps-root
and --config
.bbin
.deps.edn
does not exist, look for bb.edn
and use only --config
.: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
.bb.edn
file is not required.deps.edn
nor bb.edn
exist, throw an exception.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.
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
Good point. I'll fix that to say that we don't want to add inference rather than adding a strict check.
@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.
I think that it might be a good idea to move away from string-based selmer templates for the shims to code as data.
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)
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?
I have been working on a babashka project with the following
bb.edn
(the script's name issecrets
):Installation via
bbin install .
works, but when I call the resulting script, a classpath not found error is raised. So I added an emptydeps.edn
:Now the reinstalled script works. But why is that necessary? The
deps.edn
is useless, since I am already declaring the dependencies in thebb.edn
file and do not callclj
. So I expectedbbin 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.