11 Replies Latest reply on Jan 19, 2011 8:08 AM by Magesh Bojan

    Findbugs

    Keith Babo Master

      I just pushed Magesh's change in to enable Findbugs.  A few questions/requests:

      • Can we push this to an earlier lifecycle phase in Maven so it gets picked up by the install goal?
      • Is it possible to capture all the reports in one place?  Ideally the build directory.  Right now, you have to go around and check each module's site directory to read the reports.
      • Can we get a very visible notification that findbugs has failed?  I guess one way to do this is to have the build actually fail as a result, but I have a feeling that could get seriously annoying.  My preference would be for there to be a profile setting that allowed developers to successfully run install with findbugs issues but for the Hudson build to fail.

       

      thx

        • 1. Re: Findbugs
          Magesh Bojan Master

          Keith Babo wrote

          • Can we push this to an earlier lifecycle phase in Maven so it gets picked up by the install goal?

           

          Yes can be moved to compile time. See my updated branch http://github.com/mageshbk/core/tree/findbugs

           

          • Is it possible to capture all the reports in one place?  Ideally the build directory.  Right now, you have to go around and check each module's site directory to read the reports.

          Not possible in build, but maybe possible to aggregate the reports into the top level target/site directory. The maven findbugs plugin does not support aggregating, but can be done with the use of another plugin, the dashboard.

          • Can we get a very visible notification that findbugs has failed?  I guess one way to do this is to have the build actually fail as a result, but I have a feeling that could get seriously annoying.  My preference would be for there to be a profile setting that allowed developers to successfully run install with findbugs issues but for the Hudson build to fail.

          One other way I can think of is to have 2 different plugin configurations one for compile time and the other for deploy time. So when we try to deploy to a remote site we can make it fail. Hudson has its own plugin for consolidating findbugs results. This could be configured to show as an unstable build when errors exists.

          • 2. Re: Findbugs
            Keith Babo Master

            Per our discussion on IRC, let's bind findbugs to the verify phase.

             

            Re: the Hudson config, we should be able to use profiles to selectively set properties used in our poms.  That said, if Hudson already has a findbugs plugin that we can use separately, then maybe that's best.

            • 3. Re: Findbugs
              Tom Cunningham Master

              Proposal : I'd like to enable findbugs by default in the site goal (as well as it being enable-able manually by setting the property or having it in your profile like it is now).    Running mvn site is how we get checkstyle to run right now, so when I run a check before submitting a pull request it'd be cool if I could just do mvn site so I can get findbugs as well as checkstyle.

              • 4. Re: Findbugs
                Keith Babo Master

                Another option would be to have checkstyle and findbugs enabled with a specific maven profile.  You could then activate this profile when you are running a pre-commit/pre-pull-request build.  Obviously, we would want to use the same profile in our Hudsdon build.  Aside from convenience, one additional advantage of using a profile is that it's an easy way to keep the CI build settings in sync with your pre-commit settings.

                 

                No matter which route we go, it would be nice if checkstyle and findbugs can be selectively enabled by using a system property.   We should create a common root for these property names (as I imagine there will be more) and publish a community article to track them.

                • 5. Re: Findbugs
                  Tom Cunningham Master

                  Okay - so just so I have this straight :

                   

                  Properties :

                  -Dorg.switchyard.findbugs.enable : enables findbugs

                  -Dorg.switchyard.checkstyle.enable : enables checkstyle

                  -Dorg.switchyard.precommit.enable : enables all precommit checks, including findbugs, checkstyle, and any future checks we might add including integration tests

                   

                  In addition, checkstyle will move from the site goal to the install goal but will be disabled by default.

                  • 6. Re: Findbugs
                    Keith Babo Master

                    Yes on the first two.  The third one should actually be a maven profile, no?  IOW, it would be mvn -Pintegration.build or something similar.

                    • 7. Re: Findbugs
                      Tom Cunningham Master

                      All three would be profiles, I think we're just down to the semantics of how to enable them (either -P or a property based activation -D).     I think we're on the same page here, I'll go ahead and make the changes.

                      • 8. Re: Findbugs
                        Keith Babo Master

                        Saw your pull request, Tom.  Thanks again for taking care of this.  A few questions:

                         

                        • I didn't see the all-in-one profile.  Just wanted to confirm that you are going to add that later.
                        • Findbugs appears to work with -Pfindbugs in the install phase, but not in the site phase
                        • Checkstyle appears to work with -Pcheckstyle in the site phase, but not in the install phase
                        • I noticed that checkstyle can also be activated via a property, while findbugs cannot.  Any reason why we would do properties instead of just relying on -P ?  Maybe we use a property to enable the all-in-one?
                        • Is the dashboard report supposed to include output from findbugs and checkstyle?  At present, I only see test results in there.

                         

                        cheers,

                        keith

                        • 9. Re: Findbugs
                          Tom Cunningham Master

                          Okay, here's what I ended up doing :

                           

                          Findbugs can be activated -Pfindbugs

                          Checkstyle can be activated -Pcheckstyle

                          If you want to activate both (and any other precommit checks we might add later), do a -Dintegration.build=true

                           

                          The reason I think we should do activation by shared property is because there is no way in maven to have a profile that runs other profiles (I think there's an enhancement bug in there for maven 3 on this, but it isn't fixed).         We should allow people to run one or the other, and the best way to combine them, other than duplicating the same XML, is to have them share a common activation property.

                           

                          I think the findbugs problem mentioned above is due to running on maven-3.0.1.     I'm not seeing any issues with maven-2.2.1, and if I run  :

                           

                          mvn -Dintegration.build=true clean install

                          mvn site

                           

                          I see both findbus and checkstyle (and surefire) reports within the dashboard results.

                          • 10. Re: Findbugs
                            Keith Babo Master

                            Agreed on the system property to activate multiple profiles for the integration build.

                             

                            Thanks for the heads up on 3.0.1 vs. 2.2.1.

                            • 11. Findbugs
                              Magesh Bojan Master

                              Exception is caught when Exception is not thrown

                               

                              Does this really need to be checked? Keith feels it can be removed as long as there is reasonable exception handling procedures among the developers. Let me know what you guys think.