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

test-patch.sh doesn't validate patches before building

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 0.23.3, 2.0.0-alpha
    • Fix Version/s: 3.0.0-alpha1
    • Component/s: build
    • Labels:

      Description

      When running test-patch.sh with an invalid patch (not formatted properly) or one that doesn't compile, the script spends a lot of time building Hadoop before checking to see if the patch is invalid. It would help devs if it checked first just in case we run test-patch.sh with a bad patch file.

      1. Hadoop-8523.patch
        0.4 kB
        Jack Dintruff
      2. Hadoop-8523.patch
        0.5 kB
        Jack Dintruff
      3. HADOOP-8523.patch
        2 kB
        Jack Dintruff
      4. HADOOP-8523.patch
        1 kB
        Jack Dintruff

        Issue Links

          Activity

          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12532885/Hadoop-8523.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 javadoc. The javadoc tool appears to have generated 13 warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in .

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1130//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1130//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12532885/Hadoop-8523.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 javadoc. The javadoc tool appears to have generated 13 warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1130//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1130//console This message is automatically generated.
          Hide
          jeagles Jonathan Eagles added a comment -

          Thanks for the patch, Jack. I like that this will save me some time as I know I have hit this case before.

          Couple of comments. There are two mode this script runs in, from Jenkins to run the commit builds and by developers for testing purposes. Jenkins mode relies on setup to download the patch file from the JIRA, so the check will need to happen after the patch is downloaded. Additionally, setup needs an unpatched build to check if trunk is stable and determine pre-patched javac warnings. Perhaps by adding a --dry-run option to smart-apply-patch this can be achieved.

          Show
          jeagles Jonathan Eagles added a comment - Thanks for the patch, Jack. I like that this will save me some time as I know I have hit this case before. Couple of comments. There are two mode this script runs in, from Jenkins to run the commit builds and by developers for testing purposes. Jenkins mode relies on setup to download the patch file from the JIRA, so the check will need to happen after the patch is downloaded. Additionally, setup needs an unpatched build to check if trunk is stable and determine pre-patched javac warnings. Perhaps by adding a --dry-run option to smart-apply-patch this can be achieved.
          Hide
          jackd Jack Dintruff added a comment -

          The smart-apply-patch.sh executes a --dry-run of patch, so I'll add a patch that just moves my check until after setup is called. Thanks for pointing that out.

          Show
          jackd Jack Dintruff added a comment - The smart-apply-patch.sh executes a --dry-run of patch, so I'll add a patch that just moves my check until after setup is called. Thanks for pointing that out.
          Hide
          jackd Jack Dintruff added a comment -

          Just moved the smart-apply-patch.sh call to run after setup as per Jon's suggestion..

          Show
          jackd Jack Dintruff added a comment - Just moved the smart-apply-patch.sh call to run after setup as per Jon's suggestion..
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12532902/Hadoop-8523.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 javadoc. The javadoc tool appears to have generated 13 warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in .

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1131//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1131//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12532902/Hadoop-8523.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 javadoc. The javadoc tool appears to have generated 13 warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1131//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1131//console This message is automatically generated.
          Hide
          jeagles Jonathan Eagles added a comment -

          This is looking better. If we can move it right after patch download and before the javac warnings build in setup, then we can see even more speedup!

          Show
          jeagles Jonathan Eagles added a comment - This is looking better. If we can move it right after patch download and before the javac warnings build in setup, then we can see even more speedup!
          Hide
          jackd Jack Dintruff added a comment -

          Here's a more elegant fix up for review that doesn't require the initial build and allows the Jenkins machines to take advantage of this fix as well.

          Show
          jackd Jack Dintruff added a comment - Here's a more elegant fix up for review that doesn't require the initial build and allows the Jenkins machines to take advantage of this fix as well.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12532919/HADOOP-8523.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 javadoc. The javadoc tool appears to have generated 13 warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in .

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1133//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1133//console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12532919/HADOOP-8523.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 javadoc. The javadoc tool appears to have generated 13 warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1133//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1133//console This message is automatically generated.
          Hide
          jeagles Jonathan Eagles added a comment -

          Great! A couple of more things from the new patch. test-patch.sh now aborts at the proper place. We will want to make sure the jira comment is posted to notify the user, similar to how applyPatch is being used. smart-apply-patch.sh looks like it will have the correct results. Also smart patch is getting a bit complicated with its option parsing. Either as part of this JIRA or a new JIRA add getopts parsing to be more robust and extensible with parsing.

          Show
          jeagles Jonathan Eagles added a comment - Great! A couple of more things from the new patch. test-patch.sh now aborts at the proper place. We will want to make sure the jira comment is posted to notify the user, similar to how applyPatch is being used. smart-apply-patch.sh looks like it will have the correct results. Also smart patch is getting a bit complicated with its option parsing. Either as part of this JIRA or a new JIRA add getopts parsing to be more robust and extensible with parsing.
          Hide
          daryn Daryn Sharp added a comment -

          I'd suggest keeping this patch minimal and creating another jira for enhanced option handling.

          Show
          daryn Daryn Sharp added a comment - I'd suggest keeping this patch minimal and creating another jira for enhanced option handling.
          Hide
          jackd Jack Dintruff added a comment -

          The patch is about as minimal as I can make it right now. I can definitely start working on a patch with better argument handling and file a separate JIRA for that though. Thanks for the advice.

          Show
          jackd Jack Dintruff added a comment - The patch is about as minimal as I can make it right now. I can definitely start working on a patch with better argument handling and file a separate JIRA for that though. Thanks for the advice.
          Hide
          revans2 Robert Joseph Evans added a comment -

          A couple of comments

          1. In smart-apply patch we are already figuring out the PLEVEL and by the time it gets to the dryrun check we know that it will already apply with either a p0 p1 or p2 otherwise it would have exited by this point. We really don't need to try and apply the patch yet again.
          2. Instead of trying to add yet another thing to setup, with a cleanupAndExit in the middle of it. I would prefer to see setup split up into three different functions. One to download the patch, one to test to see if the patch applies, and one to do the initial build.
          Show
          revans2 Robert Joseph Evans added a comment - A couple of comments In smart-apply patch we are already figuring out the PLEVEL and by the time it gets to the dryrun check we know that it will already apply with either a p0 p1 or p2 otherwise it would have exited by this point. We really don't need to try and apply the patch yet again. Instead of trying to add yet another thing to setup, with a cleanupAndExit in the middle of it. I would prefer to see setup split up into three different functions. One to download the patch, one to test to see if the patch applies, and one to do the initial build.
          Hide
          jackd Jack Dintruff added a comment -

          Here's a new patch, I've tested it myself and it definitely works. It also makes test-patch much easier to understand for future development.

          Show
          jackd Jack Dintruff added a comment - Here's a new patch, I've tested it myself and it definitely works. It also makes test-patch much easier to understand for future development.
          Hide
          jeagles Jonathan Eagles added a comment -

          +1. verified test-patch.sh does abort early on badly formatted patches and continues on normally with correctly formatted patches.

          Show
          jeagles Jonathan Eagles added a comment - +1. verified test-patch.sh does abort early on badly formatted patches and continues on normally with correctly formatted patches.
          Hide
          jeagles Jonathan Eagles added a comment -

          Thanks, Jack. I put this into trunk.

          Show
          jeagles Jonathan Eagles added a comment - Thanks, Jack. I put this into trunk.
          Hide
          hudson Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1095 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1095/)
          HADOOP-8523. test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394)

          Result = FAILURE
          jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394
          Files :

          • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1095 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1095/ ) HADOOP-8523 . test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394) Result = FAILURE jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2446 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2446/)
          HADOOP-8523. test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394)

          Result = FAILURE
          jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394
          Files :

          • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2446 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2446/ ) HADOOP-8523 . test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394) Result = FAILURE jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2496 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2496/)
          HADOOP-8523. test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394)

          Result = SUCCESS
          jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394
          Files :

          • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2496 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2496/ ) HADOOP-8523 . test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394) Result = SUCCESS jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2428 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2428/)
          HADOOP-8523. test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394)

          Result = SUCCESS
          jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394
          Files :

          • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2428 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2428/ ) HADOOP-8523 . test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394) Result = SUCCESS jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1128 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1128/)
          HADOOP-8523. test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394)

          Result = SUCCESS
          jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394
          Files :

          • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1128 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1128/ ) HADOOP-8523 . test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394) Result = SUCCESS jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2508 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2508/)
          HADOOP-8523. test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394)

          Result = SUCCESS
          jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394
          Files :

          • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2508 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2508/ ) HADOOP-8523 . test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394) Result = SUCCESS jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2441 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2441/)
          HADOOP-8523. test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394)

          Result = SUCCESS
          jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394
          Files :

          • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
          • /hadoop/common/trunk/dev-support/test-patch.sh
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2441 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2441/ ) HADOOP-8523 . test-patch.sh doesn't validate patches before building (Jack Dintruff via jeagles) (Revision 1358394) Result = SUCCESS jeagles : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1358394 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/dev-support/test-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt

            People

            • Assignee:
              jackd Jack Dintruff
              Reporter:
              jackd Jack Dintruff
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development