Menu
▾
▴
#164 Update plug-in manifests for Checkstyle 7 / Java 8 transition
It has been a little while since I have looked at the project for eclipse-cs, so please forgive me for some inaccuracies. And thank you for all your great work to date. My request concerns Checkstyle 7 and the new requirement of Java 8 as the runtime. (Sidebar: I'm a little perplexed that the main jar has gone Java 8+; we have many Java 7 projects that will stay that way for some time and now we won't get any bug fixes or enhancements to static analysis without some wierd dev setup.)
With the requirement of a new min runtime, I am thinking we have a perfect opportunity to update the plug-in projects to a more modern standard with respect to Eclipse.
1) Please publish source plug-ins for the main plug-ins; being able to jump into the source when developing extensions or tracking down bugs (but not requiring cloning and importing the git repo) is key. I have been using a class decompiler to limp along.
2) All plugins would be set to JavaSE-1.8 minimum runtime in the manifest
3) Any Require-Bundle properties would have versions upped to minimum Eclipse for Java 8 development (Kepler I think) -- I took a quick look at the manifests and this may not apply; I did not see an import for org.eclipse.ui or anything similar
4) That said, maybe some of the Import-Packages should be switched to Require-Bundle directives
5) Is "Eclipse-BundleShape: dir" still required for the checkstlye plug-in?
6) In my earlier experience doing plug-in development, I found that I was trying to use guava or commons-lang in my plug-in and this mysteriously created a dependency with the checkstyle plug-in. I found this was because checkstyle ot core or some combination of all your plug-in export a bunch of packages like org.apache.commons. I think there is a way to say that a base plug-in exports certain things but only to plug-ins that explicity depend on the plug-in. That is to say some kind of "private" or "friend" exports.
7) Bonus: Somewhere, the update site must be dropping down to http (not https) or redirecting out of domain, because I have not been able to install from the update site at work for over a year now. They must have some sort of network policy, but so many other open source plug-ins work that I think the oddity must line in the update site config for checketyle.
Thanks for your time and consideration. I'd be happy to answer any questions related to these items and pitch in with coding/patches. We use Checkstlye at work quite extensively and I have been very pleased with this plug-in keeping pace with the main libraries revisions.
Hi Eric,
thanks for the detailed request, I'll respond in multiple posts.
Overall, Checkstyle 7 and Java 8 requirement is a pure runtime requirement as far as I understand, you can still check Java 1.2 + source code projects with Checkstyle 7.
So no worries, as long as you're Eclipse/Build process is running on Java 8 you should be fine with Checkstyle 7, even for Java 7 projects.
1) + 2) will do
3) the only eclipse version reference was in feature.xml ("<import plugin="org.eclipse.core.runtime" version="3.2.0" match="greaterOrEqual"/>"), which was setup up to require Eclipse 3.2 or greater. In combination with 5) I think this can be safely set to 3.7.0
There is no specific Eclipse version that cannot be run with Java 8 per se, since Java is supposed to be backwards compatible. I think only Mars or Neon requires a Java 8 VM.
4) As far as I've followed discussion around this the OSGI people recommend use of Import-Packages above all. Can you please elaborate at which place Require-Bundle would solve an existing problem?
5) I suppose not, originally it was a workaround for an Eclipse PDE bug (https://bugs.eclipse.org/bugs/show_bug.cgi?id=289248) which had supposedly been fixed in Eclipse 3.7 (Indigo). I will revert back to the jar'ed form for eclipse-cs 7 and see what happens
6) I'll try to cut down on exported packages for eclipse-cs 7. Problematic are packages which are "leaked" by the Checkstyle API itself (e.g. antlr), as established a few months back:
https://sourceforge.net/p/eclipse-cs/bugs/407/
This means that some packages will still need to be exported after all, in order to not break extension plugins.
7) This might be a problem how I am using a composite p2 repository to redirect to the actual update site version on Sourceforges download infrastructure, see http://eclipse-cs.sourceforge.net/update/compositeContent.xml
As you see today it uses HTTP URLs which might be causing your problem. I'll change this to HTTPS for the next version, and again see what happens ;-)
Current state of affairs is available here: https://sourceforge.net/p/eclipse-cs/git/ci/adopt_checkstyle_7.1/tree/
Regarding 6) I temporarily went for a very strict approach, exporting only the following packages:
com.puppycrawl.tools.checkstyle.api
antlr
antlr.collections
All other Checkstyle packages and third party packaged (guava, commons.collections) are exported only with visibility to net.sf.eclipsecs.core and net.sf.eclipsecs.ui plugins via the x-friends directive.
This is probably the minimum exposure possible right now. However, if extension plugin providers today depend on other Checkstyle or 3rd-party packages being visbible they would break with this current approach.
If you want you can give it a spin by checking out the above branch and building using Maven from the eclipsecs-parent project: mvn install
This will also create a source feature now, however provided sources currently only work for eclipse-cs classes and not Checkstyle classes, even though I jumped through several hoops to include Checkstyle sources. I guess this is caused by Checkstyle being a jar inside the plugin. I'll have to investigate some more on this.
Re: #4, I must admint I never developed a good sense for Import-Package vs. Require-Bundle. There are 2 things I can add that might help:
a) Since Eclipse goes above and beyond OSGi, if a plug-in uses an extension point from another, I choose to use Require-Bundle in that situation since you can't really call out the source of the extension point to OSGi any other way.
b) Ideally, if I were to create a plug-in that adds Checkstyle checks, I would hope that using Require-Bundle: net.sf.eclipsecs.core would give me all the necessary imports to extend the required interfaces. Similarly for adding quick-fixes; Require-Bundle: net.sf.eclipsecs.ui IMO should give me all the imports. However, under the current manifest for the ui plug-in, I needed to also import these packages:org.eclipse.core.resources, org.eclipse.jdt.core.dom, org.eclipse.swt.graphics, org.eclipse.jface.text. And I added a Require-Bundle: org.eclipse.ui.ide for the marker requirement.
I'm not sure if this sways you at all. But I ran into 2 different situations with the current manifests: 1) implementing a Checkstyle extension plug-in required me to fiddle with a lot of imports (mentioned in (b) above). 2) Implementing some other plug-ins where I was trying to use commons-lang and commons-io and I picked up a dependency on Checkstyle that I was not expecting.
Also, if using Import-package for things like commons-lang, could you use versions in your exports and imports? This would at least ensure you are linking to the correct level of the libs and no other plug-in that exports the same is interfering.
Last edit: Eric Milles 2016-08-17
Thanks again for taking the time to read and attempt to understand my ramblings. I appreciate all the good work on this project to date. We use it every day in our shop and I find it a valuable tool. I just wish it supported Groovy as well, but that's a totally other ball of wax.
Two more items that came to mind:
1) Could you update the plug-in xmls to use <?eclipse version="3.4"?> I'm not sure if this means updating any other parts.
2) Would it be possible to set the Eclipse-SourceReferences property in each plug-ins manifest? Old directions show how to make this work for a CVS repo, not sure if the Git or SVN repo that eclipse-cs is in works. But if this were set, I could choose Import As > Project from Repository... in the Plug-Ins view for a Target Platform that just links to the newest Eclipse CS update site.
For the additional items
1) Done, for the 7.1 version
2) Done, generated via Maven Tycho, unfortunately it doesn't quite work as expected. You might give it a try and fix it properly.
Regarding the Require-Bundle/Import-Packages issue. For the eclipse-plugin dependencies (internal references) Require-Bundle was already used, so no Import-Package for commons-lang etc.
Still on TODO is setting package versions for the exported packages.