search

clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

Add minimal tools.deps support #625

Closed DerGuteMoritz closed 2 years ago

DerGuteMoritz commented 2 years ago

Just enough to allow using aleph as a git dependency via tools.deps. Both the deps.edn file and build.clj's inputs are generated from project.clj which remains the source of truth for now. Can be used as a starting point for a potential full switch to tools.deps in the future.

Also included is a script for re-generating these files, as well as a CI check which should ensure that the generated files don't diverge. Note that these are intentionally not written as tools.deps or babashka tasks so as not to introduce additional dev and build dependencies.

FWIW, I've already tested this in practice, too.

Let me know if you have any ideas for improvement or disagree with e.g. the naming or the deps directory structure :pray:

KingMob commented 2 years ago

A couple people have asked for similar stuff over the years. I can't find my reply from the last time, but I'll sum up my stance like this: any switch to tools.deps must replicate all the functionality we currently have with lein before switching. Tools.deps has a few niceties like local/url/sha deps, but nothing that's a game-changer for aleph's needs.

There's also things that tools.deps lacks, and may never properly support, that we use. (E.g., setting global vars like *warn-on-reflection*. There's a hack involving :main-opts, but it breaks with anything else setting :main-opts).

That being said, I'm totally fine with an auto-generated deps.edn for users who don't want to use maven-based jars.

However, I'm not sure about the inclusion of the two deps/aleph/build.* files. It's not like we have a lot of devs to get confused about them yet, but they're not the official build tooling at the moment. I'd prefer to keep them out for now, unless you can think of a good reason to include them.

DerGuteMoritz commented 2 years ago

A couple people have asked for similar stuff over the years. I can't find my reply from the last time, but I'll sum up my stance like this: any switch to tools.deps must replicate all the functionality we currently have with lein before switching. Tools.deps has a few niceties like local/url/sha deps, but nothing that's a game-changer for aleph's needs.

Right, that's why I took the minimal approach here: Switching fully would be tremendous effort for little gain at the moment.

That being said, I'm totally fine with an auto-generated deps.edn for users who don't want to use maven-based jars.

However, I'm not sure about the inclusion of the two deps/aleph/build.* files. It's not like we have a lot of devs to get confused about them yet, but they're not the official build tooling at the moment. I'd prefer to keep them out for now, unless you can think of a good reason to include them.

This is actually a minimum requirement for making it usable as a git dependency (which was my main goal here): Since Aleph ships a handful of Java source files and Clojure doesn't transparently compile these like it does .clj sources, we need to provide a :deps/prep-lib task. IMHO, being able to use Aleph as a git dependency is desirable for allowing people to try out unreleased versions without going through the trouble of putting a custom built jar into a maven repo somewhere. Case in point: I was more annoyed by the prospect of that than by having to code up this patch here :smile:

How about I add a comment to deps/aleph/build.clj which explains this? deps/aleph/build.edn already has a comment at the top which at least should deter any future developers from touching it. Ah and I'll also add a paragraph about it to README.md. WDYT?

DerGuteMoritz commented 2 years ago

Good call, done! Also added myself to the main list of owners while at it :+1:

KingMob commented 2 years ago

CI might need rerunning. Can you merge, btw?

DerGuteMoritz commented 2 years ago

Yep, I can, but I can't trigger reruns of failed CI jobs. This reminds me I should give the CI check I added a shot in practice. Lemme just do that on here - will merge it myself afterwards if it works as intended!

KingMob commented 2 years ago

I restarted it for you.

@slipset Can you add @DerGuteMoritz to CircleCI for job-restarting perms?

DerGuteMoritz commented 2 years ago

Alright, the check failed successfully :smile: Merging away!

DerGuteMoritz commented 2 years ago

Failed test is https://github.com/clj-commons/aleph/issues/626 once more. Proceeding with the merge regardless.