Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.14.0
    • Component/s: Build Infrastructure
    • Labels:
      None

      Description

      ant had a checkstyle target, we should do something similar for maven

      1. HIVE-6123.2.patch
        3 kB
        Lars Francke
      2. HIVE-6123.1.patch
        2 kB
        Remus Rusanu

        Issue Links

          Activity

          Hide
          Thejas M Nair added a comment -

          This has been fixed in 0.14 release. Please open new jira if you see any issues.

          Show
          Thejas M Nair added a comment - This has been fixed in 0.14 release. Please open new jira if you see any issues.
          Hide
          Lars Francke added a comment -

          I documented this.

          Show
          Lars Francke added a comment - I documented this.
          Hide
          Brock Noland added a comment -

          Lars Francke I have created HIVE-7933 to add the check to the build.

          Show
          Brock Noland added a comment - Lars Francke I have created HIVE-7933 to add the check to the build.
          Hide
          Lefty Leverenz added a comment -

          Doc note: As mentioned previously, this should be documented in the Coding Conventions section of How To Contribute.

          Show
          Lefty Leverenz added a comment - Doc note: As mentioned previously, this should be documented in the Coding Conventions section of How To Contribute. How To Contribute – Coding Conventions
          Hide
          Ashutosh Chauhan added a comment -

          Committed to trunk. Thanks, Lars!
          I think its a good idea to enhance ptest framework to fail the build if a patch increases checkstyle warnings, that way Hive QA will refuse to run the build. It will be awesome if someone takes that up. Brock Noland / Szehon Ho might provider pointers on how to make that happen.

          Show
          Ashutosh Chauhan added a comment - Committed to trunk. Thanks, Lars! I think its a good idea to enhance ptest framework to fail the build if a patch increases checkstyle warnings, that way Hive QA will refuse to run the build. It will be awesome if someone takes that up. Brock Noland / Szehon Ho might provider pointers on how to make that happen.
          Hide
          Lars Francke added a comment -

          Thanks Ashutosh.

          We could also easily run Checkstyle during every build but that'd make the build slightly longer. It'd be great to extend the Jenkins bot to run checkstyle and to diff previous checkstyle results to new ones and do a -1 when new issues are introduced. I think Hadoop or HBase do this. Probably better to do this in a new JIRA

          Show
          Lars Francke added a comment - Thanks Ashutosh. We could also easily run Checkstyle during every build but that'd make the build slightly longer. It'd be great to extend the Jenkins bot to run checkstyle and to diff previous checkstyle results to new ones and do a -1 when new issues are introduced. I think Hadoop or HBase do this. Probably better to do this in a new JIRA
          Hide
          Ashutosh Chauhan added a comment -

          +1

          Show
          Ashutosh Chauhan added a comment - +1
          Hide
          Lars Francke added a comment -
          • mvn checkstyle:checkstyle is a reporting goal, means it builds a HTML page in the respective target/site folder.
          • {{mvn checkstyle:checkstyle-aggregate generates one HTML report which combines all the submodule's run.
          • mvn checkstyle:check is run as part of the build and can fail it if needed. To actually see any output use something like this: mvn checkstyle:check -Phadoop-2 -DskipTests -Dcheckstyle.consoleOutput=true

          None of these run by default atm. The only one that really makes sense is the check goal.

          This is an example invocation:
          mvn compile checkstyle:check -Phadoop-2 -DskipTests -Dcheckstyle.consoleOutput=true -Dcheckstyle.violationSeverity=warning

          We could think about something adding this to the build:
          mvn compile checkstyle:check verify -Phadoop-2 -DskipTests -Dcheckstyle.consoleOutput=true
          That'd show the warnings on each build but not fail it. Unfortunately there's now way to distinguish between "grandfathered" and new code.

          Show
          Lars Francke added a comment - mvn checkstyle:checkstyle is a reporting goal, means it builds a HTML page in the respective target/site folder. {{mvn checkstyle:checkstyle-aggregate generates one HTML report which combines all the submodule's run. mvn checkstyle:check is run as part of the build and can fail it if needed. To actually see any output use something like this: mvn checkstyle:check -Phadoop-2 -DskipTests -Dcheckstyle.consoleOutput=true None of these run by default atm. The only one that really makes sense is the check goal. This is an example invocation: mvn compile checkstyle:check -Phadoop-2 -DskipTests -Dcheckstyle.consoleOutput=true -Dcheckstyle.violationSeverity=warning We could think about something adding this to the build: mvn compile checkstyle:check verify -Phadoop-2 -DskipTests -Dcheckstyle.consoleOutput=true That'd show the warnings on each build but not fail it. Unfortunately there's now way to distinguish between "grandfathered" and new code.
          Hide
          Ashutosh Chauhan added a comment -

          I tried above two targets after applying the patch. mvn command seems to execute successfully but no violations were reported. Seems like it no checks were performed. For checkstyle:checkstyle target I saw following warnings :

          [WARNING] Unable to locate Source XRef to link to - DISABLED
          
          Show
          Ashutosh Chauhan added a comment - I tried above two targets after applying the patch. mvn command seems to execute successfully but no violations were reported. Seems like it no checks were performed. For checkstyle:checkstyle target I saw following warnings : [WARNING] Unable to locate Source XRef to link to - DISABLED
          Hide
          Lars Francke added a comment -

          I'd assume this comes with no "obligation" at all. The way it's currently implemented just enables anyone to use the checkstyle plugin manually using mvn checkstyle:checkstyle or mvn checkstyle:check.

          I suggest implementing any automatism if wanted in a follow-up JIRA.

          Show
          Lars Francke added a comment - I'd assume this comes with no "obligation" at all. The way it's currently implemented just enables anyone to use the checkstyle plugin manually using mvn checkstyle:checkstyle or mvn checkstyle:check . I suggest implementing any automatism if wanted in a follow-up JIRA.
          Hide
          Brock Noland added a comment -

          I think having checkstyle would be great. Would this grandfather existing code so that only new code would be required to meet checkstyle?

          Show
          Brock Noland added a comment - I think having checkstyle would be great. Would this grandfather existing code so that only new code would be required to meet checkstyle?
          Hide
          Hive QA added a comment -

          Overall: -1 at least one tests failed

          Here are the results of testing the latest attachment:
          https://issues.apache.org/jira/secure/attachment/12658517/HIVE-6123.2.patch

          ERROR: -1 due to 5 failed/errored test(s), 5877 tests executed
          Failed tests:

          org.apache.hadoop.hive.cli.TestCliDriver.testCliDriver_parquet_mixed_case
          org.apache.hadoop.hive.cli.TestMiniTezCliDriver.testCliDriver_dynpart_sort_opt_vectorization
          org.apache.hadoop.hive.cli.TestMinimrCliDriver.testCliDriver_ql_rewrite_gbtoidx
          org.apache.hadoop.hive.ql.TestDDLWithRemoteMetastoreSecondNamenode.testCreateTableWithIndexAndPartitionsNonDefaultNameNode
          org.apache.hive.jdbc.miniHS2.TestHiveServer2.testConnection
          

          Test results: http://ec2-174-129-184-35.compute-1.amazonaws.com/jenkins/job/PreCommit-HIVE-TRUNK-Build/181/testReport
          Console output: http://ec2-174-129-184-35.compute-1.amazonaws.com/jenkins/job/PreCommit-HIVE-TRUNK-Build/181/console
          Test logs: http://ec2-174-129-184-35.compute-1.amazonaws.com/logs/PreCommit-HIVE-TRUNK-Build-181/

          Messages:

          Executing org.apache.hive.ptest.execution.PrepPhase
          Executing org.apache.hive.ptest.execution.ExecutionPhase
          Executing org.apache.hive.ptest.execution.ReportingPhase
          Tests exited with: TestsFailedException: 5 tests failed
          

          This message is automatically generated.

          ATTACHMENT ID: 12658517

          Show
          Hive QA added a comment - Overall : -1 at least one tests failed Here are the results of testing the latest attachment: https://issues.apache.org/jira/secure/attachment/12658517/HIVE-6123.2.patch ERROR: -1 due to 5 failed/errored test(s), 5877 tests executed Failed tests: org.apache.hadoop.hive.cli.TestCliDriver.testCliDriver_parquet_mixed_case org.apache.hadoop.hive.cli.TestMiniTezCliDriver.testCliDriver_dynpart_sort_opt_vectorization org.apache.hadoop.hive.cli.TestMinimrCliDriver.testCliDriver_ql_rewrite_gbtoidx org.apache.hadoop.hive.ql.TestDDLWithRemoteMetastoreSecondNamenode.testCreateTableWithIndexAndPartitionsNonDefaultNameNode org.apache.hive.jdbc.miniHS2.TestHiveServer2.testConnection Test results: http://ec2-174-129-184-35.compute-1.amazonaws.com/jenkins/job/PreCommit-HIVE-TRUNK-Build/181/testReport Console output: http://ec2-174-129-184-35.compute-1.amazonaws.com/jenkins/job/PreCommit-HIVE-TRUNK-Build/181/console Test logs: http://ec2-174-129-184-35.compute-1.amazonaws.com/logs/PreCommit-HIVE-TRUNK-Build-181/ Messages: Executing org.apache.hive.ptest.execution.PrepPhase Executing org.apache.hive.ptest.execution.ExecutionPhase Executing org.apache.hive.ptest.execution.ReportingPhase Tests exited with: TestsFailedException: 5 tests failed This message is automatically generated. ATTACHMENT ID: 12658517
          Hide
          Lars Francke added a comment -

          I've taken the liberty to work on this: https://reviews.apache.org/r/24075/

          Show
          Lars Francke added a comment - I've taken the liberty to work on this: https://reviews.apache.org/r/24075/
          Hide
          Lefty Leverenz added a comment - - edited

          When this is ready, it will be documented in the Coding Conventions section of How To Contribute.

          https://cwiki.apache.org/confluence/display/Hive/HowToContribute#HowToContribute-CodingConventions

          Edit: fixed link to wiki.

          Show
          Lefty Leverenz added a comment - - edited When this is ready, it will be documented in the Coding Conventions section of How To Contribute. https://cwiki.apache.org/confluence/display/Hive/HowToContribute#HowToContribute-CodingConventions Edit: fixed link to wiki.
          Hide
          Remus Rusanu added a comment -

          I uploaded a patch that enables the maven checkstyle plugin. But trying to use the old ant checkstyle conf (uncomment `<!-- <configLocation>$

          {checkstyle.conf.dir}

          /checkstyle.xml</configLocation> -->` in pom.xml) results in an error when loading the plugin: "Failed during checkstyle configuration: cannot initialize module JavadocPackage - Unable to instantiate JavadocPackage: Unable to instantiate JavadocPackageCheck"

          Show
          Remus Rusanu added a comment - I uploaded a patch that enables the maven checkstyle plugin. But trying to use the old ant checkstyle conf (uncomment `<!-- <configLocation>$ {checkstyle.conf.dir} /checkstyle.xml</configLocation> -->` in pom.xml) results in an error when loading the plugin: "Failed during checkstyle configuration: cannot initialize module JavadocPackage - Unable to instantiate JavadocPackage: Unable to instantiate JavadocPackageCheck"

            People

            • Assignee:
              Lars Francke
              Reporter:
              Brock Noland
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development