Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

Irian81
Copy link

@Irian81 Irian81 commented Jul 18, 2018

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
Copy link
Member

hazendaz commented Jul 18, 2018 via email

@Irian81
Copy link
Author

Irian81 commented Jul 18, 2018 via email

@hazendaz
Copy link
Member

@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
Copy link
Author

Irian81 commented Jul 20, 2018

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
Copy link

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

@hazendaz
Copy link
Member

hazendaz commented Mar 10, 2019

@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
Copy link

PhilMFischer commented Mar 11, 2019

@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
Copy link
Member

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

@hazendaz
Copy link
Member

Closing #71 for #111

@hazendaz hazendaz closed this Mar 13, 2019
@PhilMFischer
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants