Add revapi maven plugin to build system to check API.

[sbernard31]

The idea is to add tooling to check API break using revapi.

For now I only push this modification in master (2.1?) branch.
If we agree about using this tool, we should also include this in the future 2.0.x branch.

revapi will fail the build if API break is detected.
I configure it to :

• uses semantic versioning.
• compare against 2.0.0 version.
• excludes all the demo-apps projects.
• ignore unchanged Non-Public Part of API

About the last point, I think this is not good practice and we should fix this Non-Public Part of API instead. Here 1 example :

The following API problems caused the build to fail:
[ERROR] java.class.nonPublicPartOfAPI: class org.eclipse.californium.elements.util.Asn1DerDecoder.OidEntityDefinition: 
Class 'org.eclipse.californium.elements.util.Asn1DerDecoder.OidEntityDefinition'
is indirectly included in the API (by the means of method return type for example) 
but the class is not accessible (neither public nor protected). 
(breaks semantic versioning)

I also add some commented samples about classes or packages filtering but I still think that the right way is to defined a package naming convention. This will make the configuration simpler and will allow us to have a simple claim about what is API or not.

If you cherry pick this commit on Check key usage and adapt certificate validation commit, you should see something like this :

The following API problems caused the build to fail:
[ERROR] java.method.removed:
method boolean org.eclipse.californium.scandium.dtls.CertificateRequest::addCertificateAuthorities(java.security.cert.X509Certificate[]):
Method was removed. (breaks semantic versioning)

[ERROR] java.method.removed:
method java.util.List<java.security.cert.X509Certificate> org.eclipse.californium.scandium.dtls.CertificateRequest::removeTrustedCertificates(java.util.List<java.security.cert.X509Certificate>):
Method was removed. (breaks semantic versioning)

[ERROR] java.field.nowFinal:
field org.eclipse.californium.scandium.dtls.ClientHandshaker.maxFragmentLengthCode:
Field is now final. (breaks semantic versioning)
[ERROR] java.field.nowFinal:
field org.eclipse.californium.scandium.dtls.Handshaker.certificateChain:
Field is now final. (breaks semantic versioning)
[ERROR] java.field.nowFinal:
field org.eclipse.californium.scandium.dtls.Handshaker.privateKey: 
Field is now final. (breaks semantic versioning)
[ERROR] java.field.nowFinal:
field org.eclipse.californium.scandium.dtls.Handshaker.publicKey: 
Field is now final. (breaks semantic versioning)
[ERROR] java.field.nowFinal:
field org.eclipse.californium.scandium.dtls.Handshaker.sniEnabled: 
Field is now final. (breaks semantic versioning)
[ERROR] java.field.nowFinal:
field org.eclipse.californium.scandium.dtls.Handshaker.useStateValidation: 
Field is now final. (breaks semantic versioning)

(At first sight, all this breaks seems easy to avoid)

[sbernard31]

Once archive is built, you can change the configuration and only launch : mvn revapi:check.
(if you modify the code you need to build again)

[boaks]

[ERROR] java.field.nowFinal:
field org.eclipse.californium.scandium.dtls.Handshaker.certificateChain:
Field is now final. (breaks semantic versioning)

Maybe that is a general idea, but here, that doesn't make sense for me.
Who is going to change the chain during a ongoing handshake? Why?
That's exactly, what I call "academically".

[sbernard31]

That's exactly, what I call "academically".

Ok I get you point. I understand that you think that nobody will use this field and so you can change it freely. (I'm not sure I agree but anyway I get it)
That just means that you consider this is internal API and so this should be in an "internal" package.
(if that was the case error would have not be raised)
Or in other word, this means that something we don't want to be public must be internal. So you keep it like this as long as you don't increment major version because this is what you guarantee with semantic versioning. All of this is not "academical" this is just a good way to manage API.

That's generally why a project begin by putting a lot of thing in "internal" package and move it bit by bit in the public API if needed.

• internal => public means no API break
• public => internal mean API break

Without that convention, you can only bet on "good sense" which is very subjective and brings confusion. Without tooling I pretty sure we will also break "real API" accidentally.

In this particular case, if we want to support semantic versioning this would be easy :
If this is important to change it to final, go to 3.0. If we can live with non final, just keep it (add a TODO for when we will create a 3.0 branch) that's it. (no so huge, to be honest, I do not see the issue)

[boaks]

FMPOV that "programming language based check" is in the current state of californium source the wrong approach! I don't see the real benefit. It just nails down code without real reason.
Beside of "programming language basics", do you see any argument to be able to change the "chain"?

To use such a tool, I would prefer to use then a maintained list of exclusions (as above).
So, do you know, how to use Specifying The Analysis Configuration Using JSON?

[boaks]

Do you plan to invest your time in a API cleanup as base for 3.0?

[sbernard31]

Beside of "programming language basics", do you see any argument to be able to change the "chain"?

I don't get it. Could you reword ?

To use such a tool, I would prefer to use then a maintained list of exclusions (as above).

You mean :

1. by adding internal.* or impl.* package
2. or just adding exclusion one by one, class by class or package by package ?

I really don't like the 2., because as explained this means that users are not able to clearly now what is API or not.
E.g. : Handshaker and SessionCache are both in dtls package, 1 will be part of the API, the other not.

Do you plan to invest your time in a API cleanup as base for 3.0 ?

I can try to find time to work on this but I will probably need your help to know what is part of the API.

It just nails down code without real reason.

For me :

• either we really want to release a 2.1 and it just means that 2.1 will not modify the code so much.
  The changes for Verify certificates also without trusted certificates. #1146 (which is more no changes 😅) to keep the API is really minor even if you consider this useless. So sound not to be a big deal.
• or we don't care about release 2.1 and directly go to 3.0 without API clean and next time we will probably increment major version too.
• or we go to 3.0 and start cleaning API.
• or we don't do semantic versioning (but @sophokles73 sounds not really OK with that)

I'm open to all this ways, but saying that "we do semantic versioning without claiming clearly what is API" disturbs to me a lot.

[boaks]

Beside of "programming language basics", do you see any argument to be able to change the "chain"?

That means: according the programming language, changing something "protected" to "final" maybe considered to be a breaking API change. Considering the "domain", it maybe more a bugfix. Same would apply to change Asn1DerDecoder.OID to private. From the programming language it may be considered to be a breaking API change, from the "domain", it maybe a bugfix (though the OID is not that useful outside).

to know what is part of the API.

That's the hard part and it will be time consuming. And I have my strong doubts about the benefits.

I still think, use the tool, but with explicit list all breaks and document, why it's considered that these breaks are internal. If someone has a use-case to consider it as public, then we may even change such stuff again. That procedure will not "block the development ahead" for the API cleanup.
Therefore I asked, if you know how to use that json, because to plugin even reports the snippet to exclude the violation from the check. I would add them into such a file together with some comment.

[sbernard31]

I still think, use the tool, but with explicit list all breaks and document, why it's considered that these breaks are internal.

So we strongly disagree about how to handle versioning... 😿

Therefore I asked, if you know how to use that json, because to plugin even reports the snippet to exclude the violation from the check.

I don't test the json way, I just play with the xml way to exclude package or class.

Same would apply to change Asn1DerDecoder.OID to private. From the programming language it may be considered to be a breaking API change, from the "domain", it maybe a bugfix (though the OID is not that useful outside).

I'm not sure to understand but "the non breaking way to fix this API inconsistency" is to make the class OidEntityDefinition to public.
If you thing this attribute must not be used and should be private in the next major, you just deprecate it in the minor and go to private in the next major. (again I can see the issue)
But Non-Public Part of API is another problem this is more about API consistency than semantic versioning. We can decide to fix this consistency issue only for the 3.0.

That means: according the programming language, changing something "protected" to "final" maybe considered to be a breaking API change. Considering the "domain", it maybe more a bugfix.

I can not understand how this could be a bug fix ? I agree that this could be a kind of "API bug" and so I can not see the urgency to fix it in a minor version. If it exists case where there is a real conflict between fixing a real impacting bug and breaking the API of course we could discuss if this is worth to increment the major or not (and maybe do an exception) My guess is this is really rare.

[boaks]

I'm not sure to understand but "the non breaking way to fix this API inconsistency" is to make the class OidEntityDefinition to public.

FMPOV, that will result in arguments based on the programming language.
I strongly prefer arguments based on real use-cases.

[boaks]

I can not see the urgency to fix it in a minor version

Then any "cleanup" like this will be postponed.

[sbernard31]

Then any "cleanup" like this will be postponed.

Yes and why this is such a problem ? again if you don't want to postponed just increment the major.
(and if you cleanly separate API and internal, you just need to postponed for API)

[boaks]

Sorry, for me that is just useless boiler plate!
Changing Asn1DerDecoder.OID to private without deprecating or major version is for me the right approach. Beside of "strict programming language API rules" I see no reason to invest time into that.

[sbernard31]

FMPOV, that will result in arguments based on the programming language.
I strongly prefer arguments based on real use-cases.

Yes, an API is a programming language contract between user and developer.
Here I just say there is an easy way to respect this contract. So no need to create an additional clause.
As I said before, if there is no easy way, additional clause could be discussed but like with law this could take time.

(This very long discussion is a good example)

[sbernard31]

Changing Asn1DerDecoder.OID to private without deprecating or major version is for me the right approach.

This is not how a contract works ... Why this seems to you such a big issue to just keep this field public (and optionally deprecated) ? I don't get why this could be a such big issue to do that.

@sophokles73 please help us !!! 😫 we are going in circles ...

[sophokles73]

Here are my two cents

We are using semantic versioning. In particular this means that a developer building an application based on Cf 2.0.0's public API should be able to replace Cf 2.0.0 artifacts with 2.x artifacts without being required to change any of his application code.

To me, the only question remaining in this context is: which of Cf 2.0.0's packages and classes do we consider public API. I agree with @boaks in that we should make this assessment/decision based on practical requirements and use cases or usage scenarios instead of merely based on the name of the package. I do agree with @sbernard31 in that in the next major release we should start reflecting the fact whether a class is public API or not also in the name of the package that it belongs to, e.g. introducing package names that end in .impl do denote that this is an implementation specific package which is not part of the public API.

Using a maven plugin to detect breaking of public API makes a lot of sense to me as it is hard to keep track of transitive dependencies. FMPOV it will be a matter of configuring the plugin appropriately based on our assessment of which classes constitute the public API.

In any case, I guess we will not get around making that assessment first so that we can decide how to proceed with any of the warnings issued so far.

[sbernard31]

I think a good question is : Should we release a 2.1 ? what is the benefits of that ?
Because if we say next release will be 3.0. Almost all disagreement disappears.
We could even try to create 3.0 version with almost no break about what we currently consider as "our unclear 2.0 public API"

[sophokles73]

Because if we say next release will be 3.0. Almost all disagreement disappears.

It does for 3.0.0. But what about 3.1.0?

I think a good question is : Should we release a 2.1 ? what is the benefits of that ?

I guess that depends on the content of 2.1. For a service release, i.e. 2.0.1, the benefit is that users can take advantage of fixes without be required to change their code.

[sbernard31]

But what about 3.1.0?

Maybe, I'm wrong but I understand that every body(3 of us) pretty much agree that for the 3.0 we should separate clearly public and internal API using package naming + tooling ?
I understand that the main @boak's concern is about who will do that ? I can give a try. (at January beginning after my vacation)

I guess that depends on the content of 2.1. For a service release, i.e. 2.0.1, the benefit is that users can take advantage of fixes without be required to change their code.

If we decide that a 2.1 would be great, the idea would be to create a 3.0 with the objective in mind to limit/avoid the break with "our unclear 2.0 public API". So strictly a 3.0 but for most users a smooth upgrade. The 3.0 will not be a guarantee but a pragmatic best effort, then we will provide real guarantee with next release cycle thanks to revapi.

[sophokles73]

then we will provide real guarantee with next release cycle thanks to revapi.

Not because of revapi but because we then (hopefully) have defined which is public API and which is not ;-)

[sophokles73]

understand that the main @boak's concern is about who will do that ? I can give a try. (at January beginning after my vacation)

Sounds good. Let me know if I can help. However, I have to admit that my Cf knowledge and the purpose of all its classes has become a little rusty over the years ;-)

[boaks]

@sbernard31, about 3.0 instead of 2.1:

I simply don't consider you're findings so far (mainly by the strict technical analysis) as issues.
Anyway, if we found, that people are irritated by a 2.1 and claim, that it violates to much API, I also don't object to rename it 3.0 before releasing it. But not just for a tool or academical arguments.

[sbernard31]

if we found, that people are irritated by a 2.1

You find at least me. 😅 How many people do you need ? 😁

But not just for a tool or academical arguments.

I think this is a kind of a caricature 😞.
Once we will have defined what is internal and what is not, I bet revapi will mainly(only?) report real break.
Or maybe it will reveal that we did a mistake about what is part of the API and so it's too late for this major. We can to try imagine how user will use our library to define API but once we deliver it, we should consider that anything we put in the API could be used and should stay in the API until we increase the major version, that's the deal with users.

The supposedly pragmatic way about breaking because surely nobody use it, then hoping that people will open a ticket, then fix it and so maybe generating a new break does doesn't make sense to me.
I can't imagine a linux system managed like this 😱

@boaks, I believed that what is most important for you :

1. you want a 2.1 to provide a smooth upgrade to user
2. you want to be able to change what you consider as internal NOW
3. do no want to spend so much time on this.

I do my best to try to find a solution which would satisfy that unless the number of the version...and it sounds not enough... 😢

[sophokles73]

Once we will have defined what is internal and what is not, I bet revapi will mainly(only?) report real break.
Or maybe it will reveal that we did a mistake about what is part of the API and so it's too late for this major. We can to try imagine how user will use our library to define API but once we deliver it, we should consider that anything we put in the API could be used and should stay in the API until we increase the major version, that's the deal with users.

I agree with that.

The supposedly pragmatic way about breaking because surely nobody use it, then hoping that people will open a ticket, then fix it and so maybe generating a new break does make sense to me.
I can't imagine a linux system managed like this scream

I assume you meant

... a new break doesn't make sense to me.

And I also agree with that.

FMPOV there is no way around establishing a base line of what we (currently) consider public API as that will determine, if we need to create a 3.0.0 for a breaking change or if we can include it in a 2.1.0 ...

[sbernard31]

FMPOV there is no way around establishing a base line of what we (currently) consider public API as that will determine, if we need to create a 3.0.0 for a breaking change or if we can include it in a 2.1.0 ...

I get your point but my position is :
As currently, there is no "impl"/"internal" package nor any kind of description of what is part of the public API in the 2.0.0, I assume that users can expect that anything is part of the API. (I suppose this is the main disagreement ...)
Considering this assumption, when we will clarify this by moving some classes in impl package this will of course generate an API break (which for me should be handled with a major version increment)
If we didn't do a mistake about identifying public/internal API and we (almost?) only generate break in internal part, this would be easy to upgrade from the 2.0.0 to 3.0.0 version, and so we keep the initial objective of the 2.1 version which is to provide a smooth upgrade.

[boaks]

For me it's hard to foresee, what this would actually cause.
For "my way" I adapted the PR#s #1165 and #1157 to show, what my intention will be.

[boaks]

if we found, that people are irritated by a 2.1

You find at least me. 😅 How many people do you need ? 😁

FMPOV, it's more about the use-cases, then about the number of people.
Though the use-case are not bound to the "strict language" usage but rather to the domain usage, my forecast will be, that there are not too many "use-case breaks".

[boaks]

@sbernard31
@sophokles73

FYI:
revapi seems to require some setup for californium.actinium and californium.tools, at least the builds are failing on ci.eclipse.org.
I disabled the check therefore in these two sub-projects.

Don't hesitate to fix it and enable it again, if that pays off for you.