| Hi,
I haven’t considered the binary compatibility side. Might still be binary compatible if the user code only refers to the interface, but hard to know for sure to be honest. Definitely wouldn’t be binary compatible (or source compatible) if users are referring to the old superclass.
Target interfaces are never statically guaranteed by the JVM verifier; every invokeinterface receiver is typed as a simple object reference. Therefore (unlike invokevirtual calls), no assumptions can be made about the receiver's vtable layout. Instead, the receiver's class (as represented by its _klass field) must be checked more carefully. Where a virtual call can blindly perform two or three indirections to reach the target method, an interface call must first inspect the receiver's class to determine (a) if that class actually implements the interface, and (b) if so, where that interface's methods are recorded within that particular class.
An alternative, to keep things binary compatible, would be to duplicate and deprecate the old classes. I’m a fan of this approach for complex classes where there is a need for a large rewrite. The downside is that we have a lot of code hanging around until the next major release and any users who are using the existing classes need to switch over to the new ones manually.
I would take you up on dropping binary compatibility for minor releases. Minor releases bring new features and improvements that hopefully make up for any extra work.
I’ll see if Ontotext has a strong stance on this.
Håvard On 24 May 2022, at 00:36, Jeen Broekstra <jeen@xxxxxxxxxxxx> wrote:
I haven't tested (not thought this through fully) but I think what you propose will break binary compatibility no matter how careful we are: if you change the superclass the JVM will just not link up with the correct method signature in the subclass anymore in the recompiled library - even if the client code does not refer to the original superclass. Again: I'm not sure, but we should be able to test this out.
If that is indeed the case, then I think we should not put this in as another "special case" or exception. Instead we could agree to just completely drop the binary compatibility guarantee for minor releases, and instead only guarantee source compatibility.
It is probably a simpler message to our users anyway to say: "when upgrading to a minor release we recommend you recompile your application", instead of: "when upgrading to a minor release you won't have to recompile your application unless you refer to this class/interface anywhere in your code, or have (insert other special case) - oh and be careful because you may not immediately find out if it works or not". Not that we ever explicitly say any of this :)
Jeen
On Tue, 24 May 2022, at 03:31, Håvard Ottestad wrote:
Hi,
TLDR: I’m proposing we allow changing which superclass a class extends in a minor release, as long as we agree that the users shouldn’t rely on that superclass declaration.
I’m running into a need to change the superclass of some classes. Essentially it’s for the query optimizers where they all extend an abstract class in order to inherit a basic implementation. I can’t see that anyone would be instantiating the optimizers based on that abstract class, especially since the abstract class implements an interface.
Changing superclasses could potentially be a breaking change if objects are referred to as instances of that superclass. The classic example is Animal sheep = new NorwegianSpottedMountainSheep();
It’s not always clear cut when a superclass could or should be used by our users. I think we would have to consider every case separately. As a rule of thumb though I would say it’s ok for most cases where there is a clear interface at the top level and the superclass would be considered an intermediary class. MemLiteral would be a good example, Literal is the clear top level interface and users shouldn’t rely on MemLiteral extending SimpleLiteral .
I would like to propose that we allow the removal of superclasses in minor releases, but still rely on our judgement of whether the change is backwards compatible or not.
Cheers,
Håvard
On 17 May 2022, at 10:36, Håvard Ottestad <hmottestad@xxxxxxxxx> wrote:
Hi,
To help out with the language:
- patch = 4.0.1
- minor = 4.1.0
- major = 5.0.0
I want to point out that we can still revert the changes I made if we want to follow semver more strictly. I pushed things through so that the build wouldn't be broken and felt that it was cleaner to change the japicmp config rather than revert any code.
If we want to strictly follow SemVer then I propose that we do major releases more frequently. Maybe we could have three branches, main, minor and major instead of main and develop. We can set up a github action to keep them in sync like we have for main --> develop today.
I am still quite annoyed at Elasticsearch for moving around packages in minor/patch releases. It completely blocked me from upgrading Spring in a project I had at work because I was using the Elasticsearch client through both Spring Data and RDF4J.
Even though it's a pain to keep things backwards compatible and follow SemVer I definitely see the benefit to our users!
My preference is that we continue our approach from RDF4J 3.x.x and allow new public and protected interface/class methods with a default implementation or a static implementation for minor releases. For patch releases we do not allow this and follow strict binary and source compatibility.
Cheers,
Håvard
On Mon, 16 May 2022, at 23:24, Håvard Ottestad wrote:
Hi,
Our japicmp settings are stricter then they used to be and that is why the build is broken.
Thanks for fixing this Håvard, and apologies that my original response was a bit cranky (I blame insufficient caffeine).
Let's talk about compatibility a little, more generally - it might be good to revisit some of the thinking behind this, also for people newer to the project.
Japicmp is a tool that helps us guard against putting in any changes that are backward incompatible in patch or minor releases (in accordance with the definitions of SemVer). Before we go into the specific settings of the japicmp, let's discuss what we actually want to achieve, guarantee-wise.
Backward compatibility in Java typically breaks down in two types: source compatibility, and binary compatibility.
Binary compatibility is the strongest kind of guarantee. Put simply, it guarantees that the new version is a drop-in replacement for the older version, and the client application can immediately use it, without the need to be recompiled.
Source compatibility means that the new version can be used by existing applications without having to make code changes - but the application will need to be recompiled.
For RDF4J patch releases, binary compatibility is a very desirable trait: we want our users able to drop jar file 4.0.2 over the top of 4.0.1 and not have to recompile their app. We've previously also made this guarantee for minor releases. However, we allow a few exceptions:
- we make classes/methods annotated with @InternalUseOnly or @Experimental exempt from the compatibility checks. This is true for either patch or minor releases - though the advice is not to use this as part of a patch fix;
- the japicmp tool by default considers adding a new default method in an interface not binary (or even source!) compatible, and only allows it as part of a major new release. We previously put a config override in place to mark this as source+binary compatible and allowed in minor releases (this is what Håvard just restored).
I'm not entirely sure why japicmp considers new default methods incompatible, though I suspect it has to do with erring on the side of caution and there being corner cases such as described in these articles:
What it comes down to is that we make a judgment call to use new default methods with due care. While a new default method does not necessarily break binary compatibility, if done unwisely (e.g. interfering with an existing method) it can lead to runtime-errors.
The thing to keep in mind, of course, is that all that japicmp does is just guide-rails, and not a complete guarantee of compatibility. There are loads of changes that we could put in that would satisfy the japicmp checks but would completely break an existing application.
Cheers,
Jeen
_______________________________________________
rdf4j-dev mailing list
_______________________________________________
rdf4j-dev mailing list
_______________________________________________rdf4j-dev mailing listrdf4j-dev@xxxxxxxxxxxTo unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/rdf4j-dev
|
