search

spotbugs / spotbugs-maven-plugin

Maven Mojo Plug-In to generate reports based on the SpotBugs Analyzer
https://spotbugs.github.io/spotbugs-maven-plugin/
Apache License 2.0
69 stars 51 forks source link

Add new parameter to provide SpotBugs with source paths from Maven #71

Closed Irian81 closed 5 years ago

Irian81 commented 5 years ago

Initial PR for review to add support for full source paths on source filter (when available).

The only thing I'm not sure about in this PR is the prerequisite maven version that will not normally compile unless upped to 3.5.3, that will inhibit users from taking advantage of this plugin unless in the 3.5.3 version or upper.

This should be paired with spotbugs PR #700, as it basically enables the functionality there to be used by the maven plugin.

hazendaz commented 5 years ago

I'm not seeing why maven needs updated. We already tried that and the community using this plugin had lots of issues so we need to stay on 3.1.1 support for now.

Get Outlook for Androidhttps://aka.ms/ghei36

From: Irian81 notifications@github.com Sent: Wednesday, July 18, 2018 2:41:53 AM To: spotbugs/spotbugs-maven-plugin Cc: Subscribed Subject: [spotbugs/spotbugs-maven-plugin] Add new parameter to provide SpotBugs with source paths from Maven (#71)

Initial PR for review to add support for full source paths on source filter (when available).

The only thing I'm not sure about in this PR is the prerequisite maven version that will not normally compile unless upped to 3.5.3, that will inhibit users from taking advantage of this plugin unless in the 3.5.3 version or upper.

This should be paired with spotbugs PR #700, as it basically enables the functionality there to be used by the maven plugin.

You can view, comment on, or merge this pull request online at:

https://github.com/spotbugs/spotbugs-maven-plugin/pull/71

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/spotbugs/spotbugs-maven-plugin/pull/71, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AA7ho1Nhgoq8o9YfNsAZA4vjyaWdm24_ks5uHtixgaJpZM4VUB2Q.

Irian81 commented 5 years ago

Sorry it was late in the night yesterday and I forgot to turn off automatic maven home switching, so I was having build problems because my system was automatically switching to maven 3.1.1 for building the plugin and that is not allowed by the enforcer rules.

I've reverted the change so everything should be fine now.

Il 18.07.2018 14:57 Jeremy Landis ha scritto:

I'm not seeing why maven needs updated. We already tried that and the community using this plugin had lots of issues so we need to stay on 3.1.1 support for now.

Get Outlook for Android

From: Irian81 Sent: Wednesday, July 18, 2018 2:41:53 AM To: spotbugs/spotbugs-maven-plugin Cc: Subscribed Subject: [spotbugs/spotbugs-maven-plugin] Add new parameter to provide SpotBugs with source paths from Maven (#71)

Initial PR for review to add support for full source paths on source filter (when available).

The only thing I'm not sure about in this PR is the prerequisite maven version that will not normally compile unless upped to 3.5.3, that will inhibit users from taking advantage of this plugin unless in the 3.5.3 version or upper.

This should be paired with spotbugs PR #700, as it basically enables the functionality there to be used by the maven plugin.

You can view, comment on, or merge this pull request online at:

https://github.com/spotbugs/spotbugs-maven-plugin/pull/71

Commit Summary

  • Add new parameter to provide SpotBugs with source paths from Maven

File Changes

  • M pom.xml (6)
  • M src/main/groovy/org/codehaus/mojo/spotbugs/SpotBugsMojo.groovy (20)

Patch Links:

https://github.com/spotbugs/spotbugs-maven-plugin/pull/71.patch https://github.com/spotbugs/spotbugs-maven-plugin/pull/71.diff

--

You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub, or mute the thread.

-- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or mute the thread [2].

Con MyOpen hai Giga, SMS e i minuti che vuoi da 3€ al mese, per sempre. Cambi gratis quando e come vuoi e in più hai 10€ di credito omaggio! SCOPRI DI PIU’ http://tisca.li/myopen

hazendaz commented 5 years ago

@Irian81 Thanks. I'll hang on this for a while until spot bugs is off snapshot. However, have a question, how does this improve things? Maven already knows it's source, why are we scanning outside of maven source? Seems like an incorrect maven project setup to me.

Irian81 commented 5 years ago

The use case for this can be found in main spotbugs issue 694.

The added source roots are required to let the modified source filter that is being reviewed in PR 700 work, since without this modification to the maven plugin only the target folder is known to spotbugs during the analisys phase so source exlusion based off real source paths would not work.

If you want to test drive this there is an example project attached to issue 694 that with the current version of the maven plugin and spotbugs gives 4 positives, with the modified version of the maven plugin, parameter <addSourceDirs>true</addSourceDirs> and patched spotbugs source filter gives only 2 positives.

PhilMFischer commented 5 years ago

@Irian81 Great work! we are also looking into this issue. Is there anything we can support you with?

hazendaz commented 5 years ago

@PhilMFischer Merge conflicts would need resolved and I think there is still a pice not yet applied on spotbugs as the first link provided in prior commits is still open.

PhilMFischer commented 5 years ago

@hazendaz Yes I see, I will take care of the merge conflict. Just fixed the merge conflict, see PR #111 . Suggest to continue working on PR #111.

The other part you emntion is not totally clear to me. What is still open? What exactly do you mean?

hazendaz commented 5 years ago

@PhilMFischer I think your work is over on PR https://github.com/spotbugs/spotbugs/pull/903 on spotbugs and needs merged before this can. I was originally referring to PR 700.

hazendaz commented 5 years ago

Closing #71 for #111

PhilMFischer commented 5 years ago

@hazendaz Right, i got your point. Do we reopen this PR once the spotbugs/spotbugs#903 is sucessfully merged?