l3nz / cli-matic

Compact, hands-free [sub]command line parsing library for Clojure.
Eclipse Public License 2.0
361 stars 29 forks source link

Remove core.async from deps.edn #114

Closed aviflax closed 3 years ago

aviflax commented 4 years ago

I was working on compiling my project to a native executable using GraalVM and I was having some trouble — it was requiring a LOT of memory and a LOT of time. (For example I was having trouble building my tool with native-image on GitHub Actions, as the runners provided by GitHub are limited to 7GB of RAM.)

So I decided to look at my project’s dependency tree, and I noticed that cli-matic seemed to have a rather subtree. core.async caught my eye immediately; it wasn’t immediately obvious why cli-matic would need core.async.

So I poked around in the cli-matic repo to try to see what core.async was used for, and I found optionals.clj. The docstring in the ns form doesn’t like core.async, but there’s a section at the bottom that’s about core.async.

So it seemed to me that core async was supposed to be optional. My next question was: if it was optional, why was it being included in my project. First I checked out deps.edn because my project uses tools.deps, and saw, unsurprisingly, that core.async was right in there.

Next I checked project.clj and saw this:

[org.clojure/core.async "0.5.527" :scope "provided"]

seeing that :scope "provided" led me to hypothesize that when the file deps.edn was created, core.async was transferred into it inadvertently, given that deps.edn has no equivalent to Maven’s provided scope.

So it seems to me that core.async should probably be removed from deps.edn so that, for projects that use tools.deps, core.async would be actually optional.

So I tried it, and the results seemed positive. With core.async and its dependencies removed from my project, I was suddenly able to build my project on GitHub Actions with 6GB of RAM allocated to GraalVM, and my build times on my machine dropped significantly.

This change seems to me to be a good thing — given that cli-matic is all about creating CLI programs with Clojure, and that such programs have a significantly better UX when compiled with GraalVM’s native-image tool, this change seems like a no-brainer to me.

l3nz commented 4 years ago

Hi @aviflax I'm 100% with you, core.async is in there just because you may want to return a value with it, but cli-matic works without. You are supposed to consume cli.matic from the JAR/POM, deps.edn is just for my own local testing of "examples" scripts, and so it is a kitchen sink of all configs.

aviflax commented 4 years ago

@l3nz thanks for explaining, that makes sense!

So I guess this is sort-of a feature request: I’d like to be able to use the library as a “gitlib” — this is a capability of tools.deps wherein a project can specify the coordinates of a dep using :git/url and :sha and tools.deps will clone the dep via Git, then add the local clone to the classpath.

In general, I prefer to “consume” libraries as gitlibs (example) — I can elaborate why, if you’d like — and that’s how I got into this mess in the first place: I was looking at the project on github.com and I noticed that it had a deps.edn file. I was happy to see that (🙂) and I assumed (🤦) that the file was suitable for use when consuming the project as a gitlib.

So I hope you will consider a minor change to the file so that we can (hopefully) support both use cases — local testing, and consuming the project as a gitlib.

Off the top of my head, the simplest approach would be, I think, to move core.async to an alias in the deps.edn, and then you’d use that alias when testing locally. e.g. instead of running clojure -m my.test.runner you’d run clojure -R:optionals -m my.test.runner

Here’s an example; I’ve taken the liberty of adding all the optional libs, and I’ve also moved the specific versions of Clojure and spec to a test alias, which just makes sense to me.

For example ```clojure {:deps {org.clojure/tools.cli {:mvn/version "0.3.7"} expound {:mvn/version "0.7.1"}} :aliases {:test {:extra-deps {org.clojure/clojure {:mvn/version "1.9.0"} org.clojure/spec.alpha {:mvn/version "0.1.143"}}} :optionals {:extra-deps {org.clojure/core.async {:mvn/version "0.5.527"} orchestra {:mvn/version "2018.12.06-2"} cheshire {:mvn/version "5.8.1"} io.forward/yaml {:mvn/version "1.0.9"}}}}} ```

I could then modify all the scripts in examples to add -R:test:optionals to the clojure command… I think that should work? The only thing I’m not sure about here is: I just noticed that in the dir examples there’s another deps.edn … isn’t that the one that’s being used when those scripts are executed? If so, could we / should we perhaps limit the deps.edn in the root to the required dependencies?

Sorry for the wall of text — I appreciate your time and attention! Thank you!

aviflax commented 4 years ago

BTW, one more thing: I just noticed that the README currently says, near the top, under Using:

The library is available on Clojars:

Or the library can be easily referenced through Github (make sure you change the commit-id): ```clojure {:deps {cli-matic {:git/url "https://github.com/l3nz/cli-matic.git" :sha "374b2ad71843c07b9d2ddfc1d4439bd7f8ebafab"}}} ```

Which would seem to imply that using this library as a gitlib via tools.deps is a fully-supported method of usage. This doesn’t seem to jive, to me, with the deps.edn file bringing in deps that are supposed to be optional.

I’d be happy to help fix this, either along the lines I suggested above, or however you prefer; please let me know if you have any interest.

Thank you!