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

Switch to lein2deps #635

Closed DerGuteMoritz closed 2 years ago

DerGuteMoritz commented 2 years ago

Also went ahead and included a switch to the new recommended cimg CircleCI images. Lemme know if you'd prefer that as a separate PR.

arnaudgeiser commented 2 years ago

My 2 cents: I would prefer the automation on the CI if possible.

arnaudgeiser commented 2 years ago

So basically, replacing deps/ensure-deps-up-to-date [1] implementation by something that would regenerate the deps.edn (instead of throwing an error) file.

Either by:

I don't know how difficult is the latter, but I would prefer that way to keep the history clean.

In any case, I don't have any strong opinion on this subject...

[1] : https://github.com/clj-commons/aleph/pull/625/files#diff-78a8a19706dbd2a4425dd72bdab0502ed7a2cef16365ab7030a5a0588927bf47R40

KingMob commented 2 years ago

What's the latest word with this?

DerGuteMoritz commented 2 years ago

Ah sorry, dropped the ball on this over vacation :smile: I am slightly against automating this further for the following reasons:

Adding an additional commit with only the deps.edn file change

This makes the change non-atomic (see item 3 in https://github.com/clj-commons/aleph/blob/master/CONTRIBUTING.md#updating-dependencies).

Updating deps.edn as part of the previous commit

Seems iffy to modify a commit like this, especially when it's coming from a third party.

A pre-commit hook wouldn't have the above issues but there's no guarantee that people will actually run it, so the CI check would still have to remain. I also tend to find pre-commit hooks a bit annoying since they add latency to an interactive command (in this particular case considerable latency when it's invoked for the first time because it needs to download dependencies). @arnaudgeiser also stated above that he would prefer it be a CI job.

Can anyone think of another alternative we haven't considered?

Given that dependency bumps are rather rare, I think doing one extra roundtrip against CI is tolerable. WDYT?

DerGuteMoritz commented 2 years ago

Oh yeah, I'm actually not strongly opposed to relaxing the "atomic change" rule. However, I have no experience in setting up a CircleCI job to allow it to push commits so this would require extra effort I can't expend right now. We could just file an issue about this, tho!

arnaudgeiser commented 2 years ago

On my side, it was just a proposition. I don't really think it's worth spending time on it. Our time is better invested doing something else.

KingMob commented 2 years ago

I don't want to hold it up, and I agree it's not high priority.

Still, it really feels like this is something computers should be able to do 😄

Approving for now

DerGuteMoritz commented 2 years ago

Still, it really feels like this is something computers should be able to do smile

I feel you :smile: But yeah, probably not worth pushing on this case.

That said, I just thought of a potential way to achieve tools.deps compat without pre-generating a deps.edn file: Add a (static) dependency on a :local/root which points to a non-existing subdir and is populated by a :deps/prep-lib task which bascially just runs lein2deps. Drawback is that we'd push a bb dependency onto the user but I think it should also be possible to run lein2deps via clojure (possibly requires some tweaks). Seems a bit hacky, tho. Will give it a try anyway when I find a minute to spare :smile:

Merging away!