bug in error path of cross_check_pkg_resources_versus_import

[warner]

Jimmy Tang ​wrote to the mailing list with a crash on his Archlinux box.

  File "/home/jtang/tmp/tahoe-lafs/src/allmydata-tahoe-1.8.2/src/allmydata/__init__.py",
line 301, in cross_check_pkg_resources_versus_import
    % (imp_ver, name, imp_loc, pr_ver, pr_loc, e.__class__.name, e))
AttributeError: type object 'exceptions.TypeError' has no attribute 'name'
    Aborting...

I don't know if this is a real error path, or if it's just accumulating a bunch of warnings to print to stderr (i.e. if we just took out the offending line, would the program work anyways, or is the missing/old/unable-to-compute-version dependency actually important?).

This sort of problem reinforces my feeling that our __init__.py is way too complex, and that all the version-checking logic may be doing more harm than good.

This may provoke a 1.8.3, if it happens frequently enough. If the bad e.__class__.name is only triggered under weird combinations of installed dependencies, then we might be able to put it off until 1.9.0 .

[davidsarah]

warner wrote in the original Description:

I suspect it's a python-2.7 incompatibility in the error-handling path in all the crazy what-version-of-each-dependency code inside __init__.py:

...

I suspect that TypeError moved from being implemented in Python (in py2.6) to being implemented in C (in py2.7), and that for some reason when you try to do the .name lookup, it explodes messily.

It should have been e.__class__.__name__. Sorry, my fault for not testing this error path adequately. It's not at all Python 2.7-specific.

(For future reference: the Description field isn't the right place to speculate about the cause of a bug.)

This may provoke a 1.8.3, if it happens frequently enough. If the bad e.__class__.name is only triggered under weird combinations of installed dependencies, then we might be able to put it off until 1.9.0 .

This error occurs on lines 287 and 301 of __init__.py. It happens when:

• the --version or --version-and-path option to bin/tahoe is used, and
• the version of a package obtained either from import or from pkg_resources, is not parseable at all by src/allmydata/util/verlib.py. (A non-normalized but parseable version is fine.)

--version-and-path is passed to bin/tahoe by default when you run tests via setup.py. You can use the --quiet option (i.e. setup.py test --quiet or setup.py trial --quiet) to suppress this.

I don't know if this is a real error path, or if it's just accumulating a bunch of warnings to print to stderr (i.e. if we just took out the offending line, would the program work anyways, or is the missing/old/unable-to-compute-version dependency actually important?).

We don't know whether the warning was indicative of a real error in this case, because we don't know what the version or package was. The best way to check that would be for Jimmy Tang to replace .name with .__name__ on lines 287 and 301 of __init__.py and then see what is printed.

It's not clear to me how common it is to have versions that verlib.py will raise an exception when trying to parse.

[davidsarah]

Replying to davidsarah:

This error [...] happens when:

• the --version or --version-and-path option to bin/tahoe is used, and
• the version of a package obtained either from import or from pkg_resources, is not parseable at all by src/allmydata/util/verlib.py. (A non-normalized but parseable version is fine.)

Oops, cross_check_pkg_resources_versus_import is also called by get_package_versions_string, which is used when constructing a storage client or introducer node. This will prevent running nodes whenever the second condition above holds.

This situation is not happening on any of the buildbots, so it can't be very common. However I think it still requires a 1.8.3 :-(

[zooko]

I think this is so rare that it doesn't require 1.8.3.

It is quite rare for one of our deps to have a version number that can't be parsed by verlib.py.

[davidsarah]

Add unit tests for cross_check_pkg_resources_versus_import, and a regression test for ref #1355. This requires a little refactoring to make it testable.

[davidsarah]

allmydata/init.py: .name was used in place of the correct .name when printing an exception. Also, robustify string formatting by using %r instead of %s in some places. fixes #1355.

[david-sarah@…]

In 71c301ca3444f41e:

allmydata/init.py: .name was used in place of the correct .name when printing an exception. Also, robustify string formatting by using %r instead of %s in some places. fixes #1355.

[davidsarah]

The test patch was applied as 787d12165af49a4a (trac didn't auto-comment for some reason).