we can end up importing the wrong version of a dependency even though the right one is "already the active version in easy-install.pth"

[davidsarah]

(Split from ticket:1065#comment:7.)

​Log for the test step on 'Ruben Fedora':

Searching for pycrypto==2.1.0
Best match: pycrypto 2.1.0
Processing pycrypto-2.1.0-py2.7-linux-x86_64.egg
pycrypto 2.1.0 is already the active version in easy-install.pth

Using /home/buildbot/tahoe/Ruben Fedora/build/support/lib/python2.7/
site-packages/pycrypto-2.1.0-py2.7-linux-x86_64.egg
[...]
running trial
[...]
pkg_resources.VersionConflict: (pycrypto 2.2 (/usr/lib64/python2.7/
site-packages), Requirement.parse('pycrypto==2.0.1,==2.1,>=2.3'))

I believe this is due to the same underlying issue as #1137, that is, the handling of sys.path by the setuptools-installed site.py and .pth files being broken. If the path handling were working correctly, it should not have been possible that "pycrypto 2.1.0 is already the active version in easy-install.pth", but we end up importing pycrypto 2.2 when running trial.

Here's an instance of the same problem with foolscap ​on 'Shawn jaunty':

Searching for foolscap==0.5.1
Best match: foolscap 0.5.1
Processing foolscap-0.5.1-py2.6.egg
foolscap 0.5.1 is already the active version in easy-install.pth
Installing flappclient script to support/bin
Installing flogtool script to support/bin
Installing flappserver script to support/bin

Using /var/lib/buildbot/tahoe/mordsith/build/support/lib/python2.6/
site-packages/foolscap-0.5.1-py2.6.egg
[...]
running trial
[...]
pkg_resources.VersionConflict: (foolscap 0.4.2 (/usr/local/lib/python2.6/
dist-packages/foolscap-0.4.2-py2.6.egg),
Requirement.parse('foolscap[secure_connections]>=0.5.1'))

So this is not specific to pycrypto or due to the use of a disjunctive requirement. I think I have also seen this when running bin/tahoe, not just for setup.py trial.

[zooko]

I think I have a working isolated, deterministic test of this functionality now, and I think I understand how to fix it, too.

[warner]

import+exec twistd, instead of find+spawn, to remove an intermediate process

[zooko]

Committed to the ticket1190 branch as [20101027052730-92b7f-4fb13712e23ceaef5b42474f599278f142010bc9], [20101027060443-92b7f-32a95aeca42279a788bff2683568b810f8e73fb7], ac3b26ecf29c08cb where it made the intended test go from ​red to ​green but also unfortunately I think it might have made different tests on different buildslaves go from green to red. I'm sorry that I'm not certain about that latter part. I'm very sleepy! Maybe we can sort it all out tomorrow.

A convenient way to do that might be to compare ​buildbot results for branch=ticket1190 vs. ​for branch=trunk.

[zooko]

Okay in the cold light of morning I see that the current failures on the ticket1190 branch are of three kinds:

• a bug in darcsver so that the version number comes out as "unknown" or "0.0.0": ​http://tahoe-lafs.org/buildbot/builders/Arthur%20lenny%20c7%2032bit/builds/503/steps/test/logs/stdio
• a UserWarning which triggers test_no_noise to go red: ​arthur build 503
• a bug where the system-wide installation of allmydata-tahoe is getting added to the working set even though it is incompatible with the requirement of allmydata-tahoe==$THIS_PARTICULAR_VERSION: ​Kyle OpenBSD-4.6 amd64 build 426 bin/tahoe --version, ​Kyle OpenBSD-4.6 amd64 build 426 python setup.py test, ​FreeStorm Win7-amd64-mingw-py2.6 build 111 python setup.py test

[davidsarah]

[4764/ticket1190] and [4766/ticket1190] look ok, or at least worth trying.

[4765/ticket1190] adds a __requires__ line to newly generated .tac files. This is liable to cause the same kind of forward-compatibility problem we had trying to change the appname in #1159 -- i.e. when we change the requirements, existing .tac files will still reflect the requirements at the time they were generated.

(This will cause even more subtle problems than the appname change, because the effect will be to allow earlier versions of dependencies than intended, and so regress bugs we thought we'd fixed, rather than always failing when an old .tac is used.)

I'm more and more convinced that .tac files are more trouble than they're worth and that we should stop using them (see ticket:1159#comment:10). Node directories should contain only data, not code.

[warner]

I added a hefty comment to ticket:1159 . Yes, it's time for .tac files to die, and certainly we shouldn't be adding any new text to them, and double-certainly we shouldn't be touching existing ones (or relying upon having specific new code in them).

Any pkg_resources stuff should be done in tahoe start before transferring control to twistd. This is possible with the import+call patch, and is probably impossible with the old spawn-twistd approach, so we should start by landing that one.

[warner]

eeargh. Please don't use __requires__. It appears to be a completely undocumented hack on top of the unpleasant hack that setuptools already is. The only information I could find about it is ​here. Let's find a way to drain and delete the .tac file and do any sys.path munging in bin/tahoe before tahoe start tries to import anything of interest.

[david-sarah@…]

In 270322ad4762f47f:

startstop_node.py: pyflakes import fix. refs #1190

[davidsarah]

I landed the import+call patch on trunk in ac3b26ecf29c08cb, and fixed a pyflakes warning that it introduced. This seems to have (accidentally?) fixed the test failure in ​http://tahoe-lafs.org/buildbot/builders/Ruben%20Fedora/builds/639/steps/test/logs/stdio; we ​now have no warning on the Ruben Fedora builder due to reimporting twisted, and so test_runner.RunNode.test_client_no_noise passes.

IIUC, when (and if) twisted daemonizes, the child process should inherit the sys.path that was munged by the script that ran the parent process. Therefore zooko's __requires__ hack should work (modulo being completely undocumented). But if that's correct, then we only need [4764/ticket1190] and the change to setup.py from [4765/ticket1190], not the change to the .tac file generation.

(We could get rid of .tac files by adapting the code used for tahoe run, I think, but that's not this ticket.)

[davidsarah]

I reverted the changes to .tac generation on the ticket1190 branch: [4768/ticket1190].

Build results: ​http://tahoe-lafs.org/buildbot/waterfall?branch=ticket1190

[zooko]

I'm not completely sure I understand this, but I think these patches are wrongly thinking that run_trial.py wants to be run from the build/ directory: [20101028045609-93fa1-4ecc4f29843251b644e2f94a3c36d8eeeb91a3ab], [20101028040404-93fa1-adf26e78fa02437caa930936c33d8bd4346d7f9b], [20101028051106-93fa1-07679d72a9d09448744c1883ecf5fccc1c0110a1]. Instead I think it wants to be run from the src/ directory (or the egg, or the prefixinstall). Note that it sometimes prints out an error message like the following:

HACK Zooko-Ofsimplegeos-MacBook-Pro:~/playground/tahoe-lafs/ticket1190/build$ python ../misc/build_helpers/test-with-fake-pkg.py 

Traceback (most recent call last):
  File "/Users/zooko/playground/tahoe-lafs/ticket1190/src/../misc/build_helpers/run_trial.py", line 81, in <module>
    raise AssertionError(msg)
AssertionError: We seem to be testing the code at '/Users/zooko/playground/tahoe-lafs/ticket1190'
(according to the source filename '/Users/zooko/playground/tahoe-lafs/ticket1190/src/allmydata/test/test_base62.py'),
but expected to be testing the code at '/Users/zooko/playground/tahoe-lafs/ticket1190/src'.
This script needs to be run from the source directory to be tested.

Which is why I think it wants to have PWD be the src dir. This message was added in this patch: 3af6f19cb0f02fb4.

I will attach the "../misc/build_helpers/test-with-fake-pkg.py" file which was used to generate the above output.

[zooko]

Oh, the output in comment:11, made using test-with-fake-pkg.py​, also had this patch applied to misc/build_helpers/run_trial.py:

HACK Zooko-Ofsimplegeos-MacBook-Pro:~/playground/tahoe-lafs/ticket1190$ darcs whatsnew -u
hunk ./misc/build_helpers/run_trial.py 16
         if mo:
             return mo.group(1)
 
-version = read_version_py(os.path.join('lib', 'allmydata', '_version.py'))
+version = read_version_py(os.path.join('allmydata', '_version.py'))
 
 APPNAME='allmydata-tahoe'
 
hunk ./misc/build_helpers/run_trial.py 24
 # requirements.
 
 adglobals = {}
-execfile(os.path.join('lib', 'allmydata', '_auto_deps.py'), adglobals)
+execfile(os.path.join('allmydata', '_auto_deps.py'), adglobals)
 install_requires = adglobals['install_requires']
 
 __requires__ = [APPNAME + '==' + version] + install_requires

[zooko]

I updated the buildmaster config to turn on test-from-egg and test-from-prefixdir on all builders and triggered a build of all builders on the ticket1190 branch. Goodnight! :-)

[zooko]

I ran the build steps locally and this shows me more clearly the problem. The filesystem layout for the test-from-egg step is:

HACK Zooko-Ofsimplegeos-MacBook-Pro:~/playground/tahoe-lafs/ticket1190/egginstalldir$ find . \( -name _version.py -o -name _auto_deps.py \) 
./allmydata_tahoe-1.8.0_r4776-py2.6.egg/allmydata/_auto_deps.py
./allmydata_tahoe-1.8.0_r4776-py2.6.egg/allmydata/_version.py
./foolscap-0.5.1-py2.6.egg/foolscap/_version.py

For the test-from-prefixdir step it is:

HACK Zooko-Ofsimplegeos-MacBook-Pro:~/playground/tahoe-lafs/ticket1190/prefixinstalldir$ find . \( -name _version.py -o -name _auto_deps.py \)
./lib/python2.6/site-packages/allmydata/_auto_deps.py
./lib/python2.6/site-packages/allmydata/_version.py

and finally for the nascent test-with-fake-pkg.py it would be:

HACK Zooko-Ofsimplegeos-MacBook-Pro:~/playground/tahoe-lafs/ticket1190$ find . \( -name _version.py -o -name _auto_deps.py \) 
./build/lib/allmydata/_auto_deps.py
./build/lib/allmydata/_version.py
./darcsver-1.6.3.egg/darcsver/_version.py
./setuptools_darcs-1.2.11-py2.6.egg/setuptools_darcs/_version.py
./setuptools_trial-0.5.9.egg/setuptools_trial/_version.py
./src/allmydata/_auto_deps.py
./src/allmydata/_version.py
./support/lib/python2.6/site-packages/darcsver-1.6.3.egg/darcsver/_version.py
./support/lib/python2.6/site-packages/foolscap-0.5.1-py2.6.egg/foolscap/_version.py

We want to run run_trial.py from all three contexts.

Another source of confusion is that distutils creates a subdirectory named build, and buildbot creates a subdirectory named build, but (unless I'm already confused) these are different -- buildbot checks out the source into a subdirectory of the buildbot named build, and then runs python setup.py build in there, which creates a subdirectory of that directory which is also named build.

[davidsarah]

Replying to zooko:

Another source of confusion is that distutils creates a subdirectory named build, and buildbot creates a subdirectory named build, but (unless I'm already confused) these are different -- buildbot checks out the source into a subdirectory of the buildbot named build, and then runs python setup.py build in there, which creates a subdirectory of that directory which is also named build.

Yes, that was the main source of my confusion last night. run_trial.py needs to be run from the directory that contains the source that is to be tested (which will have allmydata as a direct subdirectory). So [4772/ticket1190] and [4774/ticket1190] are wrong.

[4775/ticket1190] is correct when running from the src directory of a tarball or darcs checkout, but to handle the test-from-prefixdir and test-from-egg cases, we need to tolerate the zetuptoolz egg not existing. (This will work provided that the system-installed setuptools on a builder is zetuptoolz. If it isn't, we'll still have setuptools- or distribute-specific bugs, but we'll be no worse off than we are now.)

[david-sarah@…]

In [4777/ticket1190]:

misc/build_helpers/run_trial.py: fix directory layout assumptions. Use a zetuptoolz egg if it exists but don't require one. refs #1190

[davidsarah]

[4778/ticket1190] gets the paths to _version.py and _auto_deps.py right this time :-)

[4779/ticket1190] adds 'mock' as a dependency in the __requires__ list when using run_trial.py. If there are other test dependencies in future, they can be specified in a test_requires variable of _auto_deps.py; this defaults to ['mock'].

Several of the buildslaves are still failing on the test-from-egg and test-from-prefixdir steps because they don't have the 'mock' package installed. This includes ​Ruben Fedora, so I can't check that the example in the description is fixed (also Shawn ubuntu-amd64 is offline). The ones that do have this package installed are working -- e.g. Freestorm-WinXP has gone green on both these steps when it was red before.

I've changed the buildmaster config (tahoe/bbgeneral.py) so that a failure or warning on the test-from-egg and test-from-prefixdir steps will make the whole build orange. This might need a buildmaster restart to take effect.

[david-sarah@…]

In [4780/ticket1190]:

add misc/build_helpers/test-with-fake-pkg.py. refs #1190

[david-sarah@…]

In 390c40cd8ce1e579:

add misc/build_helpers/test-with-fake-pkg.py. refs #1190

[davidsarah]

buildmaster: * tahoe/bbgeneral.py: mark the overall build as having warnings if the test-from-egg or test-from-prefixdir steps fail or warn. * tahoe/bbgeneral.py, bbsupport.py: add test-with-fake-pkg step

[davidsarah]

Replying to davidsarah:

I've changed the buildmaster config (tahoe/bbgeneral.py) so that a failure or warning on the test-from-egg and test-from-prefixdir steps will make the whole build orange. This might need a buildmaster restart to take effect.

Yes, it does. Also to add the test-with-fake-pkg step.

[david-sarah@…]

In [4781/ticket1190]:

misc/build_helpers/run_trial.py: correct off-by-one directory level when running from a src directory. refs #1190

[david-sarah@…]

In [4783/ticket1190]:

startstop_node.py: pyflakes import fix. refs #1190

[zooko]

Okay! There is now a buildstep named test-with-fake-pkg which tests this behavior, and all buildslaves which are able to run that buildstep (it requires mock) show that the current code in the ticket1190 branch passes. The next step is to ask Brian if he will not object to merging this to trunk, and then possibly rebasing it for trunk, or else just applying all 18 patches from ticket1190 to trunk.

Here are the buildbot results for ticket1190: ​http://tahoe-lafs.org/buildbot/waterfall?branch=ticket1190

Brian: do you agree to merge this branch onto trunk?

[zooko]

Oh, and this will need an entry in NEWS.

[davidsarah]

I will rebase these patches (no need to leave evidence of my mistakes in the history ;-)

[warner]

I'll look at the patch once it's rebased and I can think about just one change instead of 18.

[david-sarah@…]

In fd02946074821be9:

setup.py, misc/build_helpers/run_trial.py: use undocumented requires variable to cause setuptools/zetuptoolz to put the correct versions of dependencies on sys.path. Also ensure that run_trial adds the bundled zetuptoolz egg at the start of sys.path if present. Make the source directory comparison work correctly for the test-with-fake-pkg build step. refs #1190

[david-sarah@…]

In 647aa74d687157a9:

bundled zetuptoolz: if main.requires exists then do not add packages to the working set if they provide an incompatible version of a package. Also put a complete requires listing the transitive closure of dependencies at the beginning of generated scripts, rather than a shallow requires specifying only the application version. refs #1190

[david-sarah@…]

In 8835f009d0e6907a:

misc/build_helpers/run_trial.py: look for zetuptoolz egg in the parent directory, not the cwd of run_trial. refs #1190

[david-sarah@…]

In 2a8f700026a207d9:

scripts/runner.py: remove pkg_resources.require() calls. These are at best redundant because we have already called _auto_deps.require_auto_deps() (from allmydata.init) at that point, and they are causing failure of the test-from-prefixdir step on some buildslaves. refs #1190

[david-sarah@…]

In 1950d5a719b00175:

scripts/runner.py: fix unused import of allmydata. refs #1190

[david-sarah@…]

In 7cec440a14745159:

misc/build_helpers/test-with-fake-pkg.py: look for eggs in the parent of the src directory. refs #1190

[davidsarah]

We no longer have any VersionConflict errors on any of the builders. We do have DistributionNotFound errors (e.g. ​here for twisted) and ImportErrors (e.g. ​here for nevow and ​here for mock), but I think those are different problems.

[zooko]

Replying to davidsarah:

We do have DistributionNotFound errors (e.g. ​here for twisted)

I think this one might be a bug in Fedora: ​Fedora issue #523210.

You can see from ​this step that pkg_resources.require('Twisted') yields nothing even though import twisted works.

and ImportErrors (e.g. ​here for nevow

This is expected because test-from-prefixdir is explicitly not using the setuptools automatic resolution of dependencies. It disables that feature by passing --single-version-externally-managed. Since Eugen lenny-amd64 doesn't have Nevow installed into the system, it is expected to fail. Oh wait! But instead of failing by ImportError, it ought to fail by DistributionNotFound. It fails by ImportError because import nevow is present in allmydata/__init__.py​ before _auto_deps.require_auto_deps() is called.

and ​here for mock),

We need to apply a hack similar to 7cec440a14745159 to the test-from-egg step in the buildmaster configuration to fix this... There! I've updated the buildmaster config to add eggs to the sys.path from the source base dir (the dir that has setup.py instead of the CWD and that fixed it: ​test-from-egg step.

[zooko]

David-Sarah has written a NEWS entry. Once they commit it, this ticket can be closed.

[david-sarah@…]

In cb764da0edc2b161:

NEWS: entries for #1190 and #1212, and minor cleanups. refs #1190, #1212

[zooko]

I'm very sleepy so I might be misunderstanding, but I think this has regressed in trunk:

​http://tahoe-lafs.org/trac/tahoe-lafs/ticket/1233#comment:19

[zooko]

My problem that I reported in comment:19:ticket:1233 with test-with-fake-pkg not working on my mac was just due to the fact that you have to run python setup.py test before test-with-fake-pkg will work.

[zooko]

In the attempt to reproduce this, I installed setuptools-0.6c9 and foolscap 0.4.2, to emulate what is currently installed on Brian's linode builder. Here is the result on my laptop zomp of building ticket1233:

​http://tahoe-lafs.org/buildbot/builders/Zooko%20zomp%20Mac-amd64%2010.6%20py2.6/builds/234/steps/test-with-fake-dists/logs/stdio

aha! Same error!

pkg_resources.VersionConflict: (foolscap 0.4.2 (/Library/Python/2.6/site-packages/foolscap-0.4.2-py2.6.egg), Requirement.parse('foolscap[secure_connections]>=0.5.1'))

So it has something to do with my having downgraded just from now distribute 0.6.14 to setuptools 0.6c9, I guess.

same thing on trunk:

​http://tahoe-lafs.org/buildbot/builders/Zooko%20zomp%20Mac-amd64%2010.6%20py2.6/builds/235/steps/test-with-fake-dists/logs/stdio

I'm upgrading from setuptools-0.6c9 back to distribute-0.6.14 and trying again just to confirm.

I'm inclined to somehow mark this as a "wont-fix" if it is only a problem with that older version of setuptools.

On the other hand, I don't know how the system-installed version of setuptools manages to get into the act at all--we force our bundled zetuptoolz to be imported first thing in our setup.py. Is this VersionConflict arising from some other process than the one that loads our setup.py??

So, while I am really not motivated to develop a work-around for a bug in setuptools-0.6c9 (very old and now little-used), I am motivated to understand better what is going on here...

Help?

[zooko]

Oh look I upgraded to distribute-0.6.14 again and it still has the same error:

​http://tahoe-lafs.org/buildbot/builders/Zooko%20zomp%20Mac-amd64%2010.6%20py2.6/builds/236/steps/test-with-fake-dists/logs/stdio

So if you install foolscap-0.4.2 this is sufficient to trigger the bug.

[davidsarah]

In the 'test' step (and subsequent testing steps), we have previously done a build. In the 'test-with-fake-dists' step, we have not. I believe that's the difference that is causing this regression for 'test-with-fake-dists'.

Note that although 'setup.py test' is supposed to be equivalent to 'setup.py build' followed by 'setup.py trial', it's not quite equivalent because the sys.path will be different. ('build' has built things that can be put onto the path for 'trial', whereas a 'test' without a preceding 'trial' 'build' has to use whatever was previously installed.)

[zooko]

Okay I finally isolated the difference between getting this VersionConflict and not getting it on my laptop (zomp). I determined that if I rm src/allmydata/_version.py then run python misc/build_helpers/test-with-fake-dists.py I get the VersionConflict but if I don't rm src/allmydata/_version.py then I don't get the VersionConflict.

No doubt this has something to do with this code in our setup.py. :-)

As David-Sarah pointed out in comment:42, we're seeing this VersionConflict on some buildslaves now because I moved the invocation of test-with-fake-dists.py (by the buildmaster) to happen before any build step. (This is necessary because the build step provides a good version of pycryptopp so once it has run then the test-with-fake-dists.py step can't determine how well our setup scripts handle the lack of a good version of pycryptopp...)

[zooko]

Okay I've attempted to address this issue in our testing with 50f8c37a2b0049a5 (on ticket1233). Please review!

I triggered a build of ticket1233:

​http://tahoe-lafs.org/buildbot/waterfall?branch=ticket1233&first_time=1290152000&last_time=1290162000

[zooko]

Okay, the buildbot is nicely green now with &branch=ticket1233:

​http://tahoe-lafs.org/buildbot/waterfall?branch=ticket1233

[davidsarah]

b4c14421f7c2f25a is good as far as it goes. I still have reservations about the test-with-fake-dists step; the fact that it tests a package that really exists means that it can be fooled by an installed pycryptopp, and not test what it is supposed to be testing. But maybe it's sufficient for 1.8.1.

[zooko]

Currently it doesn't give a false positive unless pycryptopp-0.5.14 is installed (a very rare old version of pycryptopp). It can give a Skip, due to a recent version of pycryptopp being installed or a true FAIL. I agree it isn't perfect, but I haven't been able to figure out how to do better.

I guess really we should have a unit test of zetuptoolz which constructs a fake package with a dependency to test this behavior of zetuptoolz.

As far as a test of Tahoe-LAFS--I'm okay with this one. :-)

So I'm going to interpret your comment:46 as a +0 and remove the review-needed flag and add reviewed.

[zooko]

Committed to trunk as 50f8c37a2b0049a5.

[zooko]

So I think we need to wait and see if my patches fix #1233 on trunk and if so then we can close this ticket as fixed as well as that one.

[zooko]

Okay! Looks like it worked.