Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-13583

Incorporate checkcompatibility script which runs Java API Compliance Checker

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.6.4
    • Fix Version/s: 2.8.0, 3.0.0-alpha2
    • Component/s: scripts
    • Labels:
      None
    • Target Version/s:

      Description

      Based on discussion at YETUS-445, this code can't go there, but it's still very useful for release managers. A similar variant of this script has been used for a while by Apache HBase and Apache Kudu, and IMO JACC output is easier to understand than JDiff.

      1. HADOOP-13583.001.patch
        12 kB
        Andrew Wang
      2. HADOOP-13583.002.patch
        13 kB
        Andrew Wang
      3. HADOOP-13583.003.patch
        13 kB
        Andrew Wang

        Issue Links

          Activity

          Hide
          andrew.wang Andrew Wang added a comment -

          Patch attached. This is the same code as what was at YETUS-445.

          Show
          andrew.wang Andrew Wang added a comment - Patch attached. This is the same code as what was at YETUS-445 .
          Hide
          busbey Sean Busbey added a comment -

          +1 (non-binding)

          one nit:

          +# compatibility checker from the HBase project, but ported to
          

          should be Apache HBase.

          Show
          busbey Sean Busbey added a comment - +1 (non-binding) one nit: +# compatibility checker from the HBase project, but ported to should be Apache HBase.
          Hide
          rkanter Robert Kanter added a comment -

          Looks good overall. I was actually partway through working on something like this before I saw you already did it

          A few minor things:

          1. Missing space after "classes." in help text:
              -a ANNOTATIONS, --annotation ANNOTATIONS
                                    Fully-qualified Java annotation. Java ACC will only
                                    check compatibility of annotated classes.Can be
                                    specified multiple times.
            
          2. Maybe add a few useful/common example usages to the BUILDING.txt file? e.g. "Use these arguments to check HDFS compatibility between Hadoop 2.7 and 2.8". Otherwise, there's a lot of initial guesswork to figure out what you actually want for -i, -e, and -a.
          3. JAVA_ACC_GIT_URL isn't used anywhere. This currently points to the git repo but we're actually downloading a tarball of the 1.8 release elsewhere.
          4. I ran into some problems getting it to run because I didn't have some of the target directories it needed (./target/ and ./dev-support/target/. It was also missing a directory level when trying to run japi-compliance-checker.pl. I made a few changes to fix all this:
            diff --git a/dev-support/bin/checkcompatibility.py b/dev-support/bin/checkcompatibility.py
            old mode 100644
            new mode 100755
            index 7725297..e441f34
            --- a/dev-support/bin/checkcompatibility.py
            +++ b/dev-support/bin/checkcompatibility.py
            @@ -132,6 +132,8 @@ def checkout_java_acc(force):
                   return
                 logging.info("Forcing re-download.")
                 shutil.rmtree(acc_dir)
            +  else:
            +    os.makedirs(acc_dir)
               logging.info("Downloading Java ACC...")
            
               url = "https://github.com/lvc/japi-compliance-checker/archive/1.8.tar.gz"
            @@ -169,7 +171,7 @@ def run_java_acc(src_name, src_jars, dst_name, dst_jars, annotations):
                            "\n\t".join(src_jars),
                            "\n\t".join(dst_jars))
            
            -  java_acc_path = os.path.join(get_java_acc_dir(), "japi-compliance-checker.pl")
            +  java_acc_path = os.path.join(get_java_acc_dir(), "japi-compliance-checker-1.8", "japi-compliance-checker.pl")
            
               src_xml_path = os.path.join(get_scratch_dir(), "src.xml")
               dst_xml_path = os.path.join(get_scratch_dir(), "dst.xml")
            @@ -292,9 +294,6 @@ def main():
                 for a in annotations:
                   logging.info("\t%s", a)
            
            -  # Download deps.
            -  checkout_java_acc(args.force_download)
            -
               # Set up the build.
               scratch_dir = get_scratch_dir()
               src_dir = os.path.join(scratch_dir, "src")
            @@ -308,6 +307,9 @@ def main():
                 checkout_java_tree(get_git_hash(src_rev), src_dir)
                 checkout_java_tree(get_git_hash(dst_rev), dst_dir)
            
            +  # Download deps.
            +  checkout_java_acc(args.force_download)
            +
               # Run the build in each.
               if args.skip_build:
                 logging.info("Skipping the build")
            
          Show
          rkanter Robert Kanter added a comment - Looks good overall. I was actually partway through working on something like this before I saw you already did it A few minor things: Missing space after "classes." in help text: -a ANNOTATIONS, --annotation ANNOTATIONS Fully-qualified Java annotation. Java ACC will only check compatibility of annotated classes.Can be specified multiple times. Maybe add a few useful/common example usages to the BUILDING.txt file? e.g. "Use these arguments to check HDFS compatibility between Hadoop 2.7 and 2.8". Otherwise, there's a lot of initial guesswork to figure out what you actually want for -i , -e , and -a . JAVA_ACC_GIT_URL isn't used anywhere. This currently points to the git repo but we're actually downloading a tarball of the 1.8 release elsewhere. I ran into some problems getting it to run because I didn't have some of the target directories it needed ( ./target/ and ./dev-support/target/ . It was also missing a directory level when trying to run japi-compliance-checker.pl . I made a few changes to fix all this: diff --git a/dev-support/bin/checkcompatibility.py b/dev-support/bin/checkcompatibility.py old mode 100644 new mode 100755 index 7725297..e441f34 --- a/dev-support/bin/checkcompatibility.py +++ b/dev-support/bin/checkcompatibility.py @@ -132,6 +132,8 @@ def checkout_java_acc(force): return logging.info("Forcing re-download.") shutil.rmtree(acc_dir) + else: + os.makedirs(acc_dir) logging.info("Downloading Java ACC...") url = "https://github.com/lvc/japi-compliance-checker/archive/1.8.tar.gz" @@ -169,7 +171,7 @@ def run_java_acc(src_name, src_jars, dst_name, dst_jars, annotations): "\n\t".join(src_jars), "\n\t".join(dst_jars)) - java_acc_path = os.path.join(get_java_acc_dir(), "japi-compliance-checker.pl") + java_acc_path = os.path.join(get_java_acc_dir(), "japi-compliance-checker-1.8", "japi-compliance-checker.pl") src_xml_path = os.path.join(get_scratch_dir(), "src.xml") dst_xml_path = os.path.join(get_scratch_dir(), "dst.xml") @@ -292,9 +294,6 @@ def main(): for a in annotations: logging.info("\t%s", a) - # Download deps. - checkout_java_acc(args.force_download) - # Set up the build. scratch_dir = get_scratch_dir() src_dir = os.path.join(scratch_dir, "src") @@ -308,6 +307,9 @@ def main(): checkout_java_tree(get_git_hash(src_rev), src_dir) checkout_java_tree(get_git_hash(dst_rev), dst_dir) + # Download deps. + checkout_java_acc(args.force_download) + # Run the build in each. if args.skip_build: logging.info("Skipping the build")
          Hide
          rkanter Robert Kanter added a comment -

          Another thing: If it finds any incompatibilities, it returns an exit code of 1, so the call to subprocess.check_call(args) in run_java_acc fails and you don't get any useful output in the terminal, though it still generates the html report:

          Traceback (most recent call last):
            File "./dev-support/bin/checkcompatibility.py", line 337, in <module>
              main()
            File "./dev-support/bin/checkcompatibility.py", line 333, in main
              dst_rev, dst_jars, annotations)
            File "./dev-support/bin/checkcompatibility.py", line 196, in run_java_acc
              subprocess.check_output(args)
            File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 573, in check_output
              raise CalledProcessError(retcode, cmd, output=output)
          subprocess.CalledProcessError: Command '['perl', '/Users/rkanter/dev/hadoop-common2/dev-support/target/java-acc/japi-compliance-checker-1.8/japi-compliance-checker.pl', '-l', 'hadoop', '-d1', '/Users/rkanter/dev/hadoop-common2/target/compat-check/src.xml', '-d2', '/Users/rkanter/dev/hadoop-common2/target/compat-check/dst.xml', '-report-path', '/Users/rkanter/dev/hadoop-common2/target/compat-check/report.html', '-annotations-list', '/Users/rkanter/dev/hadoop-common2/target/compat-check/annotations.txt']' returned non-zero exit status 1
          

          The error codes are listed at http://lvc.github.io/japi-compliance-checker/#Error
          It sounds like we should have the python code allow an exit code of either 0 or 1.

          Show
          rkanter Robert Kanter added a comment - Another thing: If it finds any incompatibilities, it returns an exit code of 1, so the call to subprocess.check_call(args) in run_java_acc fails and you don't get any useful output in the terminal, though it still generates the html report: Traceback (most recent call last): File "./dev-support/bin/checkcompatibility.py", line 337, in <module> main() File "./dev-support/bin/checkcompatibility.py", line 333, in main dst_rev, dst_jars, annotations) File "./dev-support/bin/checkcompatibility.py", line 196, in run_java_acc subprocess.check_output(args) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 573, in check_output raise CalledProcessError(retcode, cmd, output=output) subprocess.CalledProcessError: Command '['perl', '/Users/rkanter/dev/hadoop-common2/dev-support/target/java-acc/japi-compliance-checker-1.8/japi-compliance-checker.pl', '-l', 'hadoop', '-d1', '/Users/rkanter/dev/hadoop-common2/target/compat-check/src.xml', '-d2', '/Users/rkanter/dev/hadoop-common2/target/compat-check/dst.xml', '-report-path', '/Users/rkanter/dev/hadoop-common2/target/compat-check/report.html', '-annotations-list', '/Users/rkanter/dev/hadoop-common2/target/compat-check/annotations.txt']' returned non-zero exit status 1 The error codes are listed at http://lvc.github.io/japi-compliance-checker/#Error It sounds like we should have the python code allow an exit code of either 0 or 1.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for the review Robert, sloppy on my part to not test the patch before porting it from Yetus. This rev should address most, as well as Sean's nit.

          On the exit code, I think we should still return non-zero when there are incompatibilities. I left this as is, but if you want I can squash the exception stack trace into just returning the exit code from JACC.

          Show
          andrew.wang Andrew Wang added a comment - Thanks for the review Robert, sloppy on my part to not test the patch before porting it from Yetus. This rev should address most, as well as Sean's nit. On the exit code, I think we should still return non-zero when there are incompatibilities. I left this as is, but if you want I can squash the exception stack trace into just returning the exit code from JACC.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -0 pylint 0m 2s The patch generated 20 new + 0 unchanged - 0 fixed = 20 total (was 0)
          -1 whitespace 0m 0s The patch 2 line(s) with tabs.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          0m 47s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13583
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831912/HADOOP-13583.002.patch
          Optional Tests asflicense pylint
          uname Linux afac90c455a7 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 272a217
          pylint v1.6.4
          pylint https://builds.apache.org/job/PreCommit-HADOOP-Build/10686/artifact/patchprocess/diff-patch-pylint.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/10686/artifact/patchprocess/whitespace-tabs.txt
          modules C: . U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10686/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -0 pylint 0m 2s The patch generated 20 new + 0 unchanged - 0 fixed = 20 total (was 0) -1 whitespace 0m 0s The patch 2 line(s) with tabs. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 0m 47s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13583 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831912/HADOOP-13583.002.patch Optional Tests asflicense pylint uname Linux afac90c455a7 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 272a217 pylint v1.6.4 pylint https://builds.apache.org/job/PreCommit-HADOOP-Build/10686/artifact/patchprocess/diff-patch-pylint.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/10686/artifact/patchprocess/whitespace-tabs.txt modules C: . U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10686/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          rkanter Robert Kanter added a comment -

          I'm okay with the exit code thing the way you left it.

          The only remaining things:

          • JAVA_ACC_GIT_URL is still declared but not used anywhere
          • pylint warnings
          • whitespace warnings
          Show
          rkanter Robert Kanter added a comment - I'm okay with the exit code thing the way you left it. The only remaining things: JAVA_ACC_GIT_URL is still declared but not used anywhere pylint warnings whitespace warnings
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for the review Robert, changes in new patch:

          • Removed the unused variable.
          • Whitespace is spurious since it's in a comment. It's an example of output from the git command, which has tabs.
          • I ran pylint locally with the google style guide. I like the single-letter vars for short for loops, and I left main as is though it's long. Here's the pylint output:
          C:  1, 0: Missing module docstring (missing-docstring)
          C:143,27: Invalid variable name "w" (invalid-name)
          C:164,27: Invalid variable name "f" (invalid-name)
          C:195,40: Invalid variable name "f" (invalid-name)
          C:209, 8: Invalid variable name "f" (invalid-name)
          C:222, 8: Invalid variable name "f" (invalid-name)
          C:286, 8: Invalid variable name "f" (invalid-name)
          C:294, 8: Invalid variable name "f" (invalid-name)
          C:302, 8: Invalid variable name "a" (invalid-name)
          R:234, 0: Too many branches (13/12) (too-many-branches)
          R:234, 0: Too many statements (55/50) (too-many-statements)
          ....
          Your code has been rated at 9.39/10 (previous run: 9.18/10, +0.22)
          

          I can fix these too if you want, but I think it's pretty readable in its current state.

          Show
          andrew.wang Andrew Wang added a comment - Thanks for the review Robert, changes in new patch: Removed the unused variable. Whitespace is spurious since it's in a comment. It's an example of output from the git command, which has tabs. I ran pylint locally with the google style guide. I like the single-letter vars for short for loops, and I left main as is though it's long. Here's the pylint output: C: 1, 0: Missing module docstring (missing-docstring) C:143,27: Invalid variable name "w" (invalid-name) C:164,27: Invalid variable name "f" (invalid-name) C:195,40: Invalid variable name "f" (invalid-name) C:209, 8: Invalid variable name "f" (invalid-name) C:222, 8: Invalid variable name "f" (invalid-name) C:286, 8: Invalid variable name "f" (invalid-name) C:294, 8: Invalid variable name "f" (invalid-name) C:302, 8: Invalid variable name "a" (invalid-name) R:234, 0: Too many branches (13/12) (too-many-branches) R:234, 0: Too many statements (55/50) (too-many-statements) .... Your code has been rated at 9.39/10 (previous run: 9.18/10, +0.22) I can fix these too if you want, but I think it's pretty readable in its current state.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -0 pylint 0m 4s The patch generated 11 new + 0 unchanged - 0 fixed = 11 total (was 0)
          -1 whitespace 0m 0s The patch 2 line(s) with tabs.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          1m 4s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13583
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12833828/HADOOP-13583.003.patch
          Optional Tests asflicense pylint
          uname Linux e53bf028a252 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f5d9235
          pylint v1.6.4
          pylint https://builds.apache.org/job/PreCommit-HADOOP-Build/10817/artifact/patchprocess/diff-patch-pylint.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/10817/artifact/patchprocess/whitespace-tabs.txt
          modules C: . U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10817/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -0 pylint 0m 4s The patch generated 11 new + 0 unchanged - 0 fixed = 11 total (was 0) -1 whitespace 0m 0s The patch 2 line(s) with tabs. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 1m 4s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13583 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12833828/HADOOP-13583.003.patch Optional Tests asflicense pylint uname Linux e53bf028a252 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f5d9235 pylint v1.6.4 pylint https://builds.apache.org/job/PreCommit-HADOOP-Build/10817/artifact/patchprocess/diff-patch-pylint.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/10817/artifact/patchprocess/whitespace-tabs.txt modules C: . U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10817/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Robert Kanter, mind doing another round of review?

          Show
          andrew.wang Andrew Wang added a comment - Hi Robert Kanter , mind doing another round of review?
          Hide
          rkanter Robert Kanter added a comment -

          +1 after these two trivial things:

          • 2 lines with tabs reported by Jenkins
          • checkcompatibility.py should have executable permissions like the other programs in the bin dir, and so you can actually run it out of the box
          Show
          rkanter Robert Kanter added a comment - +1 after these two trivial things: 2 lines with tabs reported by Jenkins checkcompatibility.py should have executable permissions like the other programs in the bin dir, and so you can actually run it out of the box
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for reviewing Robert. The two tabs are example output in a comment for some parsing code, so I'd prefer not to change it. I can chmod +x the file before commit.

          Robert, this good with you?

          Show
          andrew.wang Andrew Wang added a comment - Thanks for reviewing Robert. The two tabs are example output in a comment for some parsing code, so I'd prefer not to change it. I can chmod +x the file before commit. Robert, this good with you?
          Hide
          rkanter Robert Kanter added a comment -

          Sounds good to me.

          Show
          rkanter Robert Kanter added a comment - Sounds good to me.
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for the reviews Robert! Committed to trunk, branch-2, branch-2.8.

          Show
          andrew.wang Andrew Wang added a comment - Thanks for the reviews Robert! Committed to trunk, branch-2, branch-2.8.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10749 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10749/)
          HADOOP-13583. Incorporate checkcompatibility script which runs Java API (wang: rev 0cd22d66691fdbc3fb3e0a35ad1625236c3ae3f7)

          • (edit) BUILDING.txt
          • (add) dev-support/bin/checkcompatibility.py
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10749 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10749/ ) HADOOP-13583 . Incorporate checkcompatibility script which runs Java API (wang: rev 0cd22d66691fdbc3fb3e0a35ad1625236c3ae3f7) (edit) BUILDING.txt (add) dev-support/bin/checkcompatibility.py

            People

            • Assignee:
              andrew.wang Andrew Wang
              Reporter:
              andrew.wang Andrew Wang
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development