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

Improve smart-apply-patch.sh to apply binary diffs

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha1
    • Fix Version/s: 3.0.0-alpha1
    • Component/s: None
    • Labels:
      None
    • Target Version/s:

      Description

      The Unix patch command cannot apply binary diffs as generated via git diff --binary. This means we cannot get effective test-patch.sh runs when the patch requires adding a binary file.

      We should consider using a different patch method.

      1. HADOOP-10926.004.patch
        2 kB
        Colin P. McCabe
      2. HADOOP-10926.003.patch
        2 kB
        Colin P. McCabe
      3. HADOOP-10926.002.patch
        2 kB
        Colin P. McCabe
      4. HADOOP-10926.001.patch
        3 kB
        Colin P. McCabe

        Issue Links

          Activity

          Hide
          andrew.wang Andrew Wang added a comment -

          Switching to use git apply --binary instead makes the most sense to me, since I know that almost all of us use git anyway. I looked into this a bit, and we'd need the following:

          • smart-apply-patch.sh assumes patch-specific options, e.g. -E --dry-run
          • Jenkins PreCommit job is passing in /usr/bin/patch, would need to update this to point to "git apply" instead
          • Need to make sure that git is installed on the build boxes. Needs to be a recent version too, I know older versions had some issues.

          I've asked about shell access to the build boxes to check their git versions. Shell access will also be useful for manual testing, since making the flip to git apply also requires updating the Jenkins job.

          Show
          andrew.wang Andrew Wang added a comment - Switching to use git apply --binary instead makes the most sense to me, since I know that almost all of us use git anyway. I looked into this a bit, and we'd need the following: smart-apply-patch.sh assumes patch -specific options, e.g. -E --dry-run Jenkins PreCommit job is passing in /usr/bin/patch, would need to update this to point to "git apply" instead Need to make sure that git is installed on the build boxes. Needs to be a recent version too, I know older versions had some issues. I've asked about shell access to the build boxes to check their git versions. Shell access will also be useful for manual testing, since making the flip to git apply also requires updating the Jenkins job.
          Hide
          cmccabe Colin P. McCabe added a comment -

          Here's a patch which implements this in smart-apply-patch.sh. I opted to use "git apply" only for git patches. This is because "git apply" doesn't implement all the quirks of the GNU patch format. So we get maximum compatibility by continuing to use GNU patch for any non-git files. Checking if something is generated by git is easy. I also added a bit of error dumping when patch application fails.

          Show
          cmccabe Colin P. McCabe added a comment - Here's a patch which implements this in smart-apply-patch.sh. I opted to use "git apply" only for git patches. This is because "git apply" doesn't implement all the quirks of the GNU patch format. So we get maximum compatibility by continuing to use GNU patch for any non-git files. Checking if something is generated by git is easy. I also added a bit of error dumping when patch application fails.
          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/12677399/HADOOP-10926.001.patch
          against trunk revision 5b1dfe7.

          +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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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/4962//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4962//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/12677399/HADOOP-10926.001.patch against trunk revision 5b1dfe7. +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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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/4962//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4962//console This message is automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          +1 LGTM, thanks Colin. As Colin told me offline, he tested manually with a few different patch scenarios.

          Show
          andrew.wang Andrew Wang added a comment - +1 LGTM, thanks Colin. As Colin told me offline, he tested manually with a few different patch scenarios.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #6360 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6360/)
          HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev b0e19c9d54cecef191b91431f9ca62a76a000f45)

          • dev-support/smart-apply-patch.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #6360 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6360/ ) HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev b0e19c9d54cecef191b91431f9ca62a76a000f45) dev-support/smart-apply-patch.sh hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          stevel@apache.org Steve Loughran added a comment -

          HADOOP-11240 reports that YARN patches are failing, and this is the likely cause. Reverting the patch & re-opening the JIRA

          Show
          stevel@apache.org Steve Loughran added a comment - HADOOP-11240 reports that YARN patches are failing, and this is the likely cause. Reverting the patch & re-opening the JIRA
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #6366 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6366/)
          Revert "HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe)" (stevel: rev c9bec46c92cc4df8d3247a3f235c303c8ae94655)

          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #6366 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6366/ ) Revert " HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe)" (stevel: rev c9bec46c92cc4df8d3247a3f235c303c8ae94655) hadoop-common-project/hadoop-common/CHANGES.txt dev-support/smart-apply-patch.sh
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Yarn-trunk #726 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/726/)
          HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev b0e19c9d54cecef191b91431f9ca62a76a000f45)

          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/smart-apply-patch.sh
            Revert "HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe)" (stevel: rev c9bec46c92cc4df8d3247a3f235c303c8ae94655)
          • dev-support/smart-apply-patch.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #726 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/726/ ) HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev b0e19c9d54cecef191b91431f9ca62a76a000f45) hadoop-common-project/hadoop-common/CHANGES.txt dev-support/smart-apply-patch.sh Revert " HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe)" (stevel: rev c9bec46c92cc4df8d3247a3f235c303c8ae94655) dev-support/smart-apply-patch.sh hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          stevel@apache.org Steve Loughran added a comment -

          having resolved this patch, jenkins patches are applying again.

          I know how near-impossible it is to test the jenkins integration except by committing the patch —perhaps we ought to have some process in place for managing those commits

          I propose:

          1. do at a weekend when less people are submitting patches
          2. after applying the patch, do some test patches to verify they take. Ideally "regression test" patches, here text patches, and "feature test" patches that test the applicability of the new feature. If these patches were added to JIRAs and submitted before committing the change, we'd expect the regression patch to continue to apply, and the feature patch to go from failing to working
          Show
          stevel@apache.org Steve Loughran added a comment - having resolved this patch, jenkins patches are applying again. I know how near-impossible it is to test the jenkins integration except by committing the patch —perhaps we ought to have some process in place for managing those commits I propose: do at a weekend when less people are submitting patches after applying the patch, do some test patches to verify they take. Ideally "regression test" patches, here text patches, and "feature test" patches that test the applicability of the new feature. If these patches were added to JIRAs and submitted before committing the change, we'd expect the regression patch to continue to apply, and the feature patch to go from failing to working
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1940 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1940/)
          HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev b0e19c9d54cecef191b91431f9ca62a76a000f45)

          • dev-support/smart-apply-patch.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
            Revert "HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe)" (stevel: rev c9bec46c92cc4df8d3247a3f235c303c8ae94655)
          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1940 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1940/ ) HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev b0e19c9d54cecef191b91431f9ca62a76a000f45) dev-support/smart-apply-patch.sh hadoop-common-project/hadoop-common/CHANGES.txt Revert " HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe)" (stevel: rev c9bec46c92cc4df8d3247a3f235c303c8ae94655) hadoop-common-project/hadoop-common/CHANGES.txt dev-support/smart-apply-patch.sh
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Hdfs-trunk #1915 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1915/)
          HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev b0e19c9d54cecef191b91431f9ca62a76a000f45)

          • dev-support/smart-apply-patch.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
            Revert "HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe)" (stevel: rev c9bec46c92cc4df8d3247a3f235c303c8ae94655)
          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk #1915 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1915/ ) HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev b0e19c9d54cecef191b91431f9ca62a76a000f45) dev-support/smart-apply-patch.sh hadoop-common-project/hadoop-common/CHANGES.txt Revert " HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe)" (stevel: rev c9bec46c92cc4df8d3247a3f235c303c8ae94655) hadoop-common-project/hadoop-common/CHANGES.txt dev-support/smart-apply-patch.sh
          Hide
          xyao Xiaoyu Yao added a comment -

          We are still seeing the Jenkins error related to test-patch.sh.

          https://builds.apache.org/job/PreCommit-HDFS-Build/8570//console

          Compiling /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build@2
          /home/jenkins/tools/maven/latest/bin/mvn clean test -DskipTests -DHadoopPatchProcess -Ptest-patch > /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build@2/../patchprocess/trunkJavacWarnings.txt 2>&1
          /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build@2/dev-support/test-patch.sh: line 270: 3108 Killed $MVN clean test -DskipTests -D$

          {PROJECT_NAME}

          PatchProcess -Ptest-patch > $PATCH_DIR/trunkJavacWarnings.txt 2>&1
          Trunk compilation is broken?
          .

          Show
          xyao Xiaoyu Yao added a comment - We are still seeing the Jenkins error related to test-patch.sh. https://builds.apache.org/job/PreCommit-HDFS-Build/8570//console Compiling /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build@2 /home/jenkins/tools/maven/latest/bin/mvn clean test -DskipTests -DHadoopPatchProcess -Ptest-patch > /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build@2/../patchprocess/trunkJavacWarnings.txt 2>&1 /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build@2/dev-support/test-patch.sh: line 270: 3108 Killed $MVN clean test -DskipTests -D$ {PROJECT_NAME} PatchProcess -Ptest-patch > $PATCH_DIR/trunkJavacWarnings.txt 2>&1 Trunk compilation is broken? .
          Hide
          cmccabe Colin P. McCabe added a comment -

          Sorry for the inconvenience, Steve Loughran. I tested with a bunch of patches, but as you said, it's hard to test every case. It looks like the issue here is related to differences in what "git apply" produces on stdout as opposed to what GNU patch produces.

          Thinking about this a little bit more, I think maybe the way to go here is to have a special case for patches that are clearly git patches generated without --no-prefix. These patches are always generated with reference to the project root, not to some other directory. Even if "git diff" is run when inside a subdirectory, we know that the a/ and b/ actually stand for the project root. This would allow us to skip a bunch of the crazy "let's figure out where the patch root is" logic-- just for the special case of git patches generated with a prefix.

          Xiaoyu wrote: We are still seeing the Jenkins error related to test-patch.sh. https://builds.apache.org/job/PreCommit-HDFS-Build/8570//console

          The build error you linked to doesn't seem related to this change. The error message is:

          /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build@2/dev-support/test-patch.sh: line 270:  3108 Killed                  $MVN clean test -DskipTests -D${PROJECT_NAME}PatchProcess -Ptest-patch > $PATCH_DIR/trunkJavacWarnings.txt 2>&1
          Trunk compilation is broken?
          

          The "Killed" means that test-patch.sh received a SIGKILL, possibly as a result of an OOM condition on the executor.

          Also, the build you linked to is at commit 58c0bb9ed9f4a2491395b63c68046562a73526c9, which is after the change has been reverted, so there is no code from this change actually being executed at this time.

          Show
          cmccabe Colin P. McCabe added a comment - Sorry for the inconvenience, Steve Loughran . I tested with a bunch of patches, but as you said, it's hard to test every case. It looks like the issue here is related to differences in what "git apply" produces on stdout as opposed to what GNU patch produces. Thinking about this a little bit more, I think maybe the way to go here is to have a special case for patches that are clearly git patches generated without --no-prefix . These patches are always generated with reference to the project root, not to some other directory. Even if "git diff" is run when inside a subdirectory, we know that the a/ and b/ actually stand for the project root. This would allow us to skip a bunch of the crazy "let's figure out where the patch root is" logic-- just for the special case of git patches generated with a prefix. Xiaoyu wrote: We are still seeing the Jenkins error related to test-patch.sh. https://builds.apache.org/job/PreCommit-HDFS-Build/8570//console The build error you linked to doesn't seem related to this change. The error message is: /home/jenkins/jenkins-slave/workspace/PreCommit-HDFS-Build@2/dev-support/test-patch.sh: line 270: 3108 Killed $MVN clean test -DskipTests -D${PROJECT_NAME}PatchProcess -Ptest-patch > $PATCH_DIR/trunkJavacWarnings.txt 2>&1 Trunk compilation is broken? The "Killed" means that test-patch.sh received a SIGKILL, possibly as a result of an OOM condition on the executor. Also, the build you linked to is at commit 58c0bb9ed9f4a2491395b63c68046562a73526c9, which is after the change has been reverted, so there is no code from this change actually being executed at this time.
          Hide
          cmccabe Colin P. McCabe added a comment -

          This patch adds a special case for git patches generated without --no-prefix as discussed above. This is a lot easier to do, since we don't need to search for the patch root when applying such patches. This avoids the need to parse the output on stdout of "git apply" and simplifies the application process for such patches a lot. I use bash regexes in order to verify that a patch is a git patch.

          Show
          cmccabe Colin P. McCabe added a comment - This patch adds a special case for git patches generated without --no-prefix as discussed above. This is a lot easier to do, since we don't need to search for the patch root when applying such patches. This avoids the need to parse the output on stdout of "git apply" and simplifies the application process for such patches a lot. I use bash regexes in order to verify that a patch is a git patch.
          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/12677660/HADOOP-10926.002.patch
          against trunk revision 3c5f5af.

          +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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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/4978//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4978//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/12677660/HADOOP-10926.002.patch against trunk revision 3c5f5af. +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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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/4978//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4978//console This message is automatically generated.
          Hide
          andrew.wang Andrew Wang added a comment -

          +1 again, let's give it another whirl.

          Show
          andrew.wang Andrew Wang added a comment - +1 again, let's give it another whirl.
          Hide
          cmccabe Colin P. McCabe added a comment -

          Thanks, Andrew. I'm going to wait until later tonight to commit this. Hopefully I can commit at a time when not many people are submitting patches. I will also run some sample builds at that time (with and without prefix, with and without git), to verify that everything is working as expected.

          Show
          cmccabe Colin P. McCabe added a comment - Thanks, Andrew. I'm going to wait until later tonight to commit this. Hopefully I can commit at a time when not many people are submitting patches. I will also run some sample builds at that time (with and without prefix, with and without git), to verify that everything is working as expected.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #6400 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6400/)
          HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 348bfb7d592cb8ca31a47b90761d8a4389d9f6ba)

          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #6400 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6400/ ) HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 348bfb7d592cb8ca31a47b90761d8a4389d9f6ba) dev-support/smart-apply-patch.sh
          Hide
          cmccabe Colin P. McCabe added a comment -

          Seems to be behaving so far. Will do a few more tests.

          Show
          cmccabe Colin P. McCabe added a comment - Seems to be behaving so far. Will do a few more tests.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #6402 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6402/)
          HADOOP-10926: Improve test-patch.sh to apply binary diffs: fix typo (cmccabe: rev e1f7d654e561987fb9a89fe72bddd80bfe04e125)

          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #6402 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6402/ ) HADOOP-10926 : Improve test-patch.sh to apply binary diffs: fix typo (cmccabe: rev e1f7d654e561987fb9a89fe72bddd80bfe04e125) dev-support/smart-apply-patch.sh
          Hide
          cmccabe Colin P. McCabe added a comment -

          So I found one last small issue with this. In the DRY_RUN case, it should not be printing out "Going to apply git patch with: git apply <args>". It doesn't print that out for the GNU patch case. Also there was another small issue with the dry_run case (git doesn't have a dry-run argument; instead, if you don't specify --apply (but do specify --stat) you get dry-run.

          I reverted this for now and will post another version. On the plus side, I don't think anyone was inconvenienced this time around, except me of course. This is just based on me looking at the jenkins job progressing.

          Show
          cmccabe Colin P. McCabe added a comment - So I found one last small issue with this. In the DRY_RUN case, it should not be printing out "Going to apply git patch with: git apply <args>". It doesn't print that out for the GNU patch case. Also there was another small issue with the dry_run case (git doesn't have a dry-run argument; instead, if you don't specify --apply (but do specify --stat) you get dry-run. I reverted this for now and will post another version. On the plus side, I don't think anyone was inconvenienced this time around, except me of course. This is just based on me looking at the jenkins job progressing.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #6403 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6403/)
          Revert "HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 4727064dab7eb2b126c9e24e9b55eea714304b15)

          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #6403 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6403/ ) Revert " HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 4727064dab7eb2b126c9e24e9b55eea714304b15) dev-support/smart-apply-patch.sh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #729 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/729/)
          HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 348bfb7d592cb8ca31a47b90761d8a4389d9f6ba)

          • dev-support/smart-apply-patch.sh
            HADOOP-10926: Improve test-patch.sh to apply binary diffs: fix typo (cmccabe: rev e1f7d654e561987fb9a89fe72bddd80bfe04e125)
          • dev-support/smart-apply-patch.sh
            Revert "HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 4727064dab7eb2b126c9e24e9b55eea714304b15)
          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #729 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/729/ ) HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 348bfb7d592cb8ca31a47b90761d8a4389d9f6ba) dev-support/smart-apply-patch.sh HADOOP-10926 : Improve test-patch.sh to apply binary diffs: fix typo (cmccabe: rev e1f7d654e561987fb9a89fe72bddd80bfe04e125) dev-support/smart-apply-patch.sh Revert " HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 4727064dab7eb2b126c9e24e9b55eea714304b15) dev-support/smart-apply-patch.sh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #1918 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1918/)
          HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 348bfb7d592cb8ca31a47b90761d8a4389d9f6ba)

          • dev-support/smart-apply-patch.sh
            HADOOP-10926: Improve test-patch.sh to apply binary diffs: fix typo (cmccabe: rev e1f7d654e561987fb9a89fe72bddd80bfe04e125)
          • dev-support/smart-apply-patch.sh
            Revert "HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 4727064dab7eb2b126c9e24e9b55eea714304b15)
          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1918 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1918/ ) HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 348bfb7d592cb8ca31a47b90761d8a4389d9f6ba) dev-support/smart-apply-patch.sh HADOOP-10926 : Improve test-patch.sh to apply binary diffs: fix typo (cmccabe: rev e1f7d654e561987fb9a89fe72bddd80bfe04e125) dev-support/smart-apply-patch.sh Revert " HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 4727064dab7eb2b126c9e24e9b55eea714304b15) dev-support/smart-apply-patch.sh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1943 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1943/)
          HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 348bfb7d592cb8ca31a47b90761d8a4389d9f6ba)

          • dev-support/smart-apply-patch.sh
            HADOOP-10926: Improve test-patch.sh to apply binary diffs: fix typo (cmccabe: rev e1f7d654e561987fb9a89fe72bddd80bfe04e125)
          • dev-support/smart-apply-patch.sh
            Revert "HADOOP-10926. Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 4727064dab7eb2b126c9e24e9b55eea714304b15)
          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1943 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1943/ ) HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 348bfb7d592cb8ca31a47b90761d8a4389d9f6ba) dev-support/smart-apply-patch.sh HADOOP-10926 : Improve test-patch.sh to apply binary diffs: fix typo (cmccabe: rev e1f7d654e561987fb9a89fe72bddd80bfe04e125) dev-support/smart-apply-patch.sh Revert " HADOOP-10926 . Improve test-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 4727064dab7eb2b126c9e24e9b55eea714304b15) dev-support/smart-apply-patch.sh
          Hide
          ajisakaa Akira Ajisaka added a comment -

          Reopens this issue.

          On the plus side, I don't think anyone was inconvenienced this time around, except me of course.

          I'm inconvenienced in HADOOP-7984. Now Jenkins doesn't support the patch for the files that have CR+LF (e.g. *.cmd files).

          Show
          ajisakaa Akira Ajisaka added a comment - Reopens this issue. On the plus side, I don't think anyone was inconvenienced this time around, except me of course. I'm inconvenienced in HADOOP-7984 . Now Jenkins doesn't support the patch for the files that have CR+LF (e.g. *.cmd files).
          Hide
          cmccabe Colin P. McCabe added a comment -

          Hi Akira Ajisaka,

          This patch was reverted, so it actually is not in effect right now. So this patch is not the source of your CR+LF problems.

          It does look like GNU patch sometimes transforms CR+LF to LF, unless the --binary flag is passed. From the documentation:

          cmccabe@keter:~/hadoop1> patch --version
          GNU patch 2.7.1
          
          cmccabe@keter:~/hadoop1> man patch
          ...
                 --binary
                    Write  all  files  in binary mode, except for standard output and /dev/tty.  When reading, disable the heuristic for transforming CRLF line endings into LF line endings.  This option is needed on POSIX systems when applying patches
                    generated on non-POSIX systems to non-POSIX files.  (On POSIX systems, file reads and writes never transform line endings. On Windows, reads and writes do transform line endings by  default,  and  patches  should  be  generated  by
                    diff --binary when line endings are significant.)
          ...
          

          We have never used the --binary option for GNU patch, so we get the tranformation behavior described here. It seems like when you are working with Windows files, this CR+LF -> LF behavior is actually undesirable. So this is another reason to use git apply rather than patch. It seems like the change in this JIRA would have actually fixed your problem, not caused it.

          I am curious, though, why this wasn't an issue before. Perhaps upgrading the version of GNU patch changed the behavior?

          Show
          cmccabe Colin P. McCabe added a comment - Hi Akira Ajisaka , This patch was reverted, so it actually is not in effect right now. So this patch is not the source of your CR+LF problems. It does look like GNU patch sometimes transforms CR+LF to LF, unless the --binary flag is passed. From the documentation: cmccabe@keter:~/hadoop1> patch --version GNU patch 2.7.1 cmccabe@keter:~/hadoop1> man patch ... --binary Write all files in binary mode, except for standard output and /dev/tty. When reading, disable the heuristic for transforming CRLF line endings into LF line endings. This option is needed on POSIX systems when applying patches generated on non-POSIX systems to non-POSIX files. (On POSIX systems, file reads and writes never transform line endings. On Windows, reads and writes do transform line endings by default , and patches should be generated by diff --binary when line endings are significant.) ... We have never used the --binary option for GNU patch, so we get the tranformation behavior described here. It seems like when you are working with Windows files, this CR+LF -> LF behavior is actually undesirable. So this is another reason to use git apply rather than patch . It seems like the change in this JIRA would have actually fixed your problem, not caused it. I am curious, though, why this wasn't an issue before. Perhaps upgrading the version of GNU patch changed the behavior?
          Hide
          cnauroth Chris Nauroth added a comment -

          I am curious, though, why this wasn't an issue before. Perhaps upgrading the version of GNU patch changed the behavior?

          By my best guess, the CRLF problems started happening with the migration to git. In theory, we have our .gitattributes files set up correctly to handle this. They look correct to me. Somehow though, there appears to be a behavioral difference compared to svn propset svn:eol-style.

          From what I've seen, git apply has handled all LF vs. CRLF issues correctly

          Show
          cnauroth Chris Nauroth added a comment - I am curious, though, why this wasn't an issue before. Perhaps upgrading the version of GNU patch changed the behavior? By my best guess, the CRLF problems started happening with the migration to git. In theory, we have our .gitattributes files set up correctly to handle this. They look correct to me. Somehow though, there appears to be a behavioral difference compared to svn propset svn:eol-style . From what I've seen, git apply has handled all LF vs. CRLF issues correctly
          Hide
          cmccabe Colin P. McCabe added a comment -

          Hmm, it looks like using git apply might be the best way to handle CRLF files, as well as do binary diffs.

          Here's a new version which behaves better with --dry-run.

          Show
          cmccabe Colin P. McCabe added a comment - Hmm, it looks like using git apply might be the best way to handle CRLF files, as well as do binary diffs. Here's a new version which behaves better with --dry-run .
          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/12683261/HADOOP-10926.004.patch
          against trunk revision a4df9ee.

          +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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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/5116//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5116//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/12683261/HADOOP-10926.004.patch against trunk revision a4df9ee. +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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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/5116//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5116//console This message is automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          looks OK to me, though needs someone with more bash skills to review it.

          IF this goes in, are there some test JIRAs which can be submitted to test all the various cases?

          Show
          stevel@apache.org Steve Loughran added a comment - looks OK to me, though needs someone with more bash skills to review it. IF this goes in, are there some test JIRAs which can be submitted to test all the various cases?
          Hide
          andrew.wang Andrew Wang added a comment -

          +1 from me. Colin P. McCabe I'll trust that you've got some test JIRAs queued up as Steve suggests, since we've tried this a few times now

          Show
          andrew.wang Andrew Wang added a comment - +1 from me. Colin P. McCabe I'll trust that you've got some test JIRAs queued up as Steve suggests, since we've tried this a few times now
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #6596 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6596/)
          HADOOP-10926. Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3)

          • dev-support/smart-apply-patch.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #6596 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6596/ ) HADOOP-10926 . Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3) dev-support/smart-apply-patch.sh hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          cmccabe Colin P. McCabe added a comment -

          Committed to trunk. I am running some test builds and monitoring... looks good so far.

          Show
          cmccabe Colin P. McCabe added a comment - Committed to trunk. I am running some test builds and monitoring... looks good so far.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #754 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/754/)
          HADOOP-10926. Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3)

          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #754 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/754/ ) HADOOP-10926 . Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3) hadoop-common-project/hadoop-common/CHANGES.txt dev-support/smart-apply-patch.sh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #16 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/16/)
          HADOOP-10926. Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3)

          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #16 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/16/ ) HADOOP-10926 . Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3) hadoop-common-project/hadoop-common/CHANGES.txt dev-support/smart-apply-patch.sh
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Hdfs-trunk #1944 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1944/)
          HADOOP-10926. Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3)

          • dev-support/smart-apply-patch.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk #1944 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1944/ ) HADOOP-10926 . Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3) dev-support/smart-apply-patch.sh hadoop-common-project/hadoop-common/CHANGES.txt
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Hdfs-trunk-Java8 #16 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/16/)
          HADOOP-10926. Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3)

          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk-Java8 #16 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/16/ ) HADOOP-10926 . Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3) hadoop-common-project/hadoop-common/CHANGES.txt dev-support/smart-apply-patch.sh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #16 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/16/)
          HADOOP-10926. Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3)

          • hadoop-common-project/hadoop-common/CHANGES.txt
          • dev-support/smart-apply-patch.sh
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #16 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/16/ ) HADOOP-10926 . Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3) hadoop-common-project/hadoop-common/CHANGES.txt dev-support/smart-apply-patch.sh
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1968 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1968/)
          HADOOP-10926. Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3)

          • dev-support/smart-apply-patch.sh
          • hadoop-common-project/hadoop-common/CHANGES.txt
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1968 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1968/ ) HADOOP-10926 . Improve smart-apply-patch.sh to apply binary diffs (cmccabe) (cmccabe: rev 2967c17fef0fabe9683437a1b13475e7480ec9a3) dev-support/smart-apply-patch.sh hadoop-common-project/hadoop-common/CHANGES.txt

            People

            • Assignee:
              cmccabe Colin P. McCabe
              Reporter:
              andrew.wang Andrew Wang
            • Votes:
              0 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development