Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [rdf4j-dev] Source and binary compatiblity checks (was: Re: Broken build)
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 16 May 2022, at 23:58, Jeen Broekstra <jeen@xxxxxxxxxxxx> wrote:


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.

Japicmp is complaining about a new public static method on an interface from this PR: https://github.com/eclipse/rdf4j/pull/3800

I've intended to update the japicmp settings to allow for new methods to interfaces that have a default (or static) implementation. https://github.com/eclipse/rdf4j/blob/7db975d5e686bbd1a8d7b8d60c650966adbaea81/pom.xml#L904


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:
  1.  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;
  2. 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@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/rdf4j-dev

Back to the top