Request to deploy to the Marketplace: Guava Helper for Java 8


(Kengo TODA) #1

Hello SonarSource folks,

I want to deploy a SonarQube plugin that depends on SonarQube 5.6.6LTS, SonarJava 4.0 and FindBugs 3.5. This plugin helps developer to migrate from legacy Guava classes (such as Optional and SettableFuture) to standard Java 8 classes (such as Optional and CompletableFuture). Here is necessary information:

homepage: https://github.com/KengoTODA/guava-helper-for-java-8
download: https://repo1.maven.org/maven2/jp/skypencil/guava/guava-helper-sonarqube-plugin/1.0.3/
pluginKey: guava
license: Apache License v2

Could you have a review? Thanks a lot!


(Julien Henry) #2

Hi Kengo,

I find this kind of rules very useful, but I think they should ultimately be added to SonarJava, so that they can appear in SonarLint.

I can’t really give you feedback on the rule implementation, since I don’t know the findbugs API, but I find the plugin key a bit too generic. Maybe something like “guavamigration” would be more specific.


(Kengo TODA) #3

Sorry I cannot get your point. You mentioned two things, then what is TODO in my side?
If it’s just replacing key with “guavamigration”, it’s OK, I will do in next release 1.0.4.

If you suggest me to add to SonarJava, I need to say that I have no motivation to do so.


(Julien Henry) #4

If you suggest me to add to SonarJava, I need to say that I have no motivation to do so.

I was just sharing my personal opinion, no worries :slight_smile:


(Kengo TODA) #5

Thanks for your quick response :heart:
I’ll release 1.0.4 tomorrow, with “guavamigration” key.


(G Ann Campbell) #6

Hi,

The requirements to be added to the Marketplace are here: https://docs.sonarqube.org/display/DEV/Deploying+to+the+Marketplace

I haven’t tried to test it yet, but off-hand it looks like there are issues with requirements 3 and 7.

Specifically, since you don’t declare a sonar.pluginKey property (at least, I didn’t see one) then your entire artifact id becomes the plugin key: guava-helper-for-java-8, and that breaks the rule to have only alpha-numeric (lower case) keys. I know you’re planning to re-key the plugin, so please just keep this point in mind.

On #7, you don’t provide a link to a SonarCloud analysis. Maybe it’s there, but I’m too busy (lazy) to go looking right now.

Beyond those requirements, I’m a bit confused. You said initially your plugin depends on the SpotBugs plugin, but your README says it provides the plugin…? And if the latter, what happens if I install your plugin on an instance where SpotBugs is already installed?

And in either case, I have to wonder if the rules wouldn’t be better in the SpotBugs plugin. But like Julien, I’m just expressing a personal opinion.

So… once the things listed above are sorted out, I’ll do the testing required before I can add you to the Marketplace, and we’ll go from there.

Ann


(Kengo TODA) #7

Hi, Thanks for your reply!

Just now I released v1.0.4 that contains fix for plugin key, please have a try. :pray:

requirement 3

You can find plugin.key in this pom, it’s now “guavamigration”.
I’ve also confirmed that MANIFEST.MF contains expected plugin key.

requirement 7

You can find SonarCloud analysis in this page. Sorry for my laziness.
Quality gate has no problem.

Beyond those requirements, I’m a bit confused. You said initially your plugin depends on the SpotBugs plugin, but your README says it provides the plugin…?

My tool provides both SpotBugs plugin (not the ‘SpotBugs plugin’ for SonarQube, it’s plugin for SpotBugs) and SonarQube plugin, this is the point making you confused. Please kindly ignore ‘providing SpotBugs plugin’ part.


(G Ann Campbell) #8

Hi,

I’ve not (exactly) forgotten this. I had a busy week last week and hope to get to this in the next couple of days.

Regarding this:

Just to make sure, your project artifact will work as either a plugin to SonarQube or a plugin to SpotBugs? (Pretty cool!)

Ann


(Kengo TODA) #9

Yes my product provides both of them, you can find SpotBugs plug-in in the same groupId. Thanks for your notice!


(G Ann Campbell) #10

Hi,

In my initial testing my server (v7.2) fails to start, even after I install the most recent FindBugs plugin.

I failed to pick up on this earlier (sorry!) but your initial message said your plugin depends on

None of those versions is current. In fact, since the fall, 6.7 has been the SonarQube LTS, and 5.6 hit EOL a couple months ago. Beyond that, SonarJava 4.0 was released two years ago; the current version is 5.5, and the current version of the FindBugs plugin is 3.7.

Sorry, but I cannot add a new plugin that is only compatible with a deprecated version of SonarQube. If you want to bring this up to compatibility with 6.7 and ping me again, I’ll be happy to re-start my testing.

That said, I’d like to give you a couple of totally optional sidenotes:

  • By convention, release artifacts (the plugin jars) are typically attached to the releases in GitHub. You’ve provided Maven download links, and after sorting through the directory, I was able to find what I needed, but it would be easier for users if you gave a direct link to the jar and/or added the jars to the releases in GitHub
  • Your initial message indicates a dependency on the FindBugs plugin, but I didn’t see that in a quick skim of your docs/README.md (maybe it’s there; I didn’t ready thoroughly). When I installed FindBugs via the Marketplace, it upgraded SonarJava to the required version for me, so it may be that this dependency would be handled automatically once you made it into the Marketplace (I don’t have a good way to test that) but it would be helpful/friendly if your README proactively alerted users to the fact that they’ll need both - whether or not the FindBugs install is handled automatically by the Marketplace. That’s especially important in the case of FindBugs since its sensor seems to run whether or not you’ve got FindBugs rules in your profile, and that can have an impact on analysis time.

Ann


(Kengo TODA) #11

Thanks for your reply! :pray:

I also confirmed that my plugin doesn’t work with the latest sonar-java, that uses Guava 19.0.
It seems that I should not package Guava to my plugin.

I’ll test with latest SonarQube LTS, and propose next release v1.0.5.


(Kengo TODA) #12

Hello @ganncamp and all, I’ve released v1.0.5 that supports SonarQube 6.7.4.
You can download jar file from GitHub release page:

Your initial message indicates a dependency on the FindBugs plugin, but I didn’t see that in a quick skim of your docs/README.md

I created a compatibility table in README.md, you may check it. Thanks!


(G Ann Campbell) #13

Hi,

I’ve downloaded the new jar and tested again.

I’ll start with the good stuff:

  • server startup is successful with the new version of your plugin \o/
  • analysis with your rule enabled completes successfully (Note that I was too lazy to come up with a project that would actually violate the rule.)

However, I think this must be addressed:

  • you’ve added your rule to the findbugs repository, and then overridden the name of that repository to be ‘GuavaHelper’. I find this renaming to be deceptive. In fact, your plugin doesn’t add 459 rules; it adds 1. Its FindBugs dependency adds the other 458. Instead, you should create your own repository and add your single rule to it.

And finally, what I think should be addressed:

  • I understand that you wrote this as a FindBugs rule so that you could provide a dual-purpose artifact: SonarQube plugin and SpotBugs plugin. However, this is a bit heavy on the SonarQube side. As I said earlier, the FindBugs sensor runs whether or not you have FindBugs rules enabled, and this can have a strong impact on analysis times, i.e. a high price to pay on a (more than) daily basis for your single rule. I believe this rule could be written for SonarQube without the FindBugs dependency. But that’s up to you.
  • the rule description is rather terse. It would benefit from expansion with at least the first few paragraphs from your README.md.

Ann

P.S. It’s incumbent on me to point out that this same job could be accomplished with SonarJava’s “Track uses of disallowed classes” rule template.


(Kengo TODA) #14

Hi Ann,

Thanks for your review! I will reflect them to the next patch version.

According to this Javadoc in sonar-plugin-api, it is allowed to add rule to existing repository. But I renamed the FindBugs repository wrongly, I’ve fixed my code in this commit.

OK I will consider to implement rules based on sonar-java in next minor version. Firstly let me confirm that it works with sonar-findbugs.

I will update README.md to guide reader to visit the README at the top of project.


(Kengo TODA) #15

Hello, I’ve released v1.0.6 that does not rename FindBugs repository:

Please have a review. Thanks in advance :slight_smile:


(G Ann Campbell) #16

Hi,

I think you’re making a mistake in terms of visibility by adding your rule to the FindBugs repo, but that’s up to you. I’ve added this to the update center.

:slight_smile:
Ann


(Kengo TODA) #17

Confirmed, thank you!


(Tibor Blenessy) #18

Hello @KengoTODA,

sorry to interfere here, but I was bit surprised by this statement.

What kind of issue have you encountered? Usually best way to deliver plugins is to use maven shade plugin and create fat jar by relocating all your dependencies inside your package. That way it shouldn’t matter which version of Guava you use.


(Kengo TODA) #19

I hadn’t note stacktrace, but I remember that basePlugin has dependency on method in legacy Guava that was removed recently. I tried to use useChildFirstClassLoader but it couldn’t be solution.