Pig
  1. Pig
  2. PIG-68

Improving build.xml in many ways :)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.1.0
    • Fix Version/s: 0.1.0
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      The build file can be improve in many ways:

      • add revision number to pig.jar name (like: pig-r1234.jar)
      • put pig.jar in the dist dir
      • "clean" target leave a "depend" folder undeleted
      • use a regexp to delete files in "org\apache\pig\impl\logicalLayer\parser" folder instead of listing all files one by one that you want to delete
      • put all artifacts (classes, jar, etc...) in the dist folder so that when doing clean you just need to specify dist
      • provide a description for targets (for "ant -projecthelp" command)
      • use spaces or tabs but not both (spaces are better for patch and diff in my opinion)
      1. build.xml-PIG-68-v01.patch
        14 kB
        Benjamin Francisoud
      2. build.xml-PIG-68-v02.patch
        14 kB
        Benjamin Francisoud
      3. build.xml-PIG-68-v03.patch
        14 kB
        Benjamin Francisoud
      4. build.xml-PIG-68-v04.patch
        14 kB
        Benjamin Francisoud
      5. build.xml-PIG-68-v05.patch
        14 kB
        Benjamin Francisoud
      6. build.xml-PIG-68-v06-SG.patch
        20 kB
        Stefan Groschupf
      7. build.xml-PIG-68-v07-SG.patch
        20 kB
        Stefan Groschupf
      8. out
        17 kB
        Olga Natkovich
      9. build.xml-PIG-68-v08-SG.patch
        20 kB
        Stefan Groschupf
      10. build.xml-PIG-68-v09-SG.patch
        20 kB
        Stefan Groschupf
      11. build.xml
        13 kB
        Stefan Groschupf

        Issue Links

          Activity

          Hide
          Benjamin Francisoud added a comment -

          patch for trunk/build.xml

          Show
          Benjamin Francisoud added a comment - patch for trunk/build.xml
          Hide
          Benjamin Francisoud added a comment -

          previous patch was wrong, I forgot to add this line:
          <delete file="$

          {src.dir}

          /org/apache/pig/tools/pigscript/parser/*.java" />

          adding new patch.

          Show
          Benjamin Francisoud added a comment - previous patch was wrong, I forgot to add this line: <delete file="$ {src.dir} /org/apache/pig/tools/pigscript/parser/*.java" /> adding new patch.
          Hide
          Utkarsh Srivastava added a comment -

          Looks great.
          Wouldn't it be better to leave a symlink in $

          {basedir}

          called pig.jar so that it is easy to run. Plus existing scripts that assume that existence don't break. That would also need to be deleted on clean.

          Show
          Utkarsh Srivastava added a comment - Looks great. Wouldn't it be better to leave a symlink in $ {basedir} called pig.jar so that it is easy to run. Plus existing scripts that assume that existence don't break. That would also need to be deleted on clean.
          Hide
          Andrzej Bialecki added a comment -

          Not all supported OS-es support symlinks - Windows doesn't.

          Show
          Andrzej Bialecki added a comment - Not all supported OS-es support symlinks - Windows doesn't.
          Hide
          Benjamin Francisoud added a comment -

          When I did this patch, I didn't notice that pig.jar was use in script or documentation.

          The fact that this patch set it's name to pig-r1234.jar is convenient on a developer /release manager point of view but less when it comes to scripts and documentation like in deploying pig page

          In this patch version, I added a <copy ...> at the end of the jar target to make a copy of dist/pig-r1234.jar to pig.jar

          Show
          Benjamin Francisoud added a comment - When I did this patch, I didn't notice that pig.jar was use in script or documentation. The fact that this patch set it's name to pig-r1234.jar is convenient on a developer /release manager point of view but less when it comes to scripts and documentation like in deploying pig page In this patch version, I added a <copy ...> at the end of the jar target to make a copy of dist/pig-r1234.jar to pig.jar
          Hide
          Utkarsh Srivastava added a comment -

          +1

          Olga, or Alan, I think a review from one of you two is needed before I can commit this one.

          Show
          Utkarsh Srivastava added a comment - +1 Olga, or Alan, I think a review from one of you two is needed before I can commit this one.
          Hide
          Olga Natkovich added a comment -

          I applied the patch to my tree and ran "ant clean" and it did not remove java files generated by javacc. I see that you have a pattern in clean that should remove them but it did not work in my case. Does it work for you?

          I am using the following version of ant: Apache Ant version 1.6.5 compiled on June 2 2005.

          Show
          Olga Natkovich added a comment - I applied the patch to my tree and ran "ant clean" and it did not remove java files generated by javacc. I see that you have a pattern in clean that should remove them but it did not work in my case. Does it work for you? I am using the following version of ant: Apache Ant version 1.6.5 compiled on June 2 2005.
          Hide
          Benjamin Francisoud added a comment -

          I think the package name changed because of PIG-32 so I suppose the regexp to delete those files need to be updated.
          I will take a look at it.

          Show
          Benjamin Francisoud added a comment - I think the package name changed because of PIG-32 so I suppose the regexp to delete those files need to be updated. I will take a look at it.
          Hide
          Benjamin Francisoud added a comment -

          My mistake, I didn't use the correct syntax in the delete tag.

          In this patch version I replaced it with:

          <delete>
              <fileset dir="${src.dir}/org/apache/pig/impl/logicalLayer/parser" includes="*.java"/>
              <fileset dir="${src.dir}/org/apache/pig/tools/pigscript/parser" includes="*.java"/>
          </delete>
          
          Show
          Benjamin Francisoud added a comment - My mistake, I didn't use the correct syntax in the delete tag. In this patch version I replaced it with: <delete> <fileset dir= "${src.dir}/org/apache/pig/impl/logicalLayer/parser" includes= "*.java" /> <fileset dir= "${src.dir}/org/apache/pig/tools/pigscript/parser" includes= "*.java" /> </delete>
          Hide
          Benjamin Francisoud added a comment -

          Does NodeIdGenerator.java should be deleted also, it is commited in svn ?

          Show
          Benjamin Francisoud added a comment - Does NodeIdGenerator.java should be deleted also, it is commited in svn ?
          Hide
          Olga Natkovich added a comment -

          NodeGenerator is needed - without it the code does not compile. Also, clean build does not compile. I think you need to also remove QueryParser.jj file in thesrc/org/apache/pig/tools/pigscript/parser/ directory since it is generated.

          Please, make sure that ant clean followed by ant builds successfully with your next change and also that unit tests run, thanks.

          Show
          Olga Natkovich added a comment - NodeGenerator is needed - without it the code does not compile. Also, clean build does not compile. I think you need to also remove QueryParser.jj file in thesrc/org/apache/pig/tools/pigscript/parser/ directory since it is generated. Please, make sure that ant clean followed by ant builds successfully with your next change and also that unit tests run, thanks.
          Hide
          Benjamin Francisoud added a comment -

          Same with PigScriptParser.jj ? Should it be deleted ?

          Show
          Benjamin Francisoud added a comment - Same with PigScriptParser.jj ? Should it be deleted ?
          Hide
          Benjamin Francisoud added a comment -

          Fixed the "clean" target (but don't delete PigScriptParser.jj; can be added later)
          Fixed the test target added some test output in the console using this line:

          <formatter type="plain" usefile="no"/>
          

          Unfortunately some tests don't pass on my local cluster but I don't think it's related to ant.
          The same tests are also failing the same way with a non modify build.xml.

          Can you try the patch by running the tests.

          Thanks

          Show
          Benjamin Francisoud added a comment - Fixed the "clean" target (but don't delete PigScriptParser.jj; can be added later) Fixed the test target added some test output in the console using this line: <formatter type= "plain" usefile= "no" /> Unfortunately some tests don't pass on my local cluster but I don't think it's related to ant. The same tests are also failing the same way with a non modify build.xml. Can you try the patch by running the tests. Thanks
          Hide
          Stefan Groschupf added a comment -

          Here is my contribution to this topic.
          I basically completely rewrote the build.xml in a clean way based on the hadoop build.xml.
          The build.xml provides all functionality as before, plus
          + creating a pig and a pig-core.jar (as contributed by Iván de Prado).
          + creating a release tar ball - as hadoop
          + generate all sources into a separated folder for easy generation and removal.
          + improved configuration using build.properties
          + improved naming of jar files (version name)
          + improved java doc rendering
          + improved manifest file
          + improved formating

          My patch does not use the svn revision number as Benjamin Francisoud verison does.
          In general I like the idea very much but from my pov should a revision number only in "dev" jar name.
          I guess it is possible to attach the revision number in case the version name end on "-dev" but I had trouble to use ant conditions and string comparison. Maybe a ant pro can help with this last little improvement.

          Show
          Stefan Groschupf added a comment - Here is my contribution to this topic. I basically completely rewrote the build.xml in a clean way based on the hadoop build.xml. The build.xml provides all functionality as before, plus + creating a pig and a pig-core.jar (as contributed by Iván de Prado). + creating a release tar ball - as hadoop + generate all sources into a separated folder for easy generation and removal. + improved configuration using build.properties + improved naming of jar files (version name) + improved java doc rendering + improved manifest file + improved formating My patch does not use the svn revision number as Benjamin Francisoud verison does. In general I like the idea very much but from my pov should a revision number only in "dev" jar name. I guess it is possible to attach the revision number in case the version name end on "-dev" but I had trouble to use ant conditions and string comparison. Maybe a ant pro can help with this last little improvement.
          Hide
          Stefan Groschupf added a comment -

          ... for better readability here the new build.xml as plain file and not as diff.

          Show
          Stefan Groschupf added a comment - ... for better readability here the new build.xml as plain file and not as diff.
          Hide
          Olga Natkovich added a comment -

          I tried to integrate the latest patch into my tree but it failed.

          Show
          Olga Natkovich added a comment - I tried to integrate the latest patch into my tree but it failed.
          Hide
          Stefan Groschupf added a comment -

          regenerated against latest svn.

          Show
          Stefan Groschupf added a comment - regenerated against latest svn.
          Hide
          Olga Natkovich added a comment -

          I can't get the code to compile with the latest patch. Will attach the output

          Show
          Olga Natkovich added a comment - I can't get the code to compile with the latest patch. Will attach the output
          Hide
          Stefan Groschupf added a comment -

          I just tried the build file again. I get a set of wanring, however all classes compile.
          The 18 duplicate class error you get means you have a collision in the name space. Means somehow some classes exists twice in your classpath. My guess is you have the generated classes still in your project.
          Please make sure you do a ant clean before you patch the build file or please delete the javacc generated files manually.
          Also after patching you need to add src-gen to the classpath of your IDE in case you use one.

          Show
          Stefan Groschupf added a comment - I just tried the build file again. I get a set of wanring, however all classes compile. The 18 duplicate class error you get means you have a collision in the name space. Means somehow some classes exists twice in your classpath. My guess is you have the generated classes still in your project. Please make sure you do a ant clean before you patch the build file or please delete the javacc generated files manually. Also after patching you need to add src-gen to the classpath of your IDE in case you use one.
          Hide
          Olga Natkovich added a comment -

          When I got a brand new tree, the compile went fine but all unit tests failed. Are you able to run unit tests?

          I guess I will try with new unit tests that just got checked in and see what happens

          Show
          Olga Natkovich added a comment - When I got a brand new tree, the compile went fine but all unit tests failed. Are you able to run unit tests? I guess I will try with new unit tests that just got checked in and see what happens
          Hide
          Olga Natkovich added a comment -

          Stephan, sorry, can you make one more patch agains the latest tree. Your changes conflict with the unit test changes and I am not quite sure how to resolve them, thanks

          Show
          Olga Natkovich added a comment - Stephan, sorry, can you make one more patch agains the latest tree. Your changes conflict with the unit test changes and I am not quite sure how to resolve them, thanks
          Hide
          Xu Zhang added a comment -

          FYI, with the new unit test framework, you just need to issue "ant test" to run it on a local cluster.

          Show
          Xu Zhang added a comment - FYI, with the new unit test framework, you just need to issue "ant test" to run it on a local cluster.
          Hide
          Stefan Groschupf added a comment -

          I will fix it, give me some minutes.

          Show
          Stefan Groschupf added a comment - I will fix it, give me some minutes.
          Hide
          Stefan Groschupf added a comment -

          patch build.xml against r630017.
          There was a problem in my last patch related to the test classpath this is fixed now.
          The tests itself run now, but some are failing on my laptop, but I think it is not related to the build.xml file.

          Show
          Stefan Groschupf added a comment - patch build.xml against r630017. There was a problem in my last patch related to the test classpath this is fixed now. The tests itself run now, but some are failing on my laptop, but I think it is not related to the build.xml file.
          Hide
          Olga Natkovich added a comment -

          I tried to apply the patch to the latest code but it failed:

          patch p0 </home/olgan/src/pig-apache-trunk/pig/trunk/build.xmlPIG-68-v08-SG.patch
          patching file build.xml
          Hunk #1 FAILED at 1.
          1 out of 1 hunk FAILED – saving rejects to file build.xml.rej

          Show
          Olga Natkovich added a comment - I tried to apply the patch to the latest code but it failed: patch p0 </home/olgan/src/pig-apache-trunk/pig/trunk/build.xml PIG-68 -v08-SG.patch patching file build.xml Hunk #1 FAILED at 1. 1 out of 1 hunk FAILED – saving rejects to file build.xml.rej
          Hide
          Stefan Groschupf added a comment -

          BUILD SUCCESSFUL
          Total time: 12 minutes 35 seconds

          :-D

          Sorry, it was related to the build.xml. Basically the jarManager failed, since I did run the tests with the compiled classes directly but the jarManager is searching for a physically existing jar file.
          Fixed with patch version 9.

          Show
          Stefan Groschupf added a comment - BUILD SUCCESSFUL Total time: 12 minutes 35 seconds :-D Sorry, it was related to the build.xml. Basically the jarManager failed, since I did run the tests with the compiled classes directly but the jarManager is searching for a physically existing jar file. Fixed with patch version 9.
          Hide
          Stefan Groschupf added a comment -

          Here again the plain build.xml you might just overwrite the existing. Should be no problem with svn.

          Show
          Stefan Groschupf added a comment - Here again the plain build.xml you might just overwrite the existing. Should be no problem with svn.
          Hide
          Olga Natkovich added a comment -

          +1

          Show
          Olga Natkovich added a comment - +1
          Hide
          Olga Natkovich added a comment -

          Patch comitted. Thanks Benjamin and Stefan for contributing this patch

          Show
          Olga Natkovich added a comment - Patch comitted. Thanks Benjamin and Stefan for contributing this patch

            People

            • Assignee:
              Stefan Groschupf
              Reporter:
              Benjamin Francisoud
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development