clojure-emacs / orchard

A fertile ground for Clojure tooling
Eclipse Public License 1.0
323 stars 54 forks source link

Nuke support for Clojure 1.9 #245

Closed alexander-yakushev closed 2 months ago

alexander-yakushev commented 2 months ago

What do you say if we bite the bullet and drop 1.9? Short update on the reasons:

  1. 1.9 has significant incompatibilities with JDK11+. That's why we only test 1.9 with JDK8 in Orchard and cider-nrepl.
  2. 1.9 has been released in 2017. 1.10 has been released in 2018 (6 years ago).
  3. Upgrade path 1.9->1.10 is trivial AFAIK. No controversial changes like in 1.8->1.9 (spec, error messages, single JAR split).
  4. State of Clojure 2023 (1 year ago) had 1.9 usage at ~5%.
  5. 1.12 is around the corner.
vemv commented 2 months ago

What do you say if we bite the bullet and drop 1.9?

It would have been much nicer to ask first shoot second 🤠

Just over the last few weeks we were asked twice why is 1.8 unsupported.

While the argumentation that you provide is reasonable, the best way of operation is to start from a specific problem first.

If there isn't one, it seems a better spend of everyone's time (users, maintainers, contributors) to not risk breaking things, spending that time in bugfixes or features instead.

My 2c.

Thanks - V

alexander-yakushev commented 2 months ago

Just over the last few weeks we were asked twice why is 1.8 unsupported.

Like I said above, there are many more reasons to stay on 1.8 than on 1.9.

I am at the middle of a considerable inspector rework, so those 1.9 warts were getting in the way somewhat.

Then again, this can sit here and wait as long as 1.9 is needed.

alexander-yakushev commented 2 months ago

I don't mind trying to resurrect 1.8 if you think it is valuable. It's just that 1.9 is quite a meh version.

vemv commented 2 months ago

Please let's start from specific problems.

I'd like to see an isolated PR where you are trying to accomplish something. The build should be red in 1.9 and there should be commentary of why that happens and what the alternatives are (e.g. drop 1.9, apply workaround x).

Thanks - V

vemv commented 2 months ago

I am at the middle of a considerable inspector rework

Side note, I also don't know what problem are you solving. That's not to discourage you - but creating an issue takes very little time and would help us be on the same page.

The inspector has been around for some 13 years appararently so depending on the nature of the rework, it might be a good idea to create an alternative implementation if you planned, for instance, to do a near-total rework.

That kind of PR is much less demanding and risky than doing one refactor after another. e.g. "here's a nicer implementation, I've tried it locally over 2 months, it's a drop-in replacement".

bbatsov commented 2 months ago

It would have been much nicer to ask first shoot second 🤠

Not necessarily. It's often better to ask for forgiveness than for permission. 😄

The inspector has been around for some 13 years appararently so depending on the nature of the rework, it might be a good idea to create an alternative implementation if you planned, for instance, to do a near-total rework.

I have a very different perspective here and I much prefer incremental improvements over alternative implementations. @alexander-yakushev has been one of the main contributors to the inspector since it was merged into CIDER, so I have pretty high opinion of his judgement when it comes to changes there.

In general given the lack of much usage of Clojure 1.9 I think that dropping support for it is a very reasonable thing to discuss and I'm leaning towards pulling the plug on it. It simplified a bit the codebase and the test matrix, which is never a bad thing.

The PR looks great to me and I don't see anything concerning about it.

vemv commented 2 months ago

I'd be concerned about it not solving a stated problem, and forcing us to drop 1.9 compatibility before planned time, even when users have suggested otherwise.

I similarly highly trust @alexander-yakushev but I'd ask that we generously leave a trail of decisions and changes that can be meaningfully understood/reviewed.

We all make our whoopsies (Compliment breaking changes affected CIDER earlier this year) - it's cautious and respectful with users/maintainers to minimize the chances of those happening too frequently.

alexander-yakushev commented 2 months ago

Thank you for chiming in, Bozhidar!

Just to be clear, this PR is a request, not an obligation (like anywhere in open source). Consider this a place for discussion with a demo attached. I find it easier to discuss something when there is something tangible next to it – helps you see the exact benefits (or lack thereof).

I certainly don't require this merged for the work I'm doing to continue. Just opened it to gauge the interest and see if there is a consensus. If there isn't, this can wait indefinitely.

Regarding my work on the inspector, there are changes across many parts of the implementation, but nothing too drastic and the end user experience will not be altered. It also will be more safe and efficient to iterate quickly on this since I have time for an intense spike and plan to stick around for a bit to iron out the possible regressions. Which I cannot say about going the slow route.

alexander-yakushev commented 2 months ago

drop 1.9 compatibility before planned time

I was not aware that there is a criteria for deprecating Clojure versions, in that case I certainly would not rush this.

vemv commented 2 months ago

Cheers. I'd be happy to keep this PR around for a few weeks and intentfully opt to apply it if we observe that it's the best course of action.

bbatsov commented 2 months ago

drop 1.9 compatibility before planned time

I was not aware that there is a criteria for deprecating Clojure versions, in that case I certainly would not rush this.

There is a criteria and actually the current usage of Clojure 1.9 meets dropping support for it. See https://docs.cider.mx/cider/about/compatibility.html In particular this criteria:

In general we consider a Clojure release eligible for dropping once its usage drops below 5%, but we’d not drop support for any release just for the sake of doing it.

bbatsov commented 2 months ago

Just over the last few weeks we were asked twice why is 1.8 unsupported.

I guess it's mostly because we forgot to update the docs. (there it still says it's supported on https://docs.cider.mx/cider/about/compatibility.html)

In general backwards compatibility is a balancing act between the needs of the users and our own capacity. Given how our bandwidth hasn't really grown over the years I consider very carefully every opportunity to reduce the maintenance workload for us, when this is not going to be disruptive for the users. Dropping 1.9 has other implications as well - e.g. simplifying the error handling logic which diverged in Clojure 1.10.

bbatsov commented 2 months ago

I'll sleep on this, but so far my summary of the situation is:

Pros for merging:

On the downside:

As you might imagine I'm leaning strongly towards merging this and moving forward with a bit of extra cleanup here and there, as I don't see much risks here.

vemv commented 2 months ago

I wouldn't oppose merging today.

At the same time I want to contribute what seem to me guidelines that can make collaboration more streamlined - small, focused PRs that solve a problem that can be understood in advance.

I'd say that it's a relatively small thing to ask, that has objective benefits for everyone.

bbatsov commented 2 months ago

At the same time I want to contribute what seem to me guidelines that can make collaboration more streamlined - small, focused PRs that solve a problem that can be understood in advance.

No argument from me, although in general I try not to be dogmatic given that we're dealing with a huge influx of contributions.

@alexander-yakushev Please, add a changelog about this and I'll have the PR merged. I'd appreciate if you do a similar cleanup pass over cider-nrepl if you get the time to do so.

alexander-yakushev commented 2 months ago

Done!

bbatsov commented 2 months ago

Thanks!