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

Printing static code analysis errors in log #236

Closed
wants to merge 2 commits into from
Closed

Printing static code analysis errors in log #236

wants to merge 2 commits into from

Conversation

tomasbjerre
Copy link

The build log may look like:

[INFO] 
[INFO] --- violations-maven-plugin:1.24:violations (default) @ generic-webhook-trigger ---
[INFO] No references specified, will not report violations in diff
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  55.612 s
[INFO] Finished at: 2019-10-05T10:38:50+02:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal se.bjurr.violations:violations-maven-plugin:1.24:violations (default) on project generic-webhook-trigger: Too many violations found, max is 0 but found 2
[ERROR] org/jenkinsci/plugins/gwt/GenericTrigger.java
[ERROR] |          |          |          |      |                                   |
[ERROR] | Reporter | Rule     | Severity | Line | Message                           |
[ERROR] |          |          |          |      |                                   |
[ERROR] +----------+----------+----------+------+-----------------------------------+
[ERROR] |          |          |          |      |                                   |
[ERROR] | Spotbugs | RV_RETUR | INFO     | 144  | Return value  of  method  without |
[ERROR] |          | N_VALUE_ |          |      | side effect  is  ignored  <p>This |
[ERROR] |          | IGNORED_ |          |      | code calls a method  and  ignores |
[ERROR] |          | NO_SIDE_ |          |      | the  return  value.  However  our |
[ERROR] |          | EFFECT   |          |      | analysis shows  that  the  method |
[ERROR] |          |          |          |      | (including its implementations in |
[ERROR] |          |          |          |      | subclasses  if  any)   does   not |
[ERROR] |          |          |          |      | produce  any  effect  other  than |
[ERROR] |          |          |          |      | return value. Thus this call  can |
[ERROR] |          |          |          |      | be removed. </p> <p>We are trying |
[ERROR] |          |          |          |      | to reduce the false positives  as |
[ERROR] |          |          |          |      | much as  possible,  but  in  some |
[ERROR] |          |          |          |      | cases  this  warning   might   be |
[ERROR] |          |          |          |      | wrong.   Common    false-positive |
[ERROR] |          |          |          |      | cases   include:</p>   <p>-   The |
[ERROR] |          |          |          |      | method   is   designed   to    be |
[ERROR] |          |          |          |      | overridden  and  produce  a  side |
[ERROR] |          |          |          |      | effect in  other  projects  which |
[ERROR] |          |          |          |      | are  out  of  the  scope  of  the |
[ERROR] |          |          |          |      | analysis.</p> <p>- The method  is |
[ERROR] |          |          |          |      | called  to  trigger   the   class |
[ERROR] |          |          |          |      | loading which  may  have  a  side |
[ERROR] |          |          |          |      | effect.</p> <p>-  The  method  is |
[ERROR] |          |          |          |      | called   just   to    get    some |
[ERROR] |          |          |          |      | exception.</p>  <p>If  you   feel |
[ERROR] |          |          |          |      | that our assumption is incorrect, |
[ERROR] |          |          |          |      | you can use  a  @CheckReturnValue |
[ERROR] |          |          |          |      | annotation to  instruct  FindBugs |
[ERROR] |          |          |          |      | that ignoring the return value of |
[ERROR] |          |          |          |      | this method is acceptable. </p>   |
[ERROR] |          |          |          |      |                                   |
[ERROR] +----------+----------+----------+------+-----------------------------------+
[ERROR] |          |          |          |      |                                   |
[ERROR] | Spotbugs | NP_NULL_ | ERROR    | 140  | Possible null pointer dereference |
[ERROR] |          | ON_SOME_ |          |      | <p>  There   is   a   branch   of |
[ERROR] |          | PATH     |          |      | statement      that,       <em>if |
[ERROR] |          |          |          |      | executed,</em> guarantees that  a |
[ERROR] |          |          |          |      | null value will be  dereferenced, |
[ERROR] |          |          |          |      | which    would     generate     a |
[ERROR] |          |          |          |      | <code>NullPointerException</code> |
[ERROR] |          |          |          |      | when the  code  is  executed.  Of |
[ERROR] |          |          |          |      | course, the problem might be that |
[ERROR] |          |          |          |      | the  branch   or   statement   is |
[ERROR] |          |          |          |      | infeasible  and  that  the   null |
[ERROR] |          |          |          |      | pointer exception can't  ever  be |
[ERROR] |          |          |          |      | executed; deciding that is beyond |
[ERROR] |          |          |          |      | the ability of FindBugs. </p>     |
[ERROR] |          |          |          |      |                                   |
[ERROR] +----------+----------+----------+------+-----------------------------------+
[ERROR] Summary of org/jenkinsci/plugins/gwt/GenericTrigger.java
[ERROR] |          |      |      |       |       |
[ERROR] | Reporter | INFO | WARN | ERROR | Total |
[ERROR] |          |      |      |       |       |
[ERROR] +----------+------+------+-------+-------+
[ERROR] |          |      |      |       |       |
[ERROR] | Spotbugs | 1    | 0    | 1     | 2     |
[ERROR] |          |      |      |       |       |
[ERROR] +----------+------+------+-------+-------+
[ERROR] |          |      |      |       |       |
[ERROR] |          | 1    | 0    | 1     | 2     |
[ERROR] |          |      |      |       |       |
[ERROR] +----------+------+------+-------+-------+
[ERROR] 
[ERROR] 
[ERROR] Summary
[ERROR] |          |      |      |       |       |
[ERROR] | Reporter | INFO | WARN | ERROR | Total |
[ERROR] |          |      |      |       |       |
[ERROR] +----------+------+------+-------+-------+
[ERROR] |          |      |      |       |       |
[ERROR] | Spotbugs | 1    | 0    | 1     | 2     |
[ERROR] |          |      |      |       |       |
[ERROR] +----------+------+------+-------+-------+
[ERROR] |          |      |      |       |       |
[ERROR] |          | 1    | 0    | 1     | 2     |
[ERROR] |          |      |      |       |       |
[ERROR] +----------+------+------+-------+-------+

@oleg-nenashev
Copy link
Member

Would be nice to have several downstream pull requests demonstrating the enhancement

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT it will cause build failures in plugins which disable FindBugs now

pom.xml Outdated Show resolved Hide resolved
tomasbjerre added a commit to jenkinsci/generic-webhook-trigger-plugin that referenced this pull request Oct 5, 2019
@tomasbjerre
Copy link
Author

tomasbjerre commented Oct 5, 2019

Would be nice to have several downstream pull requests demonstrating the enhancement

jenkinsci/gerrit-trigger-plugin#402
jenkinsci/generic-webhook-trigger-plugin#137
jenkinsci/cloudbees-folder-plugin#134

@oleg-nenashev oleg-nenashev self-requested a review October 5, 2019 10:25
tomasbjerre added a commit to jenkinsci/generic-webhook-trigger-plugin that referenced this pull request Oct 5, 2019
tomasbjerre added a commit to jenkinsci/generic-webhook-trigger-plugin that referenced this pull request Oct 5, 2019
tomasbjerre added a commit to jenkinsci/generic-webhook-trigger-plugin that referenced this pull request Oct 5, 2019
tomasbjerre added a commit to jenkinsci/generic-webhook-trigger-plugin that referenced this pull request Oct 5, 2019
@tomasbjerre
Copy link
Author

tomasbjerre commented Oct 5, 2019

@oleg-nenashev I added profiles to the parent. But I when I try it out in jenkinsci/generic-webhook-trigger-plugin#137 they are not getting activated. I use failOnError = false but still it does fail. mvn help:active-profiles does not show my profiles being activated. Not really sure why. Will continue investigate that, any help is welcome =)

Edit: Seems only system properties can be used to activate profiles: https://stackoverflow.com/questions/5676231/how-to-activate-profile-by-means-of-maven-property

Perhaps I'll just change the PR so that the maxViolations = 999999. And just document that a user can use the feature by updating pom like:

        <violations.maxViolations>0</violations.maxViolations>
        <findbugs.failOnError>false</findbugs.failOnError>
        <spotbugs.failOnError>false</spotbugs.failOnError>

Edit2: Changed implementation to the above.

tomasbjerre added a commit to jenkinsci/generic-webhook-trigger-plugin that referenced this pull request Oct 5, 2019
@tomasbjerre tomasbjerre changed the title Printing Spotbugs errors in log before failing build Printing static code analysis errors in log Oct 6, 2019
@jetersen
Copy link
Member

jetersen commented Oct 6, 2019

@tomasbjerre force pushing makes it harder to reivew 😓 If you wish for the commit to be squashed it can be done so.

To allow control over column widths.
@oleg-nenashev oleg-nenashev requested a review from a team October 14, 2019 05:55
Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am generally against a change like this for the following reasons:

  1. it unconditionally adds another plugin execution into the lifecycle (for questionable gain - see Needed a dep on jenkins.war itself for use from hpi:run #3)
  2. it is not intelligent about which issues it will find - thus if you actually have some test files (say in a checksytle/warnings plugin then it will pick those ut)

but mainly:

  1. Jenkins already has a lovely way to report these anyway - all plugins can enable these reports and all users can view them (including trends today) whn the plugin is built on ci.jenkins.io

I can see the potential use when you are developing locally and running mvn verify - but in reality does anyone not use an IDE which can enable all of this directly in the IDE to prevent you even having to drop to the command line?

@jtnord jtnord requested a review from a team October 14, 2019 09:29
@daniel-beck
Copy link
Member

@tomasbjerre Thanks for this proposal! Having to search what the stupid error codes mean is annoying, and this looks pretty helpful in that case.

However, beyond concerns like James' around the functionality and usefulness itself, this looks pretty much unproven: This has no known uses on mvnrepository.com, no stars or forks on GitHub, and the only notable uses GitHub identifies are plugins and other components by its own maintainer.

Which means to me that, so far, the plugin basically just had to deal with issues its maintainer threw at it, and may well have problems when exposed to the wider plugin ecosystem. I realize this is kind of a chicken-and-egg problem for Tomas as a maintainer, but I'd really like to see something be more battle-tested before we roll it out this widely.


I would propose you open a discussion on the dev list asking for plugin maintainers' support for this change, as ultimately it's plugin maintainers who end up using this (and having to deal with the resulting problems).

And finally, even if there's no strong support, individual plugin maintainer can always choose to use this -- they just wouldn't get it through the parent POM.

@jetersen
Copy link
Member

There is only one way to get it battle tested :)
I'd say go for it, people can either disable the feature or we can revert the addition if it becomes a huge problem.

@jtnord
Copy link
Member

jtnord commented Oct 14, 2019

Jenkins already has a lovely way to report these anyway - all plugins can enable these reports and all users can view them (including trends today) whn the plugin is built on ci.jenkins.io

just seen @uhafner's analysis reports where never installed or have been uninstalled on the server :-o
INFRA-2294

@uhafner
Copy link
Member

uhafner commented Oct 14, 2019

They are still installed, e.g. see https://ci.jenkins.io/job/Plugins/job/analysis-model/job/master/718/spotbugs/

@olivergondza
Copy link
Member

I find it safer to get this "battle tested" on handfull of the plugins/components first and only promote it to parent-pom for everyone after positive reception.

@jtnord
Copy link
Member

jtnord commented Dec 18, 2019

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.

7 participants