Required package exports of Checkstyle in OSGI bundle (eclipse-cs)

[Roman Ivanov]

Hi Roman, I started working on the adoption of Checkstyle 7 (7.1 more specifically) into eclipse-cs. Since it's a major version jump I am considering some backward breaking changes, among others I am planning on restricting the exported packages of the main checkstyle osgi bundle. In the past this has caused some problems for other plugin providers, which led to unexpeced class version conflicts. Because of these issues I want to try to limit the exported packages to what is absolutely necessary for eclipse-cs extensions (such as sevntu-checkstyle). The current minimal approach would be to only export the following packages:     com.puppycrawl.tools.checkstyle.api     antlr     antlr.collections This would limit the visible checkstyle classes to only the Checkstyle-API. The Antlr packages are required, because the Checkstyle API itself has a direct dependency to it. However, when trying sevntu-checkstyle with this setup it breaks, because additional implementation packages are required and expected to be on the extension classpath:     com.puppycrawl.tools.checkstyle.utils     org.apache.commons.beanutils     com.google.common.collect What would be the preferred approach from the Checkstyle teams point of view? Should Checkstyle extensions only rely on the official API? Or are all runtime dependencies of Checkstyle declared safe for use? I know in most runtime environments (Maven/Ant...) cannot enforce this, but Eclipse/OSGi can. Or should the exported packages in eclipse-cs stay the same as before, with all runtime dependency packages being exported? Kind regards, Lars Hi Lars, sorry for long delay to answer you. > because additional implementation packages are required and expected to be on the extension classpath It is possible and it is not on purpose. Most likely su​ch classes are used only because they are available. But if that possible to to update sevntu plugin to keep its own version of apache commons and guava - it will be good. >  com.puppycrawl.tools.checkstyle.utils Lets add it to visible package packages, I removed all Util classes from API a while ago, but it is what extensions should use. There too much useful methods. > Because of these issues I want to try to limit the exported packages to what is absolutely necessary for eclipse-cs extensions (such as sevntu-checkstyle). yes, lets keep sevntu as just another extension and test on it all braking chnages.  > Should Checkstyle extensions only rely on the official API? dependencies should be minimal -  api (contract) + utils (not a contract, just convenience) > Or are all runtime dependencies of Checkstyle declared safe for use?  I did not give anybody such promise. All of them might be used only because there no way to restrict that. We are ok to make some restrictions. There are might be cases when smb extends certain Check , but I always discourage people to extend Checks. Usage of AbstractCheck is what all Checks should do. hmmm ... there is https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheck.java class that is not an API yet, only because it is in transition period . This is already a base class for all javadoc related Checks. Restriction mean that Eclipse extensions will not be able to create javadoc Checks at all till I make that class an API. So eclipsecs's package restrictions force me to put classes to API without testing/probation period -  not ideal but ..... .  I know that checkstyle internals are used a lot by other tools, all/most of them are not eclipsecs extensions. ------------------------------------ So lets start from scratch ..... What do we benefit from packages restriction ? Looks like with need weigh pros and cons. thanks, Roman Ivanov Hello Roman, > So lets start from scratch ..... What do we benefit from packages restriction ? Looks like with need weigh pros and cons. Checkstyle itself gains little to nothing from restricting packages. However in OSGI context a bundle (e.g. checkstyle-core) needs declare which of it's packages are visible to extensions. Today this includes all of Checkstyle's packages and Checkstyle's own dependenceis (antlr, guava, commons-*). The downside of this in OSGI realm is that those packages are now visible not just to Checkstyle extensions but also do unrelated plugins, which potentially import one of the 3rd-party packages. This can lead to nasty class version conflicts in unexpected places, e.g. the Checkstyle plugin can potentially break a different plugin. The main problem is in fact that 3rd-party dependencies today need to be exported in order for (some) checkstyle extensions to work. 2 things could be done to help with this main problem: a) if possible change Checkstyle API to not directly depend on Antlr b) publish an informal rule for extension providers that they should not depend on Checkstyle's internal 3rd-party dependencies In eclipse-cs rule B can then be enforced by not exporting those packages, extension relying on them would not work in this context unless they provide their own dependencies. Checkstyle's own packages can be continued to be exported and thus fully visible, there is little potential for class version conflicts there. Kind regards, Lars HI Lars, > a) if possible change Checkstyle API to not directly depend on Antlr yes, I though about this too, now it has an issue to not forget https://github.com/checkstyle/checkstyle/issues/3417 > b) publish an informal rule for extension providers that they should not depend on Checkstyle's internal 3rd-party dependencies That is reasonable , but looks like this warning should be hosted on your website , as it is relevant only for OSGI , OSGI is only at eclipse-cs for now. If you know good place to put in on checkstyle web site - please let me know , or do PR. > In eclipse-cs rule B can then be enforced by not exporting those packages, extension relying on them would not work in this context unless they provide their own dependencies. Lets do this, plugins need to be updated, sevntu is not an execption, if you provide patch for sevntu it will be even good as we can show example of how that should be done. > Checkstyle's own packages can be continued to be exported and thus fully visible, there is little potential for class version conflicts there. you mean com.puppycrawl.tools.checkstyle . if yes - that is fine. --------------------------- Looks like we got very valuable conversation there , can I copy paste it to mail list ? to make it public and let other see reasons of our decision. thanks, Roman Ivanov