brettwooldridge / HikariCP

光 HikariCP・A solid, high-performance, JDBC connection pool at last.
Apache License 2.0
19.91k stars 2.92k forks source link

What's the breaking change? #2127

Closed OrangeDog closed 11 months ago

OrangeDog commented 11 months ago

Looking at the changelog, it's not clear why the bump to version 6.0.0.

Which change breaks compatibility with something?

lfbayer commented 11 months ago

Because it's been a long time and I don't know the specific rules that govern what would require a bump. There was a user in the past that said if you change the class signature it would break tools, Brett had worked through that case.

This is my first time, I'm just trying to release something as it has been so long. There have been many hurdles to finish this. (I'm now waiting for approval on the new sonatype (s01) repository because apparently even sonatype has changed since the last release).

Honestly I just want to release something and I don't want to break anything.

OrangeDog commented 11 months ago

So what has been changed in the "class signature"?

lfbayer commented 11 months ago

Possibly nothing has changed. But out of an abundance of caution, instead of having to identify what would break things, I figured it is better to just assume that something has changed a rev the major version. In future releases I will pay attention and try to follow semantic versioning rules and not rev the major version without reason. But for now, I figured it is less risk to rev the major than to not.

So to repeat what I was trying to say in my first comment, "I don't know."

lfbayer commented 11 months ago

The one change that had me most concerned, and most unsure if it was ok to not rev the major version, was that the pom was changed to expect Java 11 only. I assume that the existing build artifacts also only supported Java 11. I wondered about the case where if an existing user might be on java 9, a patch version update alone might cause them to get the latest artifact. Maybe that would be ok, but I didn't want to put the effort in to comprehend that scenario.

The other thing is that a public static inner class was added to a public class (UtilityElf.CustomDiscardPolicy), which I assume doesn't change the signature of the outerclass, but I don't know what these tools are that caused a problem before, so it seemed plausible that that change could cause a similar problem.

OrangeDog commented 11 months ago

Neither change affects anything. <target>11</target> was already added in 5.0.0. The contents of a class are not part of its signature.

If that change breaks this mystery tool, then that is the fault of that tool for doing unsupported reflection stuff.

If you're going to be managing this project now, you need to learn some of the basics of Maven and Java APIs.

lfbayer commented 11 months ago

This is the specific change to the pom.xml that I was uncomfortable with: https://github.com/brettwooldridge/HikariCP/commit/a050d81004265567a92af5162e164a73823ee0b2#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8L561

Also, please be kind. Realize that I'm a human and I'm just trying to help.

A change to the major version won't hurt anyone. If anything it is a symbol of other non-code changes. semver doesn't say that you cannot change a major version without a breaking API change.

OrangeDog commented 11 months ago

Removing the profile? Yes it has no effect. It's a fix for having forgotten to do it in 5.0.0.

The majority of projects will not update to get these minor fixes, because you're advertising it as a breaking change.

What semver says is:

increment the MAJOR version when you make incompatible API changes

There's no other situation where it says to increment the major version.

lfbayer commented 11 months ago

While semver does say that the major version must be incremented for incompatible API changes, that doesn't mean that it must not be changed if there are not incompatible changes. This was a conscious decision on my part. The choice to rev the major version wasn't a "I don't know how semver works". It was a "I'm not 100% absolutely certain that in some strange case a new inner class doesn't break something downstream". And I will admit I don't entirely know the impact of a maven profile and I intentionally made a decision to not learn as I trust Brett implicitly, meaning that I knew the change was safe, but I didn't know what its impact on "compatibility" was.

My focus was on releasing and I thought a major version change was the least dangerous thing to do to get HikariCP moving again. That was a risk assessment judgement call. Based on my own assessment I also thought there wasn't any incompatible change. But concerned about the possible implications in the reflection case, and frankly not understanding clearly the implications of the profile change, I thought it was better to just release with the major version change now than to wait another six months to find the time and spirit to dig deep and understand the implications of that change.

Given that there were some bug fixes in the last 1.5 years that have been sitting around not released I assumed everyone would be rooting me on. And if a major version meant that some people wouldn't update, that doesn't seem like too terrible of a problem.

Luckily, the release hasn't actually been performed yet, so there is still a chance to use 5.0.2 for the version number.

Maybe I should have interpreted your original comment as "By the way, you don't actually need to rev the major version" rather than the subtly accusatory "What's the breaking change?"

OrangeDog commented 11 months ago

No, the original comment was based on the assumption that the maintainers knew what they were doing and I didn’t know enough about the project to see where the breaking change was.

5.1.0 would be the better version for this.

Spring for example has a policy of no major dependency updates during their major branch, and getting an exception through there because a project stopped using semver correctly has been very difficult in the past.

patric-r commented 11 months ago

6.0.0 isn't released, is it?

And: Where's the changelog for the released 5.1.0?

lfbayer commented 11 months ago

@patric-r Nothing has been released yet, I'm trying to work through some changes to pom dependencies that are getting in the way of publishing the release. Currently having fun with gpg signing. I haven't spent any time on this for the last few days so it may be a little bit longer before the release is finished.

At this point the expected release version is 5.1.0, there won't be a 6.0.0 this time.

Also thanks for pointing out the changelog, I'll update the version in that before I finish the release.

patric-r commented 11 months ago

@lfbayer Have you noticed that 5.1.0 has already been "tagged" (click on releases then select 'tag')?

lfbayer commented 11 months ago

@lfbayer Have you noticed that 5.1.0 has already been "tagged" (click on releases then select 'tag')?

Yes I have. This is because the tag stage of the maven release plugin happens before the signing phase. I'm very very tempted to do some heavy force pushing to clean up the history. So a lot of that will likely go away. First step is to just get the build working to actually be able to publish an artifact up to sonatype again.

Since nothing is officially published on maven yet, it doesn't much matter what the current state is.