Hadoop Common
  1. Hadoop Common
  2. HADOOP-9246

Execution phase for hadoop-maven-plugin should be process-resources

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0, trunk-win
    • Fix Version/s: 2.1.0-beta
    • Component/s: build
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Per discussion on HADOOP-9245, the execution phase of hadoop-maven-plugin should be process-resources and not compile.

      1. hadoop-9246.patch
        1 kB
        Karthik Kambatla
      2. hadoop-9246.patch
        1 kB
        Karthik Kambatla
      3. HADOOP-9246.2.patch
        2 kB
        Chris Nauroth
      4. HADOOP-9246-addendum.patch
        2 kB
        Alejandro Abdelnur

        Issue Links

          Activity

          Hide
          Karthik Kambatla added a comment -

          Trivial patch to fix the issue.

          Show
          Karthik Kambatla added a comment - Trivial patch to fix the issue.
          Hide
          Karthik Kambatla added a comment -

          Verified locally that clean and install work (in that order) on fresh m2 repo.

          Show
          Karthik Kambatla added a comment - Verified locally that clean and install work (in that order) on fresh m2 repo.
          Hide
          Karthik Kambatla added a comment -

          Odd that prepare-resources worked. Per http://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html#Setting_Up_Your_Project_to_Use_the_Build_Lifecycle , it should be process-resources.

          Uploading a new patch with process-resources.

          Nuked hadoop-maven-plugins from .m2/repository, and ran mvn clean; mvn install successfully.

          Show
          Karthik Kambatla added a comment - Odd that prepare-resources worked. Per http://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html#Setting_Up_Your_Project_to_Use_the_Build_Lifecycle , it should be process-resources. Uploading a new patch with process-resources. Nuked hadoop-maven-plugins from .m2/repository, and ran mvn clean; mvn install successfully.
          Hide
          Alejandro Abdelnur added a comment -

          +1

          Show
          Alejandro Abdelnur added a comment - +1
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12566401/hadoop-9246.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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any 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 hadoop-common-project/hadoop-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2089//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2089//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/12566401/hadoop-9246.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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any 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 hadoop-common-project/hadoop-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2089//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2089//console This message is automatically generated.
          Hide
          Chris Nauroth added a comment -

          What about the annotation of the default phase in VersionInfoMojo? Should we change that too?

          @Mojo(name="version-info", defaultPhase=LifecyclePhase.INITIALIZE)
          public class VersionInfoMojo extends AbstractMojo {
          

          I think we still need to be explicit about the <phase> in the pom.xml files to prevent the HADOOP-9245 problem, but I'm wondering if we should update the annotation for consistency, or possibly just remove defaultPhase and require users to be specific.

          Show
          Chris Nauroth added a comment - What about the annotation of the default phase in VersionInfoMojo ? Should we change that too? @Mojo(name= "version-info" , defaultPhase=LifecyclePhase.INITIALIZE) public class VersionInfoMojo extends AbstractMojo { I think we still need to be explicit about the <phase> in the pom.xml files to prevent the HADOOP-9245 problem, but I'm wondering if we should update the annotation for consistency, or possibly just remove defaultPhase and require users to be specific.
          Hide
          Alejandro Abdelnur added a comment -

          Chris, you right, we should.

          Show
          Alejandro Abdelnur added a comment - Chris, you right, we should.
          Hide
          Chris Nauroth added a comment -

          FYI, the HADOOP-9245 patch caused the built version info properties files to be incorrect. Running in the compile phase populated the version info properties too late, and then the filtering left tokens unreplaced like $

          {version-info.build.time}

          . Sorry for not catching this during my code review of HADOOP-9245.

          Ultimately, this caused a test failure in YARN's TestNMWebServices while trying to parse the date returned from the web service. I retested this with hadoop-9246.patch, which was binding to the prepare-resources phase, but it didn't fix the problem.

          I'm uploading HADOOP-9246.2.patch, which binds to the generate-resources phase, which runs before resource filtering. Once I did this, the version info properties files were populated correctly, and TestNMWebServices passed. I've also removed the defaultPhase from the mojo annotation.

          Show
          Chris Nauroth added a comment - FYI, the HADOOP-9245 patch caused the built version info properties files to be incorrect. Running in the compile phase populated the version info properties too late, and then the filtering left tokens unreplaced like $ {version-info.build.time} . Sorry for not catching this during my code review of HADOOP-9245 . Ultimately, this caused a test failure in YARN's TestNMWebServices while trying to parse the date returned from the web service. I retested this with hadoop-9246.patch, which was binding to the prepare-resources phase, but it didn't fix the problem. I'm uploading HADOOP-9246 .2.patch, which binds to the generate-resources phase, which runs before resource filtering. Once I did this, the version info properties files were populated correctly, and TestNMWebServices passed. I've also removed the defaultPhase from the mojo annotation.
          Hide
          Karthik Kambatla added a comment -

          Chris - thanks for correcting the patch. The latest patch looks good to me. +1

          Show
          Karthik Kambatla added a comment - Chris - thanks for correcting the patch. The latest patch looks good to me. +1
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12566529/HADOOP-9246.2.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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any 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 hadoop-common-project/hadoop-common hadoop-maven-plugins hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2094//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2094//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/12566529/HADOOP-9246.2.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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any 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 hadoop-common-project/hadoop-common hadoop-maven-plugins hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2094//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2094//console This message is automatically generated.
          Hide
          Chris Nauroth added a comment -

          The -1 from Jenkins for lack of tests is expected, because this is a build script change.

          Show
          Chris Nauroth added a comment - The -1 from Jenkins for lack of tests is expected, because this is a build script change.
          Hide
          Jason Lowe added a comment -

          +1

          Show
          Jason Lowe added a comment - +1
          Hide
          Jason Lowe added a comment -

          Thanks Karthik and Chris! I committed this to trunk.

          Show
          Jason Lowe added a comment - Thanks Karthik and Chris! I committed this to trunk.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk-Commit #3289 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3289/)
          HADOOP-9246. Execution phase for hadoop-maven-plugin should be process-resources. Contributed by Karthik Kambatla and Chris Nauroth (Revision 1439620)

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml
          • /hadoop/common/trunk/hadoop-maven-plugins/src/main/java/org/apache/hadoop/maven/plugin/versioninfo/VersionInfoMojo.java
          • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/pom.xml
          Show
          Hudson added a comment - Integrated in Hadoop-trunk-Commit #3289 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3289/ ) HADOOP-9246 . Execution phase for hadoop-maven-plugin should be process-resources. Contributed by Karthik Kambatla and Chris Nauroth (Revision 1439620) Result = SUCCESS jlowe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1439620 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml /hadoop/common/trunk/hadoop-maven-plugins/src/main/java/org/apache/hadoop/maven/plugin/versioninfo/VersionInfoMojo.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/pom.xml
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Yarn-trunk #111 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/111/)
          HADOOP-9246. Execution phase for hadoop-maven-plugin should be process-resources. Contributed by Karthik Kambatla and Chris Nauroth (Revision 1439620)

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml
          • /hadoop/common/trunk/hadoop-maven-plugins/src/main/java/org/apache/hadoop/maven/plugin/versioninfo/VersionInfoMojo.java
          • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/pom.xml
          Show
          Hudson added a comment - Integrated in Hadoop-Yarn-trunk #111 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/111/ ) HADOOP-9246 . Execution phase for hadoop-maven-plugin should be process-resources. Contributed by Karthik Kambatla and Chris Nauroth (Revision 1439620) Result = SUCCESS jlowe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1439620 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml /hadoop/common/trunk/hadoop-maven-plugins/src/main/java/org/apache/hadoop/maven/plugin/versioninfo/VersionInfoMojo.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/pom.xml
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1300 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1300/)
          HADOOP-9246. Execution phase for hadoop-maven-plugin should be process-resources. Contributed by Karthik Kambatla and Chris Nauroth (Revision 1439620)

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml
          • /hadoop/common/trunk/hadoop-maven-plugins/src/main/java/org/apache/hadoop/maven/plugin/versioninfo/VersionInfoMojo.java
          • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/pom.xml
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1300 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1300/ ) HADOOP-9246 . Execution phase for hadoop-maven-plugin should be process-resources. Contributed by Karthik Kambatla and Chris Nauroth (Revision 1439620) Result = SUCCESS jlowe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1439620 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml /hadoop/common/trunk/hadoop-maven-plugins/src/main/java/org/apache/hadoop/maven/plugin/versioninfo/VersionInfoMojo.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/pom.xml
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1328 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1328/)
          HADOOP-9246. Execution phase for hadoop-maven-plugin should be process-resources. Contributed by Karthik Kambatla and Chris Nauroth (Revision 1439620)

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml
          • /hadoop/common/trunk/hadoop-maven-plugins/src/main/java/org/apache/hadoop/maven/plugin/versioninfo/VersionInfoMojo.java
          • /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/pom.xml
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1328 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1328/ ) HADOOP-9246 . Execution phase for hadoop-maven-plugin should be process-resources. Contributed by Karthik Kambatla and Chris Nauroth (Revision 1439620) Result = SUCCESS jlowe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1439620 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/pom.xml /hadoop/common/trunk/hadoop-maven-plugins/src/main/java/org/apache/hadoop/maven/plugin/versioninfo/VersionInfoMojo.java /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/pom.xml
          Hide
          Alejandro Abdelnur added a comment -

          I think a more correct way of fixing this (realized that while doing the protoc plugin, HADOOP-9117) is setting the default phase in the annotation of the plugin.

          Show
          Alejandro Abdelnur added a comment - I think a more correct way of fixing this (realized that while doing the protoc plugin, HADOOP-9117 ) is setting the default phase in the annotation of the plugin.
          Hide
          Alejandro Abdelnur added a comment -

          addendum patch moving the phase binding to the plugin definition.

          Show
          Alejandro Abdelnur added a comment - addendum patch moving the phase binding to the plugin definition.
          Hide
          Chris Nauroth added a comment -

          Hi Alejandro,

          The addendum patch would re-break HADOOP-9245 (mvn clean without running mvn install before fails). Without specifying phase explicitly in the pom.xml, Maven needs to look at the mojo definition to determine default phase. Unfortunately, if you don't have hadoop-maven-plugins installed yet, then there is no mojo definition to inspect, and mvn clean fails.

          Of course, we can keep the change to add defaultPhase in the mojo code. I think we just also need to keep <phase> in the pom.xml files. +1 for just the change in VersionInfoMojo.

          Thanks!

          Show
          Chris Nauroth added a comment - Hi Alejandro, The addendum patch would re-break HADOOP-9245 (mvn clean without running mvn install before fails). Without specifying phase explicitly in the pom.xml, Maven needs to look at the mojo definition to determine default phase. Unfortunately, if you don't have hadoop-maven-plugins installed yet, then there is no mojo definition to inspect, and mvn clean fails. Of course, we can keep the change to add defaultPhase in the mojo code. I think we just also need to keep <phase> in the pom.xml files. +1 for just the change in VersionInfoMojo . Thanks!
          Hide
          Alejandro Abdelnur added a comment -

          arghh, got it, i was missing that. Closing it then.

          I guess we have to do the same for the protoc plugin, HADOOP-9117, will test there.

          Show
          Alejandro Abdelnur added a comment - arghh, got it, i was missing that. Closing it then. I guess we have to do the same for the protoc plugin, HADOOP-9117 , will test there.

            People

            • Assignee:
              Karthik Kambatla
              Reporter:
              Karthik Kambatla
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development