Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      had some time on a plane this weekend, so I adapted some of the clover hooks from Java-Lucene to Solr's build.xml and also put in hooks for running PMD (a bug pattern finding tool).

      the PMD hook actually teste the PMD ruleset twice, once warning about any violations, and once failing the build if any serious violations were found ... the goal would be to hook this into the "ant test" target so you can't successfully build if you have any serious rule violations.

      i strarted with a custom ruleset based on some of the bigger rules from PMD ... the theory being that as well clean up the code base we can add more nit-picky rules if we want to

      User is required to provide their own copy of PMD and/or clover on in an ANT_LIB. Clover requires (ASF committer) license, PMD is freely available...

      http://pmd.sourceforge.net/

      1. pmd-and-clover.diff
        58 kB
        Hoss Man
      2. SOLR-143-CloverAndPMD.patch
        54 kB
        Ryan McKinley

        Issue Links

          Activity

          Hoss Man created issue -
          Hide
          Hoss Man added a comment -

          this patch includes some XSLT/javascript from the PMD 3.9 release so i'm not granting lcense for inclusion until i double check that is viable.

          Show
          Hoss Man added a comment - this patch includes some XSLT/javascript from the PMD 3.9 release so i'm not granting lcense for inclusion until i double check that is viable.
          Hoss Man made changes -
          Field Original Value New Value
          Attachment pmd-and-clover.diff [ 12350420 ]
          Hide
          Yonik Seeley added a comment -

          While checking is nice, I'm not sure about requiring PMD before you can build Solr.
          Is PMD better than the code inspections in IntelliJ/Eclipse?

          Show
          Yonik Seeley added a comment - While checking is nice, I'm not sure about requiring PMD before you can build Solr. Is PMD better than the code inspections in IntelliJ/Eclipse?
          Hide
          Hoss Man added a comment -

          my thinking is that initially PMD would be a completely optional step, which committers are encouraged to run before committing, and which the nightly build runs with the "serious" threshold low enough that only extreme rules cause a build failure. ... the main goal would be having a report on the live site showing where in the nightly builds questionable behavior is (mush as the goal of clover is to have the reports about hte nightlies to see where we need more test coverage)

          i agree – it PMD shouldn't be mandatory just to build Solr.

          I can't make any specific comments about PMD being better then the tools that various IDEs have ... as i understand it many IDEs use either PMD or FindBugs ... my goal was just to have something that could be run as part of theautomated builds.

          (by all means: people using IDEs with code inspectors should run them and fix anything that looks suspicious)

          Show
          Hoss Man added a comment - my thinking is that initially PMD would be a completely optional step, which committers are encouraged to run before committing, and which the nightly build runs with the "serious" threshold low enough that only extreme rules cause a build failure. ... the main goal would be having a report on the live site showing where in the nightly builds questionable behavior is (mush as the goal of clover is to have the reports about hte nightlies to see where we need more test coverage) i agree – it PMD shouldn't be mandatory just to build Solr. I can't make any specific comments about PMD being better then the tools that various IDEs have ... as i understand it many IDEs use either PMD or FindBugs ... my goal was just to have something that could be run as part of theautomated builds. (by all means: people using IDEs with code inspectors should run them and fix anything that looks suspicious)
          Hide
          Ryan McKinley added a comment -

          Updated to apply with trunk – unlike the original patch, this does not try to fix the serious errors (we can do that later)

          For anyone trying to run - this does not require that you have PMD or clover, it just generates reports if you ask for them (and have it configured)

          For anyone trying to run, these are the command lines:
          ant clean
          ant test -Drun.clover=true
          ant clover-reports -Drun.clover=true
          ant pmd-reports

          Is there a reason to have the -Drun.clover configuration rather then the target specifying if clover is used or not?

          Show
          Ryan McKinley added a comment - Updated to apply with trunk – unlike the original patch, this does not try to fix the serious errors (we can do that later) For anyone trying to run - this does not require that you have PMD or clover, it just generates reports if you ask for them (and have it configured) For anyone trying to run, these are the command lines: ant clean ant test -Drun.clover=true ant clover-reports -Drun.clover=true ant pmd-reports Is there a reason to have the -Drun.clover configuration rather then the target specifying if clover is used or not?
          Ryan McKinley made changes -
          Attachment SOLR-143-CloverAndPMD.patch [ 12358992 ]
          Hide
          Hoss Man added a comment -

          Ryan, i haven't looked at your updated patch, but i don't understand your last comment...

          > Is there a reason to have the -Drun.clover configuration rather then the target specifying if clover is used or not?

          how would that work exactly? the run.clover property is what's used to ensure that code is compiled with clover hooks (in the existing "compile" target) and that the clover db is initialized prior to running the unit tests (in the existing "test" target).

          Show
          Hoss Man added a comment - Ryan, i haven't looked at your updated patch, but i don't understand your last comment... > Is there a reason to have the -Drun.clover configuration rather then the target specifying if clover is used or not? how would that work exactly? the run.clover property is what's used to ensure that code is compiled with clover hooks (in the existing "compile" target) and that the clover db is initialized prior to running the unit tests (in the existing "test" target).
          Hide
          Ryan McKinley added a comment -

          >
          > Ryan, i haven't looked at your updated patch, but i don't understand your last comment...
          >

          There is nothing interisting in the changes, it just does not conflict with the recently added init-forrest-entities

          The patch also removes your attempts to fix the PMD serious errors (4 months later, most of the .java files have conflict) We should probalby fix them when this does get added.

          >> Is there a reason to have the -Drun.clover configuration rather then the target specifying if clover is used or not?
          >
          > how would that work exactly? the run.clover property is what's used to ensure that code is compiled with clover hooks (in the existing "compile" target) and that the clover db is initialized prior to running the unit tests (in the existing "test" target).
          >

          magic!

          I just looked into other projects I thought did this - they have different 'compile' tasks for test and dist and require everyone to have clover. This is not appropriate for solr, so the -Drun.clover option is better.

          Show
          Ryan McKinley added a comment - > > Ryan, i haven't looked at your updated patch, but i don't understand your last comment... > There is nothing interisting in the changes, it just does not conflict with the recently added init-forrest-entities The patch also removes your attempts to fix the PMD serious errors (4 months later, most of the .java files have conflict) We should probalby fix them when this does get added. >> Is there a reason to have the -Drun.clover configuration rather then the target specifying if clover is used or not? > > how would that work exactly? the run.clover property is what's used to ensure that code is compiled with clover hooks (in the existing "compile" target) and that the clover db is initialized prior to running the unit tests (in the existing "test" target). > magic! I just looked into other projects I thought did this - they have different 'compile' tasks for test and dist and require everyone to have clover. This is not appropriate for solr, so the -Drun.clover option is better.
          Hide
          Ryan McKinley added a comment -

          what do you think about including this soon? With all the recent changes, it would be nice easily integrate with clover...

          Show
          Ryan McKinley added a comment - what do you think about including this soon? With all the recent changes, it would be nice easily integrate with clover...
          Hide
          Hoss Man added a comment -

          1) i never got a chance to look into the license issues of including the XSLT for
          formating the PMD output in solr

          2) since opening this issue, i have found better ways to do things with PMD instead of needing to run the analysis twice.

          It might be best to just pursue the clover integration separately from the PMD stuff since many people use IDEs that have "code inspectors" for doing PMD type stuff built into them. ... then we can revist PMD later if we want.

          Show
          Hoss Man added a comment - 1) i never got a chance to look into the license issues of including the XSLT for formating the PMD output in solr 2) since opening this issue, i have found better ways to do things with PMD instead of needing to run the analysis twice. It might be best to just pursue the clover integration separately from the PMD stuff since many people use IDEs that have "code inspectors" for doing PMD type stuff built into them. ... then we can revist PMD later if we want.
          Hide
          Paul Sundling added a comment -

          It would be trivial to integrate PMD and clover if you add a maven build. There are already plugins and I've used them successfully myself. They run during "site" generation which generates reports, although they can also be used to fail a build if desired.

          The eclipse plugin for PMD is great, because it includes links to the detailed description on why the issues are a bad practice. There were a few links missing, but I added a patch to add the missing links. However there is a bug in the eclipse plugin where the links stop working if you export the ruleset. (http://sourceforge.net/tracker/index.php?func=detail&aid=1745607&group_id=132036&atid=723729)

          Show
          Paul Sundling added a comment - It would be trivial to integrate PMD and clover if you add a maven build. There are already plugins and I've used them successfully myself. They run during "site" generation which generates reports, although they can also be used to fail a build if desired. The eclipse plugin for PMD is great, because it includes links to the detailed description on why the issues are a bad practice. There were a few links missing, but I added a patch to add the missing links. However there is a bug in the eclipse plugin where the links stop working if you export the ruleset. ( http://sourceforge.net/tracker/index.php?func=detail&aid=1745607&group_id=132036&atid=723729 )
          Hoss Man made changes -
          Link This issue incorporates SOLR-479 [ SOLR-479 ]
          Hide
          Hoss Man added a comment -

          Grant did a much cleaner job of adding Clover support to the build files in SOLR-479

          eventually i'd like to come back and add the PMD support in, but it's a pretty low priority (particularly until some other issues with our automated builds get resolved)

          Show
          Hoss Man added a comment - Grant did a much cleaner job of adding Clover support to the build files in SOLR-479 eventually i'd like to come back and add the PMD support in, but it's a pretty low priority (particularly until some other issues with our automated builds get resolved)
          Hoss Man made changes -
          Link This issue is related to LUCENE-3973 [ LUCENE-3973 ]

            People

            • Assignee:
              Unassigned
              Reporter:
              Hoss Man
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:

                Development