babashka: Add wrapper #241119
babashka: Add wrapper #241119
Conversation
ofborg
bot
added
11.by: package-maintainer
There was a problem hiding this comment.
The idea is good, but I don't think this is a good approach. Since the wrap is done in the same derivation as the program, if we disable the wrapper we end up needing to recompile the whole babashka.
I would prefer this being a separate derivation, like the mpv or neovim packages, where we have babashka-unwrapped to be the non-wrapped derivation and babashka is the wrapped one. This way, the wrap can even be modified (e.g. using another JDK) without triggering a recompilation.
|
I think the unwrapped/wrapped suggestion seems smart. I also think having convenience built in to the main package would be nice, so I support the idea of this PR. Thanks for working on this! |
|
@thiagokokada It makes sense, I added a It's not clear in the git diff, but the I also noticed that we can remove the |
| seems to hit the sweet spot for those cases. | ||
| installPhase = | ||
| let unwrapped-bin = "${babashka-unwrapped}/bin/bb"; in | ||
| '' |
There was a problem hiding this comment.
Can you add the unwrapped package as passthru? See the mpv package for example.
There was a problem hiding this comment.
You mean just passthru.unwrapped = babashka-unwrapped?
|
Also please fix eval error. |
jlesquembre
force-pushed
the
bb-wrapper
branch
2 times, most recently
from
ce20017
to
74d4215
Compare
| @@ -0,0 +1,99 @@ | |||
| { lib | |||
There was a problem hiding this comment.
Can we reorganize this in a subdirectory babashka? And then this is the unwrapped.nix file while the wrapper is default.nix.
There was a problem hiding this comment.
Or this is the default.nix and the other is the wrapper.nix. Not sure how is the common way to organize this.
mpv is again a good example.
There was a problem hiding this comment.
Makes sense, I went with default.nix for the unwrapped version, looking at mpv and neovim, seems the convention.
|
We also have |
|
Please mark as broken. I need to devote a little time next month to try to get it working again! |
To resolve dependecies in `bb.edn` files, Babashka depends on some Clojure files and Java, see: https://github.com/borkdude/deps.clj/tree/master#deps_clj_tools_dir The wrapper also adds rlwrap, as recommended in the Babashka documentation: https://book.babashka.org/#_repl I'm also formatting the file with nixpkgs-fmt
|
@ofborg eval |
|
Result of 4 packages built:
|
|
If anyone is interested in the impacts of the wrapper in derivation size: |
Adding java doesn't help: |
|
thanks @thiagokokada ! |
To resolve dependecies in
bb.ednfiles, Babashka depends on some Clojure files and Java, see:https://github.com/borkdude/deps.clj/tree/master#deps_clj_tools_dir
The wrapper also adds rlwrap, as recommended in the Babashka documentation:
https://book.babashka.org/#_repl
Adding Clojure and the JDK to the derivation increases considerably the closure size. I'm adding a boolean,
wrapBinary, to make easier to disable the wrapper, and reduce the closure size. That could be useful to, for example, create a Docker image.I'm also formatting the file with nixpkgs-fmt.
Description of changes
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)