Hadoop Common
  1. Hadoop Common
  2. HADOOP-7598

smart-apply-patch.sh does not handle patching from a sub directory correctly.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.0, 0.24.0
    • Fix Version/s: 0.23.0
    • Component/s: build
    • Labels:
      None

      Description

      smart-apply-patch.sh does not apply valid patches from trunk, or from git like it was designed to do in some situations.

      1. HADOOP-7598-v4.patch
        4 kB
        Robert Joseph Evans
      2. HADOOP-7598-v3.patch
        2 kB
        Robert Joseph Evans
      3. HADOOP-7598-v2.patch
        1 kB
        Robert Joseph Evans
      4. HADOOP-7598-v1.patch
        1.0 kB
        Robert Joseph Evans

        Activity

        Hide
        Luke Lu added a comment -

        I don't think the patch does what's intended. $? should be 0 if the expression is a positive match.

        Show
        Luke Lu added a comment - I don't think the patch does what's intended. $? should be 0 if the expression is a positive match.
        Hide
        Robert Joseph Evans added a comment -

        You are right. I threw the patch together quickly and did not think it through completely. I will look through the logic more tomorrow.

        Show
        Robert Joseph Evans added a comment - You are right. I threw the patch together quickly and did not think it through completely. I will look through the logic more tomorrow.
        Hide
        Robert Joseph Evans added a comment -

        @Luke

        Thanks for taking the time to look at the patch and show that it was wrong. I took the time this morning and dug into it. It turns out that the combination of commands used to get the base directories also prepends a "\n" to the output. So all of the regular expressions that were checking for specific directories were failing because of the leading blank line. I am not sure if this is due to differences between the system that smart-apply-patch.sh was developed on originally or what, but when I added in a command to strip out all of the blank lines from the output then everything worked correctly.

        Show
        Robert Joseph Evans added a comment - @Luke Thanks for taking the time to look at the patch and show that it was wrong. I took the time this morning and dug into it. It turns out that the combination of commands used to get the base directories also prepends a "\n" to the output. So all of the regular expressions that were checking for specific directories were failing because of the leading blank line. I am not sure if this is due to differences between the system that smart-apply-patch.sh was developed on originally or what, but when I added in a command to strip out all of the blank lines from the output then everything worked correctly.
        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/118//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12492604/HADOOP-7598-v2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/118//console This message is automatically generated.
        Hide
        Robert Joseph Evans added a comment -

        I looked even deeper and found that the issue was caused because I deleted the end of an XML comment in my patch. So

        -->
        

        got turned into

        --->
        

        in the patch file. This is what produced the blank lines and caused the error. Sadly with regular expressions there is no simple way to get all of the files out of a patch file because there is always the possibility that someone will add in a line

        ++ /foo/bar
        

        or delete a line

        -- /foo/bar
        

        That will mess them up. I think the simplest way to get the proper P level is to run patch with several different p levels and see which one actually applies. If none of them apply then you know it will not work.

        Show
        Robert Joseph Evans added a comment - I looked even deeper and found that the issue was caused because I deleted the end of an XML comment in my patch. So --> got turned into ---> in the patch file. This is what produced the blank lines and caused the error. Sadly with regular expressions there is no simple way to get all of the files out of a patch file because there is always the possibility that someone will add in a line ++ /foo/bar or delete a line -- /foo/bar That will mess them up. I think the simplest way to get the proper P level is to run patch with several different p levels and see which one actually applies. If none of them apply then you know it will not work.
        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/119//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12492606/HADOOP-7598-v3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/119//console This message is automatically generated.
        Hide
        Robert Joseph Evans added a comment -

        The patch does not apply, because it is trying to apply it under common.

        Show
        Robert Joseph Evans added a comment - The patch does not apply, because it is trying to apply it under common.
        Hide
        Todd Lipcon added a comment -

        Trying a bunch of p-levels doesn't work, since a patch that only adds new files will apply at any p-level. Maybe something like:

        patch --dry-run < $PATH 2>&1 | grep '^patching file ' | awk '{print $3}'
        
        Show
        Todd Lipcon added a comment - Trying a bunch of p-levels doesn't work, since a patch that only adds new files will apply at any p-level. Maybe something like: patch --dry-run < $PATH 2>&1 | grep '^patching file ' | awk '{print $3}'
        Hide
        Robert Joseph Evans added a comment -

        That is true, and I think a more common use case then someone deleting – or adding ++ to a file.

        With patch dry-run we would still have to try it at several different levels because it will not output patching file if the patch did not apply correctly.

        Show
        Robert Joseph Evans added a comment - That is true, and I think a more common use case then someone deleting – or adding ++ to a file. With patch dry-run we would still have to try it at several different levels because it will not output patching file if the patch did not apply correctly.
        Hide
        Todd Lipcon added a comment -

        At least on my system, the --dry-run does output that even if it fails:

        todd@todd-w510:/tmp$ patch --dry-run < ~/HBASE-3954.patch  2>&1 | grep '^patching file'
        patching file TestHFile.java
        patching file AbstractBlockIndex.java
        patching file HFile.java
        patching file SimpleBlockIndex.java
        

        (of course none of those files exist in /tmp, where I ran the patch command)

        Show
        Todd Lipcon added a comment - At least on my system, the --dry-run does output that even if it fails: todd@todd-w510:/tmp$ patch --dry-run < ~/HBASE-3954.patch 2>&1 | grep '^patching file' patching file TestHFile.java patching file AbstractBlockIndex.java patching file HFile.java patching file SimpleBlockIndex.java (of course none of those files exist in /tmp, where I ran the patch command)
        Hide
        Robert Joseph Evans added a comment -

        I think this new patch should address all of the issues. There is the possibility that a patch that just creates new files could get applied to the wrong directory if some of the files do not really indicate where they came from, but this issue existed before, and is not made any worse.

        Show
        Robert Joseph Evans added a comment - I think this new patch should address all of the issues. There is the possibility that a patch that just creates new files could get applied to the wrong directory if some of the files do not really indicate where they came from, but this issue existed before, and is not made any worse.
        Hide
        Robert Joseph Evans added a comment -

        For me it does not

         
        patch --version
        patch 2.5.4
        Copyright 1984-1988 Larry Wall
        Copyright 1989-1999 Free Software Foundation, Inc.
        
        This program comes with NO WARRANTY, to the extent permitted by law.
        You may redistribute copies of this program
        under the terms of the GNU General Public License.
        For more information about these matters, see the file named COPYING.
        
        written by Larry Wall and Paul Eggert
        

        Outputs:

         
        --------------------------
        |diff --git a/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/TaskAttemptListenerImpl.java b/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/TaskAttemptListenerImpl.java
        |index c12c60c..b19b631 100644
        |--- a/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/TaskAttemptListenerImpl.java
        |+++ b/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/TaskAttemptListenerImpl.java
        --------------------------
        File to patch: 
        Skip this patch? [y] 
        Skipping patch.
        2 out of 2 hunks ignored
        can't find file to patch at input line 42
        Perhaps you used the wrong -p or --strip option?
        The text leading up to this was:
        

        at least for one of the patches I had issues with when trying to patch trunk

        Show
        Robert Joseph Evans added a comment - For me it does not patch --version patch 2.5.4 Copyright 1984-1988 Larry Wall Copyright 1989-1999 Free Software Foundation, Inc. This program comes with NO WARRANTY, to the extent permitted by law. You may redistribute copies of this program under the terms of the GNU General Public License. For more information about these matters, see the file named COPYING. written by Larry Wall and Paul Eggert Outputs: -------------------------- |diff --git a/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/TaskAttemptListenerImpl.java b/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/TaskAttemptListenerImpl.java |index c12c60c..b19b631 100644 |--- a/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/TaskAttemptListenerImpl.java |+++ b/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/TaskAttemptListenerImpl.java -------------------------- File to patch: Skip this patch? [y] Skipping patch. 2 out of 2 hunks ignored can't find file to patch at input line 42 Perhaps you used the wrong -p or --strip option? The text leading up to this was: at least for one of the patches I had issues with when trying to patch trunk
        Hide
        Robert Joseph Evans added a comment -

        Sorry it does output patching file, but only for files that are new, and being added. Also it does not output the complete path in that case.

        Show
        Robert Joseph Evans added a comment - Sorry it does output patching file, but only for files that are new, and being added. Also it does not output the complete path in that case.
        Hide
        Hadoop QA added a comment -

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

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/123//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12492669/HADOOP-7598-v4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/123//console This message is automatically generated.
        Hide
        Robert Joseph Evans added a comment -

        The patch does not apply because it is for the root build, not common.

        Show
        Robert Joseph Evans added a comment - The patch does not apply because it is for the root build, not common.
        Hide
        Tom White added a comment -

        This looks good to me. I tried it out with a few patches at various levels and it worked. Which cases were failing for you before?

        Also, there is some similar logic in test-patch for determining which modules changed, so only those tests are run. However, I don't think we need to do that with the current nested structure, since Jenkins runs at the hadoop-common-project, hadoop-hdfs-project, or hadoop-mapreduce-project level and should run all the tests under that level. I've filed HADOOP-7612 to fix that.

        Show
        Tom White added a comment - This looks good to me. I tried it out with a few patches at various levels and it worked. Which cases were failing for you before? Also, there is some similar logic in test-patch for determining which modules changed, so only those tests are run. However, I don't think we need to do that with the current nested structure, since Jenkins runs at the hadoop-common-project, hadoop-hdfs-project, or hadoop-mapreduce-project level and should run all the tests under that level. I've filed HADOOP-7612 to fix that.
        Hide
        Robert Joseph Evans added a comment -

        Like I said in the previous comments I made the change because I deleted an XML comment

        -->

        as part of MAPREDUCE-2864 Which turned into a diff line

        --->

        which messed up the grep statements to find which files changed, so it was not detecting the proper level to apply the patch. This is rare enough that it is not a big deal, but it was so painful to work around (I made 4 separate patches trying to find out what happened, because each of them applied just fine at the root level) that I decided to make a fix for it anyways.

        Show
        Robert Joseph Evans added a comment - Like I said in the previous comments I made the change because I deleted an XML comment --> as part of MAPREDUCE-2864 Which turned into a diff line ---> which messed up the grep statements to find which files changed, so it was not detecting the proper level to apply the patch. This is rare enough that it is not a big deal, but it was so painful to work around (I made 4 separate patches trying to find out what happened, because each of them applied just fine at the root level) that I decided to make a fix for it anyways.
        Hide
        Mahadev konar added a comment -

        Tom/Bobby,
        Is this ready to checkin?

        Show
        Mahadev konar added a comment - Tom/Bobby, Is this ready to checkin?
        Hide
        Tom White added a comment -

        +1

        Show
        Tom White added a comment - +1
        Hide
        Robert Joseph Evans added a comment -

        @Mahadev

        I think it should be good to go.

        Show
        Robert Joseph Evans added a comment - @Mahadev I think it should be good to go.
        Hide
        Arun C Murthy added a comment -

        I just committed this. Thanks Robert!

        Show
        Arun C Murthy added a comment - I just committed this. Thanks Robert!
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Common-trunk-Commit #853 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/853/)
        HADOOP-7598. Fix smart-apply-patch.sh to handle patching from a sub directory correctly. Contributed by Robert Evans.

        acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1166913
        Files :

        • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #853 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/853/ ) HADOOP-7598 . Fix smart-apply-patch.sh to handle patching from a sub directory correctly. Contributed by Robert Evans. acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1166913 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #930 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/930/)
        HADOOP-7598. Fix smart-apply-patch.sh to handle patching from a sub directory correctly. Contributed by Robert Evans.

        acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1166913
        Files :

        • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #930 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/930/ ) HADOOP-7598 . Fix smart-apply-patch.sh to handle patching from a sub directory correctly. Contributed by Robert Evans. acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1166913 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #864 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/864/)
        HADOOP-7598. Fix smart-apply-patch.sh to handle patching from a sub directory correctly. Contributed by Robert Evans.

        acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1166913
        Files :

        • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #864 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/864/ ) HADOOP-7598 . Fix smart-apply-patch.sh to handle patching from a sub directory correctly. Contributed by Robert Evans. acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1166913 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #788 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/788/)
        HADOOP-7598. Fix smart-apply-patch.sh to handle patching from a sub directory correctly. Contributed by Robert Evans.

        acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1166913
        Files :

        • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #788 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/788/ ) HADOOP-7598 . Fix smart-apply-patch.sh to handle patching from a sub directory correctly. Contributed by Robert Evans. acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1166913 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #812 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/812/)
        HADOOP-7598. Fix smart-apply-patch.sh to handle patching from a sub directory correctly. Contributed by Robert Evans.

        acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1166913
        Files :

        • /hadoop/common/trunk/dev-support/smart-apply-patch.sh
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #812 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/812/ ) HADOOP-7598 . Fix smart-apply-patch.sh to handle patching from a sub directory correctly. Contributed by Robert Evans. acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1166913 Files : /hadoop/common/trunk/dev-support/smart-apply-patch.sh /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt

          People

          • Assignee:
            Robert Joseph Evans
            Reporter:
            Robert Joseph Evans
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development