ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-537

The zookeeper jar includes the java source files

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.3.0
    • Fix Version/s: 3.3.0
    • Component/s: build
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This is a problem if you use zookeeper as a dependency in maven because for whatever reason the maven compiler plugin will pick up the java files in the jar and compile them to the output directory. From there they will land in the generated jar file for whatever project happens to depend on zookeeper thus introducing duplicate classes (once in zookeeper.jar, once in the project's artifact).

      1. build.patch
        7 kB
        Thomas Dudziak
      2. ZOOKEEPER-537.patch
        8 kB
        Patrick Hunt

        Issue Links

          Activity

          Hide
          Thomas Dudziak added a comment -

          Fixing is easy, just add exludes in the Ant build:

          <target name="compile-extra" depends="compile-main" if="extra.src.dir">
          ...
          <copy todir="$

          {build.classes}">
          <fileset dir="${extra.src.dir}">
          <exclude name="*/.java"/>
          </fileset>
          </copy>
          </target>

          <target name="compile-main" depends="build-generated">
          ...
          <copy todir="${build.classes}

          ">
          <fileset dir="$

          {java.src.dir}

          ">
          <exclude name="*/.java"/>
          </fileset>
          <fileset dir="$

          {src_generated.dir}

          ">
          <exclude name="*/.java"/>
          </fileset>
          </copy>
          </target>

          Show
          Thomas Dudziak added a comment - Fixing is easy, just add exludes in the Ant build: <target name="compile-extra" depends="compile-main" if="extra.src.dir"> ... <copy todir="$ {build.classes}"> <fileset dir="${extra.src.dir}"> <exclude name="* / .java"/> </fileset> </copy> </target> <target name="compile-main" depends="build-generated"> ... <copy todir="${build.classes} "> <fileset dir="$ {java.src.dir} "> <exclude name="* / .java"/> </fileset> <fileset dir="$ {src_generated.dir} "> <exclude name="* / .java"/> </fileset> </copy> </target>
          Hide
          Patrick Hunt added a comment -

          3.2.1 is out the door so moving this for consideration in 3.3. I know Hadoop in general does this (mr/hdfs/etc...) and Mahadev feels strongly about this "feature".

          Thomas, can't Maven handle this? Perhaps by explicitly excluding the zk classes in question from the generated jar (the one in your project I mean). Seems like a bug in Maven regardless (we don't see this in other build systems), perhaps you can followup by entering a JIRA against Maven itself?

          Show
          Patrick Hunt added a comment - 3.2.1 is out the door so moving this for consideration in 3.3. I know Hadoop in general does this (mr/hdfs/etc...) and Mahadev feels strongly about this "feature". Thomas, can't Maven handle this? Perhaps by explicitly excluding the zk classes in question from the generated jar (the one in your project I mean). Seems like a bug in Maven regardless (we don't see this in other build systems), perhaps you can followup by entering a JIRA against Maven itself?
          Hide
          Thomas Dudziak added a comment -

          Every other open or closed source project I've worked with, separated these into two jars, one just for the binary class files and additional resources, and one only for the sources. In the maven world, you'd also see a third one containing the javadoc. The reasons for that are several. For instance, a lot of people still care about the size of a released artifact and a few kilobytes actually matters to them. Also, all kinds of tools are build among these assumptions. Take your usual Java IDE - you can point it at a jar containing class files and it will happily use them. Or you tell it this other jar contains source files, and it will link them. However if the jar contains both, then you might encounter problems in the IDE because the assumption was that they are actually separate jars.

          Show
          Thomas Dudziak added a comment - Every other open or closed source project I've worked with, separated these into two jars, one just for the binary class files and additional resources, and one only for the sources. In the maven world, you'd also see a third one containing the javadoc. The reasons for that are several. For instance, a lot of people still care about the size of a released artifact and a few kilobytes actually matters to them. Also, all kinds of tools are build among these assumptions. Take your usual Java IDE - you can point it at a jar containing class files and it will happily use them. Or you tell it this other jar contains source files, and it will link them. However if the jar contains both, then you might encounter problems in the IDE because the assumption was that they are actually separate jars.
          Hide
          Patrick Hunt added a comment -

          For some reason hadoop does it this way, we inherited their basic build structure. I don't see why we couldn't provide build artifacts like you suggest (although others may disagree. this is the first time we've ever heard of this issue, it is not a recent change), however my point before was addressed to the fact that currently released/published artifacts include the source. Unless you want to wait for this to be resolved and a new release shipped, it would be good to find a workable solution for what we have today. I'm not familiar enough with maven, hence my suggestion that you talk with someone who is.

          Show
          Patrick Hunt added a comment - For some reason hadoop does it this way, we inherited their basic build structure. I don't see why we couldn't provide build artifacts like you suggest (although others may disagree. this is the first time we've ever heard of this issue, it is not a recent change), however my point before was addressed to the fact that currently released/published artifacts include the source. Unless you want to wait for this to be resolved and a new release shipped, it would be good to find a workable solution for what we have today. I'm not familiar enough with maven, hence my suggestion that you talk with someone who is.
          Hide
          Mahadev konar added a comment -

          the only reason I feel strongly about this is the following-

          • we have had a lot of production installations for hadoop and zookeeper. I have been invloved in supporting both of them. One thing I have found while supporting these frameworks/services is that folks tend to sometime add patches to there specific installations. So, having source code as an artifact of the release makes it easy for folks like me (developers), wherein we can take a look at the code sometimes (when all else fails) and find out what this source code contains or what changes it might have (different from the real apache release, sometimes that information gets missing as well).

          Thomas:
          are you proposing a seperate source jar as a part of release artifact as well?

          Show
          Mahadev konar added a comment - the only reason I feel strongly about this is the following- we have had a lot of production installations for hadoop and zookeeper. I have been invloved in supporting both of them. One thing I have found while supporting these frameworks/services is that folks tend to sometime add patches to there specific installations. So, having source code as an artifact of the release makes it easy for folks like me (developers), wherein we can take a look at the code sometimes (when all else fails) and find out what this source code contains or what changes it might have (different from the real apache release, sometimes that information gets missing as well). Thomas: are you proposing a seperate source jar as a part of release artifact as well?
          Hide
          Thomas Dudziak added a comment -

          Yes, all I'd like to see is separate zookeeper.jar and zookeeper-source.jar (and zookeeper-javadoc.jar would also be nice). Having source code is definitely a good thing because it makes debugging so much easier
          In the maven world you would get that automatically by adding something like this to the build section in the POM

          <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-source-plugin</artifactId>
          <executions>
          <execution>
          <id>attach-sources</id>
          <phase>verify</phase>
          <goals>
          <goal>jar</goal>
          </goals>
          </execution>
          </executions>
          </plugin>

          (and similarly for javadoc).

          Show
          Thomas Dudziak added a comment - Yes, all I'd like to see is separate zookeeper.jar and zookeeper-source.jar (and zookeeper-javadoc.jar would also be nice). Having source code is definitely a good thing because it makes debugging so much easier In the maven world you would get that automatically by adding something like this to the build section in the POM <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-source-plugin</artifactId> <executions> <execution> <id>attach-sources</id> <phase>verify</phase> <goals> <goal>jar</goal> </goals> </execution> </executions> </plugin> (and similarly for javadoc).
          Hide
          Patrick Hunt added a comment -

          Here's an idea, Thomas tell me if this works for you (maven):

          1) do everything the same as today (legacy, not maven optimal), except
          2) add a new "maven" directory to the release that contains the 3 jars thomas suggested (signed bin/src/doc jars, pom)

          We can then deploy the contents of the maven directory to the maven repo as part of the release process.

          Thomas, we use Ivy to generate, any idea how to do what you detailed (the xml) for maven but with Ivy? How about a pointer to a project that you think "does things right" that I can look at and make sure we have similar output.

          Show
          Patrick Hunt added a comment - Here's an idea, Thomas tell me if this works for you (maven): 1) do everything the same as today (legacy, not maven optimal), except 2) add a new "maven" directory to the release that contains the 3 jars thomas suggested (signed bin/src/doc jars, pom) We can then deploy the contents of the maven directory to the maven repo as part of the release process. Thomas, we use Ivy to generate, any idea how to do what you detailed (the xml) for maven but with Ivy? How about a pointer to a project that you think "does things right" that I can look at and make sure we have similar output.
          Hide
          Hiram Chirino added a comment -

          I think the reported issue is invalid. There are many cases where maven jars contain source code. For example all GWT builds include the source in jars. There has never been a report of this problem. I would close this issue out until the reporter can demonstrate the problem with a test case.

          Show
          Hiram Chirino added a comment - I think the reported issue is invalid. There are many cases where maven jars contain source code. For example all GWT builds include the source in jars. There has never been a report of this problem. I would close this issue out until the reporter can demonstrate the problem with a test case.
          Hide
          Thomas Dudziak added a comment -

          @Patrick: I can make a stab at the build.xml later this week.

          @Hiram: Well, I'm no expert in GWT, but doesn't it have to be this way so that GWT compiles the java source code on the fly in a web application (quite similar to JSP pages) ? But this then means that you're not depending on the GWT build as a library, and you thus won't get any dependency conflicts because you happen to have another conflicting version on the classpath. And this is exactly the problem with zookeeper: Maven sees source files in the jar and compiles them, so I'll get zookeeper classes in my jar which then can conflict with some other version of zookeeper that I might have on the classpath.

          Show
          Thomas Dudziak added a comment - @Patrick: I can make a stab at the build.xml later this week. @Hiram: Well, I'm no expert in GWT, but doesn't it have to be this way so that GWT compiles the java source code on the fly in a web application (quite similar to JSP pages) ? But this then means that you're not depending on the GWT build as a library, and you thus won't get any dependency conflicts because you happen to have another conflicting version on the classpath. And this is exactly the problem with zookeeper: Maven sees source files in the jar and compiles them, so I'll get zookeeper classes in my jar which then can conflict with some other version of zookeeper that I might have on the classpath.
          Hide
          Patrick Hunt added a comment -

          Thanks Thomas! We really would like to provide official ZK in a Maven repo, unfortunately we don't
          have the experience/expertise/cycles to study all these issues. Every time I think we are close
          something else pops up. Your/Hiram's help is appreciated.

          Show
          Patrick Hunt added a comment - Thanks Thomas! We really would like to provide official ZK in a Maven repo, unfortunately we don't have the experience/expertise/cycles to study all these issues. Every time I think we are close something else pops up. Your/Hiram's help is appreciated.
          Hide
          Thomas Dudziak added a comment -

          This is a patch to the build.xml in trunk to add the generation of a separate src jar file.

          Show
          Thomas Dudziak added a comment - This is a patch to the build.xml in trunk to add the generation of a separate src jar file.
          Hide
          Hadoop QA added a comment -

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

          +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 tests are needed for this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          -1 release audit. The applied patch generated 184 release audit warnings (more than the trunk's current 182 warnings).

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/43/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/43/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/43/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/43/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/12423846/build.patch against trunk revision 831486. +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 tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 release audit. The applied patch generated 184 release audit warnings (more than the trunk's current 182 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/43/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/43/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/43/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/43/console This message is automatically generated.
          Hide
          Patrick Hunt added a comment -

          Thomas there are at least a couple of outstanding comments not addressed by this patch, specifically there
          is interest to not change the existing jar file as it is currently used for those non-maven users.. I put forth the
          idea to have a separate set of jars for bin/src/jdoc (btw, I don't see the jdoc jar in your patch, isn't that something
          you had mentioned would be useful?)

          https://issues.apache.org/jira/browse/ZOOKEEPER-537?focusedCommentId=12761052&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12761052

          https://issues.apache.org/jira/browse/ZOOKEEPER-537?focusedCommentId=12763561&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12763561

          Do you see an issue with what I'm suggesting?

          Show
          Patrick Hunt added a comment - Thomas there are at least a couple of outstanding comments not addressed by this patch, specifically there is interest to not change the existing jar file as it is currently used for those non-maven users.. I put forth the idea to have a separate set of jars for bin/src/jdoc (btw, I don't see the jdoc jar in your patch, isn't that something you had mentioned would be useful?) https://issues.apache.org/jira/browse/ZOOKEEPER-537?focusedCommentId=12761052&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12761052 https://issues.apache.org/jira/browse/ZOOKEEPER-537?focusedCommentId=12763561&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12763561 Do you see an issue with what I'm suggesting?
          Hide
          Mahadev konar added a comment -

          thomas,

          After reading hiram's comments on this jira, do you still think this is a problem and needs to get fixed?

          https://issues.apache.org/jira/browse/ZOOKEEPER-537?focusedCommentId=12767931&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12767931

          is the link to his comments.

          Show
          Mahadev konar added a comment - thomas, After reading hiram's comments on this jira, do you still think this is a problem and needs to get fixed? https://issues.apache.org/jira/browse/ZOOKEEPER-537?focusedCommentId=12767931&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12767931 is the link to his comments.
          Hide
          Thomas Dudziak added a comment -

          @Mahadev: Yes - see my reply above. His comparison to GWT does not apply as afaik you don't typically depend on the GWT builds as libraries.

          Show
          Thomas Dudziak added a comment - @Mahadev: Yes - see my reply above. His comparison to GWT does not apply as afaik you don't typically depend on the GWT builds as libraries.
          Hide
          Thomas Dudziak added a comment -

          @Patrick: I'll tweak it.

          Show
          Thomas Dudziak added a comment - @Patrick: I'll tweak it.
          Hide
          Thomas Dudziak added a comment -

          Updated patch that creates additional -bin, -src, -javadoc jars.

          Show
          Thomas Dudziak added a comment - Updated patch that creates additional -bin, -src, -javadoc jars.
          Hide
          Hadoop QA added a comment -

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

          +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 tests are needed for this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          -1 release audit. The applied patch generated 186 release audit warnings (more than the trunk's current 182 warnings).

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/48/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/48/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/48/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/48/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/12423862/build.patch against trunk revision 831486. +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 tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 release audit. The applied patch generated 186 release audit warnings (more than the trunk's current 182 warnings). +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/48/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/48/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/48/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/48/console This message is automatically generated.
          Hide
          Patrick Hunt added a comment -

          The patch looks good to me, a few minor issues:

          There was an issue I noticed with the license file being spec'd wrong (that's legacy though, so an old bug). This latest patch fixes that as well.

          I also added some detail to the readme about what to expect from the build artifacts (the jars).

          Thomas – the pom does not need to be signed? (minor nit, you should name the patches after the JIRA (makes our lives easier - committers/reviewers) and also you can just upload a new patch with the same name, don't delete the old one, jira will handle it correctly, allows us to see old versions)

          ZOOKEEPER_537.patch obsoletes build.patch.

          Thanks Thomas!

          Show
          Patrick Hunt added a comment - The patch looks good to me, a few minor issues: There was an issue I noticed with the license file being spec'd wrong (that's legacy though, so an old bug). This latest patch fixes that as well. I also added some detail to the readme about what to expect from the build artifacts (the jars). Thomas – the pom does not need to be signed? (minor nit, you should name the patches after the JIRA (makes our lives easier - committers/reviewers) and also you can just upload a new patch with the same name, don't delete the old one, jira will handle it correctly, allows us to see old versions) ZOOKEEPER_537.patch obsoletes build.patch. Thanks Thomas!
          Hide
          Patrick Hunt added a comment -

          hadoopqa issues are ok - rat is complaining about the new jars/checksums, which is not a real issue.

          Show
          Patrick Hunt added a comment - hadoopqa issues are ok - rat is complaining about the new jars/checksums, which is not a real issue.
          Hide
          Patrick Hunt added a comment -

          +1 – Awesome to see this make it in.

          Thanks Thomas for helping to hammer this out!

          Show
          Patrick Hunt added a comment - +1 – Awesome to see this make it in. Thanks Thomas for helping to hammer this out!
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #522 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/522/)

          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #522 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/522/ )

            People

            • Assignee:
              Thomas Dudziak
              Reporter:
              Thomas Dudziak
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development