ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-420

build/test should not require install in zkpython

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.2.2, 3.3.0
    • Component/s: contrib-bindings
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Currently you cannot just build and test the zkpython contrib, you need to actually install the zookeeper client c library as well
      as the zkpython lib itself.

      There really needs to be 2 steps:

      1) build/test zkpython "encapsulated" within the src repository, there should be no requirement to actually install anything
      (this is esp the case for automated processes and for review by PMC during release time for example)
      2) build an egg that can be distributed/installed by end user

      1. ZOOKEEPER-420.patch
        1 kB
        Henry Robinson
      2. ZOOKEEPER-420.patch
        2 kB
        Henry Robinson
      3. ZOOKEEPER-420.patch
        3 kB
        Henry Robinson
      4. ZOOKEEPER-420.patch
        3 kB
        Mahadev konar
      5. ZOOKEEPER-420.patch
        3 kB
        Patrick Hunt
      6. build.jpg
        490 kB
        Henry Robinson

        Activity

        Hide
        Henry Robinson added a comment -

        Here's a strawman patch (not yet ready for submission) which illustrates a point.

        In order to encapsulate zkpython completely, the C libraries need to be built somewhere in the encapsulated tree. From looking at the ant dependency graph (I attached a copy of the graph for interest ) and experimenting it seems that test-cppunit is the only target which makes sure that they are built, and it places them in build/test/test-cppunit.

        In this patch, Python will try and link the zookeeper module against those libraries first, before looking for /usr/local/lib. However this then introduces a dependency between zkpython and the C tests (which I can't build on my Mac, due to an unsupported ld option), which I'd rather avoid. If there were a separate ant target for building the C libraries (seems like there should be - maybe I've missed it?) then I can make zkpython test target depend on that.

        I don't know the C infrastructure well enough to hack on this without confirmation that this is the right thing to do, but will happily separate out the targets if it is.

        It seems like this is probably related to ZOOKEEPER-316 as well?

        Show
        Henry Robinson added a comment - Here's a strawman patch (not yet ready for submission) which illustrates a point. In order to encapsulate zkpython completely, the C libraries need to be built somewhere in the encapsulated tree. From looking at the ant dependency graph (I attached a copy of the graph for interest ) and experimenting it seems that test-cppunit is the only target which makes sure that they are built, and it places them in build/test/test-cppunit. In this patch, Python will try and link the zookeeper module against those libraries first, before looking for /usr/local/lib. However this then introduces a dependency between zkpython and the C tests (which I can't build on my Mac, due to an unsupported ld option), which I'd rather avoid. If there were a separate ant target for building the C libraries (seems like there should be - maybe I've missed it?) then I can make zkpython test target depend on that. I don't know the C infrastructure well enough to hack on this without confirmation that this is the right thing to do, but will happily separate out the targets if it is. It seems like this is probably related to ZOOKEEPER-316 as well?
        Hide
        Patrick Hunt added a comment -

        It would be great if we could address this in 3.3.0

        Show
        Patrick Hunt added a comment - It would be great if we could address this in 3.3.0
        Hide
        Henry Robinson added a comment -

        Here's a patch which fixes some of the issues with testing.

        1. compiling the extension now looks for C libraries in src/c/.libs before /usr/local/lib, so should be able to link without having installed the C libraries (but they must still have been built, and I still don't know if there's an ant target I should depend on for that)
        2. Running the tests now adds build/contrib/zkpython/[libdir] to PYTHONPATH, so zookeeper.so will get picked up there if it exists.
        3. Similarly, src/c/.libs gets added to both LD_LIBRARY_PATH and DYLD_LIBRARY_PATH (mac os equivalent) when the tests are run, so 'import zookeeper' should succeed.

        I'm looking into how to bundle a C library with a Python egg so that zkpython is distributable standalone. This is a separate issue, though.

        Show
        Henry Robinson added a comment - Here's a patch which fixes some of the issues with testing. 1. compiling the extension now looks for C libraries in src/c/.libs before /usr/local/lib, so should be able to link without having installed the C libraries (but they must still have been built, and I still don't know if there's an ant target I should depend on for that) 2. Running the tests now adds build/contrib/zkpython/ [libdir] to PYTHONPATH, so zookeeper.so will get picked up there if it exists. 3. Similarly, src/c/.libs gets added to both LD_LIBRARY_PATH and DYLD_LIBRARY_PATH (mac os equivalent) when the tests are run, so 'import zookeeper' should succeed. I'm looking into how to bundle a C library with a Python egg so that zkpython is distributable standalone. This is a separate issue, though.
        Hide
        Patrick Hunt added a comment -

        Henry, this sounds good, however you need one more path for 1) & 3)

        If the user types "ant test" from the toplevel build, all the java/c code will be compiled/tested. If the user then
        cd's into src/contrib/zkpython and types "ant test" it will fail. The reason is that the c code is compiled
        into the build directory (build/test/test-cppunit/.libs) rather than src/c/.libs when the "ant test" target is run.

        Re ensuring the lib is built - you might take a look at what's done for ensuring that the java classes are build for
        contribs like the "rest" contrib. You could do similar for zkpython but for the c lib (might be more trouble than it's worth though).

        Perhaps I missed it, but you should mention the PYTHONPATH in the readme (docs). I was trying LD_LIBRARY_PATH and
        was suprised that python interp couldn't find the zookeeper.so, eventually I just gave up and copied the file. If I would
        have know about PP it would have helped.

        Show
        Patrick Hunt added a comment - Henry, this sounds good, however you need one more path for 1) & 3) If the user types "ant test" from the toplevel build, all the java/c code will be compiled/tested. If the user then cd's into src/contrib/zkpython and types "ant test" it will fail. The reason is that the c code is compiled into the build directory (build/test/test-cppunit/.libs) rather than src/c/.libs when the "ant test" target is run. Re ensuring the lib is built - you might take a look at what's done for ensuring that the java classes are build for contribs like the "rest" contrib. You could do similar for zkpython but for the c lib (might be more trouble than it's worth though). Perhaps I missed it, but you should mention the PYTHONPATH in the readme (docs). I was trying LD_LIBRARY_PATH and was suprised that python interp couldn't find the zookeeper.so, eventually I just gave up and copied the file. If I would have know about PP it would have helped.
        Hide
        Henry Robinson added a comment -

        Added that directory (note, not tested because I still can't build the C tests on my Mac) and updated the README.

        Show
        Henry Robinson added a comment - Added that directory (note, not tested because I still can't build the C tests on my Mac) and updated the README.
        Hide
        Mahadev konar added a comment -

        henry I tried the patch but I still get the error:

        python-test:
        [exec] dirname: too few arguments
        [exec] Try `dirname --help' for more information.
        [exec] Running src/test/clientid_test.py
        [exec] Traceback (most recent call last):
        [exec] File "src/test/clientid_test.py", line 21, in ?
        [exec] import zookeeper, zktestbase
        [exec] ImportError: No module named zookeeper
        [exec] Running src/test/connection_test.py
        [exec] Traceback (most recent call last):
        [exec] File "src/test/connection_test.py", line 21, in ?
        [exec] import zookeeper, zktestbase
        [exec] ImportError: No module named zookeeper
        [exec] Running src/test/create_test.py
        [exec] Traceback (most recent call last):
        [exec] File "src/test/create_test.py", line 19, in ?
        [exec] import zookeeper, zktestbase, unittest, threading
        [exec] ImportError: No module named zookeeper
        [exec] Running src/test/delete_test.py
        [exec] Traceback (most recent call last):
        [exec] File "src/test/delete_test.py", line 19, in ?
        [exec] import zookeeper, zktestbase, unittest, threading
        [exec] ImportError: No module named zookeeper
        [exec] Running src/test/exists_test.py
        [exec] Traceback (most recent call last):
        [exec] File "src/test/exists_test.py", line 19, in ?
        [exec] import zookeeper, zktestbase, unittest, threading
        [exec] ImportError: No module named zookeeper
        [exec] Running src/test/get_set_test.py
        [exec] Traceback (most recent call last):
        [exec] File "src/test/get_set_test.py", line 19, in ?
        [exec] import zookeeper, zktestbase, unittest, threading
        [exec] ImportError: No module named zookeeper

        These are steps I run the tests in -

        1. ant test-core-cppunit
        2. cd src/contrib
        3. ant test

        and I get the above error. I can help debug if you do not have access to a linux machine. Let me know.

        Show
        Mahadev konar added a comment - henry I tried the patch but I still get the error: python-test: [exec] dirname: too few arguments [exec] Try `dirname --help' for more information. [exec] Running src/test/clientid_test.py [exec] Traceback (most recent call last): [exec] File "src/test/clientid_test.py", line 21, in ? [exec] import zookeeper, zktestbase [exec] ImportError: No module named zookeeper [exec] Running src/test/connection_test.py [exec] Traceback (most recent call last): [exec] File "src/test/connection_test.py", line 21, in ? [exec] import zookeeper, zktestbase [exec] ImportError: No module named zookeeper [exec] Running src/test/create_test.py [exec] Traceback (most recent call last): [exec] File "src/test/create_test.py", line 19, in ? [exec] import zookeeper, zktestbase, unittest, threading [exec] ImportError: No module named zookeeper [exec] Running src/test/delete_test.py [exec] Traceback (most recent call last): [exec] File "src/test/delete_test.py", line 19, in ? [exec] import zookeeper, zktestbase, unittest, threading [exec] ImportError: No module named zookeeper [exec] Running src/test/exists_test.py [exec] Traceback (most recent call last): [exec] File "src/test/exists_test.py", line 19, in ? [exec] import zookeeper, zktestbase, unittest, threading [exec] ImportError: No module named zookeeper [exec] Running src/test/get_set_test.py [exec] Traceback (most recent call last): [exec] File "src/test/get_set_test.py", line 19, in ? [exec] import zookeeper, zktestbase, unittest, threading [exec] ImportError: No module named zookeeper These are steps I run the tests in - ant test-core-cppunit cd src/contrib ant test and I get the above error. I can help debug if you do not have access to a linux machine. Let me know.
        Hide
        Mahadev konar added a comment -

        I was trying to debug this on my linux machine
        Looks like you have to run ant compile before running ant test. It would be better if ant test depends on compile target.

        While debugging I saw that PYTHONPATH was set to the library directory, that contained zookeeper.so (tried echoing PYTHONPATH and it points to the correct directory). So I am not sure why the tests are not able find the zookeeper module. Any suggestions?

        Show
        Mahadev konar added a comment - I was trying to debug this on my linux machine Looks like you have to run ant compile before running ant test. It would be better if ant test depends on compile target. While debugging I saw that PYTHONPATH was set to the library directory, that contained zookeeper.so (tried echoing PYTHONPATH and it points to the correct directory). So I am not sure why the tests are not able find the zookeeper module. Any suggestions?
        Hide
        Henry Robinson added a comment -

        Hi Mahadev -

        Thanks for helping out with this. I've added the compile dependency, and also fixed the "dirname: too few arguments" problem that you get if you try to manually run the tests without having run ant compile.

        For the last problem - could you add this line to run_tests.sh after the PYTHONPATH is set?

        python -c "import sys; print sys.path"

        I get:

        ['', '/Library/Python/2.5/site-packages/virtualenv-1.3.3-py2.5.egg', '/Library/Python/2.5/site-packages/simplejson-2.0.9-py2.5-macosx-10.5-i386.egg', '/Users/henry/src/cloudera/apache-zookeeper/build/contrib/zkpython/lib.macosx-10.5-i386-2.5', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python25.zip', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/plat-darwin', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/plat-mac', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/plat-mac/lib-scriptpackages', '/System/Library/Frameworks/Python.framework/Versions/2.5/Extras/lib/python', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/lib-tk', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/lib-dynload', '/Library/Python/2.5/site-packages', '/System/Library/Frameworks/Python.framework/Versions/2.5/Extras/lib/python/PyObjC']

        The third directory is the one that the script should have found for the .so file on my machine. Do you see the PYTHONPATH directory in the list you get?

        Show
        Henry Robinson added a comment - Hi Mahadev - Thanks for helping out with this. I've added the compile dependency, and also fixed the "dirname: too few arguments" problem that you get if you try to manually run the tests without having run ant compile. For the last problem - could you add this line to run_tests.sh after the PYTHONPATH is set? python -c "import sys; print sys.path" I get: ['', '/Library/Python/2.5/site-packages/virtualenv-1.3.3-py2.5.egg', '/Library/Python/2.5/site-packages/simplejson-2.0.9-py2.5-macosx-10.5-i386.egg', '/Users/henry/src/cloudera/apache-zookeeper/build/contrib/zkpython/lib.macosx-10.5-i386-2.5', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python25.zip', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/plat-darwin', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/plat-mac', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/plat-mac/lib-scriptpackages', '/System/Library/Frameworks/Python.framework/Versions/2.5/Extras/lib/python', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/lib-tk', '/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/lib-dynload', '/Library/Python/2.5/site-packages', '/System/Library/Frameworks/Python.framework/Versions/2.5/Extras/lib/python/PyObjC'] The third directory is the one that the script should have found for the .so file on my machine. Do you see the PYTHONPATH directory in the list you get?
        Hide
        Mahadev konar added a comment -

        Ok here is a patch that works. I was stupid enough not to see it... PYTHONPATH was being assigned out of the scope of the python script.

        also, I still think ant test should be dependent on compile.no?

        also, there is another problem, the tests wont work with ant test being called from the src/contrib directory. We might want to fix that as well..

        Show
        Mahadev konar added a comment - Ok here is a patch that works. I was stupid enough not to see it... PYTHONPATH was being assigned out of the scope of the python script. also, I still think ant test should be dependent on compile.no? also, there is another problem, the tests wont work with ant test being called from the src/contrib directory. We might want to fix that as well..
        Hide
        Patrick Hunt added a comment -

        This updated patch fixes the problem on my machine (ubuntu)

        Henry, you were just missing the "export" for PYTHONPATH (or just put it on the command line for python as I've done in this patch)

        I also added compile as a dependency for test (took me a bit to figure out the test was failing because I failed to compile before test, now that won't happen)

        Show
        Patrick Hunt added a comment - This updated patch fixes the problem on my machine (ubuntu) Henry, you were just missing the "export" for PYTHONPATH (or just put it on the command line for python as I've done in this patch) I also added compile as a dependency for test (took me a bit to figure out the test was failing because I failed to compile before test, now that won't happen)
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12420574/ZOOKEEPER-420.patch
        against trunk revision 818584.

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

        +1 tests included. The patch appears to include 5 new or modified tests.

        +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 does not increase the total number of release audit 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-h7.grid.sp2.yahoo.net/0/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/0/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/0/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/12420574/ZOOKEEPER-420.patch against trunk revision 818584. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +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 does not increase the total number of release audit 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-h7.grid.sp2.yahoo.net/0/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/0/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/0/console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Looks good, thanks Henry!

        Show
        Patrick Hunt added a comment - Looks good, thanks Henry!
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #485 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/485/)
        . build/test should not require install in zkpython

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #485 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/485/ ) . build/test should not require install in zkpython

          People

          • Assignee:
            Henry Robinson
            Reporter:
            Patrick Hunt
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development