Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nuke support for Clojure 1.9 #245

Merged
merged 2 commits into from
Apr 23, 2024
Merged

Nuke support for Clojure 1.9 #245

merged 2 commits into from
Apr 23, 2024

Conversation

alexander-yakushev
Copy link
Member

@alexander-yakushev alexander-yakushev commented Apr 22, 2024

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
Copy link
Member

vemv commented Apr 22, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

vemv commented Apr 22, 2024

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
Copy link
Member

vemv commented Apr 22, 2024

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
Copy link
Member

bbatsov commented Apr 22, 2024

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
Copy link
Member

vemv commented Apr 22, 2024

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
Copy link
Member Author

alexander-yakushev commented Apr 22, 2024

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
Copy link
Member Author

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 vemv marked this pull request as draft April 22, 2024 14:30
@vemv
Copy link
Member

vemv commented Apr 22, 2024

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
Copy link
Member

bbatsov commented Apr 22, 2024

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
Copy link
Member

bbatsov commented Apr 22, 2024

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
Copy link
Member

bbatsov commented Apr 22, 2024

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

Pros for merging:

  • the PR seems safe enough
  • Clojure 1.9 meets our criteria to drop support for it
  • The migration path between 1.9 and 1.10 is trivial, so I doubt many users haven't migrated already
  • It would unlock further cleanup possibilities in cider-nrepl and CIDER itself
  • Sashko has some time for an intense spike on the inspector now, but if we take the slow route he might not be able to drive his ideas to completion
  • I recall when we had broken 1.9 compatibility in cider-nrepl pretty much no one noticed this :-)

On the downside:

  • something might break (although supposedly we have decent tests)
  • someone might complain about the change (always a possibility)

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
Copy link
Member

vemv commented Apr 22, 2024

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 bbatsov marked this pull request as ready for review April 23, 2024 08:10
@bbatsov
Copy link
Member

bbatsov commented Apr 23, 2024

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
Copy link
Member Author

Done!

@bbatsov bbatsov merged commit bb22fb3 into master Apr 23, 2024
55 checks passed
@bbatsov
Copy link
Member

bbatsov commented Apr 23, 2024

Thanks!

@bbatsov bbatsov deleted the farewell-1.9 branch April 23, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants