Closed cdealti closed 7 years ago
Hello Cristiano,
The version change required by Semantic versioning is different depending on the use of the API.
For example, adding a method to javax.servlet.ServletContext is a minor change, it only breaks the people who implement a Servlet Container. Adding a method to javax.servlet.Servlet is a major change, as it breaks all people who write servlets.
In this case the baselining plugin has determined that the org.eclipse.kura.configuration.CloudService interface is a “consumer type” i.e. it is implemented by other bundles that would be considered “clients” of the API. If this determination is correct then the change genuinely is a breaking API change, and needs to be a 2.0.0. If this is not the case then you should mark the interface as @ProviderType using the OSGi annotations. That way bnd will know which versioning policy to use, and 1.1.0 will be acceptable.
I’m afraid that I can’t answer why it’s suggesting 1.0.0 as an alternative, except to say that that would be the correct version if you reverted the API changes.
Tim Ward
Apache Aries PMC Member, OSGi IoT Expert Group Chair, Author Enterprise OSGi in Action timothyjward@apache.orgmailto:timothyjward@apache.org
On 15 Jul 2016, at 11:45, Cristiano De Alti notifications@github.com<mailto:notifications@github.com> wrote:
Hello, I'm trying to use the bnd-baseline-maven-plugin to verify the Kura API baseline. I've run the analysis on this branchhttps://github.com/eclipse/kura/tree/enable-bnd-baseline-maven-plugin
The verification fails for some API packages (see below). Full maven log herehttps://s3.amazonaws.com/kura-issues-logs/bnd-baseline-maven-plugin.log.zip.
For example, if we consider the org.eclipse.kura.configuration package exported by the Kura org.eclipse.kura.api bundle, I believe we properly changed the MINOR version from 1.0.0https://github.com/eclipse/kura/blob/KURA_1.4.0_RELEASE/kura/org.eclipse.kura.api/src/main/java/org/eclipse/kura/configuration/ConfigurationService.java to 1.1.0https://github.com/eclipse/kura/blob/enable-bnd-baseline-maven-plugin/kura/org.eclipse.kura.api/src/main/java/org/eclipse/kura/configuration/ConfigurationService.java, having just added some methods and a constant to the org.eclipse.kura.configuration.CloudService interface. Changes to other classes in the same package do not touch the API.
If I'm not wrong, Semantic Versioning only requires a change to the MINOR version. However the plugin complains that:
[ERROR] Baseline mismatch for package org.eclipse.kura.configuration, MAJOR change. Current is 1.1.0, repo is 1.0.0, suggest 2.0.0 or 1.0.0
I don't understand why we should change the MAJOR number (2.0.0) and why the plugin suggest (1.0.0) as an alternative.
[ERROR] Baseline mismatch for package org.eclipse.kura.bluetooth, MAJOR change. Current is 1.2.0, repo is 1.1.0, suggest 2.0.0 or 1.2.0
[ERROR] Baseline mismatch for package org.eclipse.kura.configuration, MAJOR change. Current is 1.1.0, repo is 1.0.0, suggest 2.0.0 or 1.0.0
[ERROR] Baseline mismatch for package org.eclipse.kura.data, MAJOR change. Current is 1.1.0, repo is 1.0.0, suggest 2.0.0 or 1.0.0
[ERROR] Baseline mismatch for package org.eclipse.kura.net.wifi, MINOR change. Current is 1.2.0, repo is 1.2.0, suggest 1.3.0 or -
[ERROR] Baseline mismatch for package org.eclipse.kura.position, MAJOR change. Current is 1.1.0, repo is 1.0.0, suggest 2.0.0 or 1.1.0
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/bndtools/bnd/issues/1553, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ADs8eE1VDbntTr6zO9A0hgqenWrKEP6Hks5qV2THgaJpZM4JNR-w.
@timothyjward thanks! I haven't realized this. Then the API types must be annotated the same as their providers which makes sense since they follow the same versioning rules.
The problem is that we are still using OSGi R5 so no annotations. I assume for the time being we can define these two annotations in a separate bundle.
Cheers, Cristiano
The OSGi annotations are build-time only, and packaged separately, so there should be no problem using osgi.annotation 6.0.1 alongside osgi.core 5.0.0
Sent from my iPhone
On 19 Jul 2016, at 21:00, Cristiano De Alti notifications@github.com<mailto:notifications@github.com> wrote:
@timothyjwardhttps://github.com/timothyjward thanks! I haven't realized this. Then the API types must be annotated the same as their providers which makes sense since they follow the same versioning rules.
The problem is that we are still using OSGi R5 so no annotations. I assume for the time being we can define these two annotations in a separate bundle.
Cheers, Cristiano
You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/bndtools/bnd/issues/1553#issuecomment-233748145, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ADs8eHfMGlHQtI2BKTQJ3BjsS1aXWHq5ks5qXSzmgaJpZM4JNR-w.
Cool! thanks
Adding the @ProviderType
annotation to every type of the org.eclipse.kura.api
seems to work. It might be worth to explicitly state that it must be added to the API. The Javadoc only mentions API consumers and producers but says nothing about the API itself.
You seem to have solved your issue.
Adding the
@ProviderType
annotation to every type of theorg.eclipse.kura.api
seems to work. It might be worth to explicitly state that it must be added to the API. The Javadoc only mentions API consumers and producers but says nothing about the API itself.
I would like to reiterate that _this approach is only valid if all your API is provider type_. If any of the types are supposed to be implemented by the user of the API then this approach will give the wrong answer.
Having looked at org.eclipse.kura.api
it does seem to be the case that all of the types are ProviderTyoe, however there are obvious examples of ConsumerType interfaces in other Kura API packages, for example org.eclipse.kura.api.gpio.PinStatusListener
is almost certainly a ConsumerType interface. See (https://github.com/eclipse/kura/blob/develop/kura/org.eclipse.kura.api/src/main/java/org/eclipse/kura/gpio/PinStatusListener.java):
It really is important to mark these API types appropriately.
thx
Can we reopen this?
I'm still struggling to have this working and merged in our Eclipse Kura build.
I've properly annotated some of the interfaces of the org.eclipse.kura.api bundle
in my Kura fork with @ProviderType
.
I've set the baseline to the released org.eclipse.kura.api bundle
v.1.1.0. Please note that the baseline is not yet annotated with @ProviderType
so the first question is if the analysis is likely to fail in this case. I had the API analysis working in the past, on a previous baseline, so my impression was that the plugin did not really care if the baseline is annotated or not.
Downloading: https://repo.eclipse.org/content/repositories/kura-releases/org/eclipse/kura/org.eclipse.kura.api/1.1.0/org.eclipse.kura.api-1.1.0.jar
Downloaded: https://repo.eclipse.org/content/repositories/kura-releases/org/eclipse/kura/org.eclipse.kura.api/1.1.0/org.eclipse.kura.api-1.1.0.jar (198 KB at 98.1 KB/sec)
[ERROR] Baseline mismatch for package org.eclipse.kura, MAJOR change. Current is 1.2.1, repo is 1.2.1, suggest 2.0.0 or -
...
[ERROR] Baseline mismatch for package org.eclipse.kura.gpio, MAJOR change. Current is 1.0.1, repo is 1.0.1, suggest 2.0.0 or -
...
[ERROR] The bundle version change (1.1.0 to 1.2.0) is too low, the new version must be at least 2.0.0
As we can see the plugin thinks that I have two major changes.
The first one is for the package org.eclipse.kura
. This package only contains various Kura Exception classes, which has changed since the baseline - not sure if an incompatible way, and no interfaces. Hence I haven't annotated them. The first question is whether the plugin analyses these classes and what type of binary compatibility analysis it does. Semantically the exception classes in this package should be @noextend
.
The second package org.eclipse.kura.gpio
hasn't changed since the baseline. I've only added @ProviderType on two interfaces but this should result in a CHANGED (micro version) change.
Any idea of what's going on?
Right, I have prototyped a "more detailed" report that now comes out when you enable reporting. When enabled this shows the detail of a number of breaking changes in your build.
Specifically, all the exception types have issues due to removed fields!
Baseline mismatch for package org.eclipse.kura.gpio, MAJOR change. Current is 1.0.1, repo is 1.0.1, suggest 2.0.0 or -
[INFO] MAJOR PACKAGE org.eclipse.kura.gpio
[INFO] MAJOR CLASS org.eclipse.kura.gpio.KuraClosedDeviceException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] MAJOR CLASS org.eclipse.kura.gpio.KuraGPIODeviceException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] MAJOR CLASS org.eclipse.kura.gpio.KuraUnavailableDeviceException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] CHANGED INTERFACE org.eclipse.kura.gpio.GPIOService
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] CHANGED INTERFACE org.eclipse.kura.gpio.KuraGPIOPin
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] UNCHANGED INTERFACE org.eclipse.kura.gpio.PinStatusListener
[INFO] UNCHANGED ENUM org.eclipse.kura.gpio.KuraGPIODirection
[INFO] UNCHANGED ENUM org.eclipse.kura.gpio.KuraGPIOMode
[INFO] UNCHANGED ENUM org.eclipse.kura.gpio.KuraGPIOTrigger
[INFO] UNCHANGED VERSION 1.0.1
@timothyjward Tim, thanks! This helps tremendously!
So the MAJOR changes are related to the specific KuraExceptionS of the package org.eclipse.kura.gpio
and this is due to a refactoring of the base KuraException class of
the package org.eclipse.kura
(not shown in your report above).
You know, people like refactoring code and the implications are not always obvious, like in this case.
This leads me to some observations and questions I've already asked above. Can you please comment the following items, one by one?
The root cause is a breaking change to org.eclipse.kura.KuraException
but this is a class so it cannot be annotated.
The plugin obviously analyzes also classes, not only interfaces.
The plugin assumes the worst case, i.e. the class is meant to be extended by the API consumers so removing a protected field represents a binary incompatibility for consumers. Hence the MAJOR change requested.
In this particular case KuraException is not meant to be extended by API consumers but I don't have any other mean of indicating this intention. I guess the @noextend
Javadoc tag would be ignored by the plugin. And I cannot make the class final
.
The obvious question depending on the answer to 1 above. Should be @ConsumerType
and @ProviderType
be allowed on classes too?
Thanks for the patience.
On Tue, Jan 24, 2017 at 1:33 PM, Cristiano De Alti <notifications@github.com
wrote:
@timothyjward https://github.com/timothyjward Tim, thanks! This helps tremendously!
So the MAJOR changes are related to the specific KuraExceptionS of the package org.eclipse.kura.gpio and this is due to a refactoring of the base KuraException class of the package org.eclipse.kura (not shown in your report above). You know, people like refactoring code and the implications are not always obvious, like in this case.
This leads me to some observations and questions I've already asked above. Can you please comment the following items, one by one?
1.
The root cause is a breaking change to org.eclipse.kura.KuraException but this is a class so it cannot be annotated.
Why not?
1. 2.
The plugin obviously analyzes also classes, not only interfaces.
Yes of course! Any class in any exported package is API.
1. 2.
The plugin assumes the worst case, i.e. the class is meant to be extended by the API consumers so removing a protected field represents a binary incompatibility for consumers. Hence the MAJOR change requested.
Correct!
1. 2.
In this particular case KuraException is not meant to be extended by API consumers but I don't have any other mean of indicating this intention. I guess the @noextend Javadoc tag would be ignored by the plugin. And I cannot make the class final. 3.
The obvious question depending on the answer to 1 above. Should be @ConsumerType and @ProviderType be allowed on classes too?
They are allowed on classes.
However, one point of contention here is that even adding the @ProviderType to the Exception won't help because IIRC deletions are always considered a breaking change.
Sincerely,
1.
Thanks for the patience.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bndtools/bnd/issues/1553#issuecomment-274945642, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI9TH2AILqAAdB3V4aX7sbIMS3NUSXqks5rVm4WgaJpZM4JNR-w .
-- Raymond Augé http://www.liferay.com/web/raymond.auge/profile (@rotty3000) Senior Software Architect Liferay, Inc. http://www.liferay.com (@Liferay) Board Member & EEG Co-Chair, OSGi Alliance http://osgi.org (@OSGiAlliance)
@rotty3000 finally!!! Thanks!!! Can you point me to the Javadoc or an article or something which states that the OSGi standard annotations @ProviderType or @ConsumerType are allowed for classes. Every reference I've found only talk about Java Interfaces.
However, one point of contention here is that even adding the @ProviderType to the Exception won't help because IIRC deletions are always considered a breaking change.
I see. However it would be binary compatible in this specific case. Do you agree?
I'm starting to see the light at the end of the tunnel
@timothyjward Tim, thanks! This helps tremendously!
Assuming the PR gets approved then the extra info will be available to you by specifying <fullReport>true</fullReport>
in your bnd-baseline-maven-plugin
configuration. For now you can build the PR locally and use it if you want.
On Tue, Jan 24, 2017 at 2:33 PM, Cristiano De Alti <notifications@github.com
wrote:
@rotty3000 https://github.com/rotty3000 finally!!! Thanks!!! Can you point me to the Javadoc or an article or something which states that the OSGi standard annotations @ProviderType or @ConsumerType are allowed for classes. Every reference I've found only talk about Java Interfaces.
Section 3.7.4 of OSGi Core R6 specification mentions both Interfaces and abstract classes, but honestly I think it would have been more wise to express it as any non-final types.
However Section 10.7.3 of same talks about it being used "generally" on "types". Perhaps this warrants a clarification in the spec.
Finally, BND will honour this annotation on any class as far as I've ever experienced.
Sincerely,
However, one point of contention here is that even adding the @ProviderType to the Exception won't help because IIRC deletions are always considered a breaking change.
I see. However it would be binary compatible in this specific case. Do you agree?
I'm starting to see the light at the end of the tunnel
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bndtools/bnd/issues/1553#issuecomment-274961060, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI9TKTHiQHe2TmrFnkUK9dL0_z-IBt9ks5rVnwqgaJpZM4JNR-w .
-- Raymond Augé http://www.liferay.com/web/raymond.auge/profile (@rotty3000) Senior Software Architect Liferay, Inc. http://www.liferay.com (@Liferay) Board Member & EEG Co-Chair, OSGi Alliance http://osgi.org (@OSGiAlliance)
@rotty3000 OK, the spec and the Javadoc talk about "type" while the pluging README.MD only talks about implementing interfaces. Maybe it should be updated to explicitly clarify that classes are allowed too.
In my case the problem is that I want the build to fail if semantic versioning is not followed but here we don't have a breaking change. If I followed the plugin analysis I should change the MAJOR number which I don't want.
The only option is to revert the breaking change or modify the plugin to have a finer grain analysis. The former option is more viable. The latter seems complex ;)
https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html
@timothyjward I'll try it out as soon as possible
I'm on board with @timothyjward's proposed change now. We're just waiting for @pkriens to give his blessing. :+1:
I'm on board with @timothyjward's proposed change now. We're just waiting for @pkriens to give his blessing. 👍
This specifically refers to PR #1858, which is separate from the other PR related to the maven plugin's reporting output. The two fixes are independent of one another and will be reviewed separately.
Yes sorry for not being clear on that.
On Jan 24, 2017 4:20 PM, "Tim Ward" notifications@github.com wrote:
I'm on board with @timothyjward https://github.com/timothyjward's proposed change now. We're just waiting for @pkriens https://github.com/pkriens to give his blessing. 👍
This specifically refers to PR #1858 https://github.com/bndtools/bnd/pull/1858, which is separate from the other PR related to the maven plugin's reporting output. The two fixes are independent of one another and will be reviewed separately.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bndtools/bnd/issues/1553#issuecomment-274982277, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI9TK37LwDRQDrCZ1YT9jMCnuDeT67rks5rVpVcgaJpZM4JNR-w .
Section 3.7.4 of OSGi Core R6 specification mentions both Interfaces and abstract classes, but honestly I think it would have been more wise to express it as any non-final types.
This language, of course, is intentional. I am not convinced that a non-abstract type can be marked either provider or consumer. It is concrete and thus self-implemented. An interface or abstract class must be implemented by an API provider or consumer depending upon the purpose of the type. ResolveContext is an abstract class meant to be implemented by an API consumer. Resolver is an interface meant to be implemented by the API provider. But an exception class is a concrete class that is already self-implemented. Neither API provider not API consumer needs to implement it. So marking it provider or consumer is of little meaning. So I don't see that any spec change should be made.
@timothyjward thanks for all your work.
Unfortunately in my case the removal of the protected field from KuraException still requires a MAJOR change.
I've annotated my KuraException with @ProviderType here so the removal of the protected field should result in a MINOR change. Instead I'm still getting:
Baseline mismatch for package org.eclipse.kura, MAJOR change. Current is 1.2.1, repo is 1.2.1, suggest 2.0.0 or -
[INFO] MAJOR PACKAGE org.eclipse.kura
[INFO] MAJOR CLASS org.eclipse.kura.KuraConnectException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] CHANGED CLASS org.eclipse.kura.KuraConnectionStatus
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MAJOR CLASS org.eclipse.kura.KuraException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MAJOR CLASS org.eclipse.kura.KuraInvalidMessageException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MAJOR CLASS org.eclipse.kura.KuraInvalidMetricTypeException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MAJOR CLASS org.eclipse.kura.KuraNotConnectedException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MAJOR CLASS org.eclipse.kura.KuraPartialSuccessException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MAJOR CLASS org.eclipse.kura.KuraRuntimeException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MAJOR CLASS org.eclipse.kura.KuraStoreCapacityReachedException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MAJOR CLASS org.eclipse.kura.KuraStoreException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MAJOR CLASS org.eclipse.kura.KuraTimeoutException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MAJOR CLASS org.eclipse.kura.KuraTooManyInflightMessagesException
[INFO] REMOVED FIELD org.eclipse.kura.KuraErrorCode m_code
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] UNCHANGED ENUM org.eclipse.kura.KuraErrorCode
[INFO] UNCHANGED VERSION 1.2.1
The removal of the protected field is in this commit
Please note that the baseline bundle was not annotated at all
Please note that the baseline bundle was not annotated at all
This is why you will still see the baseline issue. The default for non annotated types is to assume @ConsumerType
(which is definitely the right default).
If you can make a fix to the 1.1.x steam to add the annotation, and make that new version your baseline, then you should be done.
This is why you will still see the baseline issue. The default for non annotated types is to assume @ConsumerType (which is definitely the right default).
Then I do not understand why, if I add a new foo() method to the DataService, the tool only requires a MINOR change:
Baseline mismatch for package org.eclipse.kura.data, MINOR change. Current is 1.1.1, repo is 1.1.1, suggest 1.2.0 or -
[INFO] MINOR PACKAGE org.eclipse.kura.data
[INFO] CHANGED CLASS org.eclipse.kura.data.DataTransportToken
[INFO] ADDED ANNOTATED org.osgi.annotation.versioning.ProviderType
[INFO] MINOR INTERFACE org.eclipse.kura.data.DataService
[INFO] ADDED METHOD foo()
The DataService was not annotated in the baseline (hence assumed @ConsumerType) but the tools seems to consider only the @ProviderType I've added in this version, otherwise it would have requested a MAJOR change.
Another problem is that the the package org.eclipse.kura.cloud is totally skipped by the baseline verification. I cannot see anything in the detailed report and if I add a new method to one of its interfaces the plugin does not detect the change. There can be other packages whose baseline verification is skipped but I still need to check this.
This issue is closed and wont be noticed during issue review. If you have an additional problem, I suggest opening a new issue (and you can refer back to this issue for historical context.)
The first part was a follow-up to the last comment of @timothyjward.
The second part could be addressed in new issue. Edit: please disregard the above comment about the org.eclipse.kura.cloud package verification being skipped. My mistake
Hello, I'm trying to use the bnd-baseline-maven-plugin to verify the Kura API baseline. I've run the analysis on this branch
The verification fails for some API packages (see below). Full maven log here.
For example, if we consider the
org.eclipse.kura.configuration
package exported by the Kuraorg.eclipse.kura.api
bundle, I believe we properly changed the MINOR version from 1.0.0 to 1.1.0, having just added some methods and a constant to theorg.eclipse.kura.configuration.CloudService
interface. Changes to other classes in the same package do not touch the API.If I'm not wrong, Semantic Versioning only requires a change to the MINOR version. However the plugin complains that:
I don't understand why we should change the MAJOR number (2.0.0) and why the plugin suggest (1.0.0) as an alternative.