tahoe-lafs/trac

stop putting pkg_resources.require() into .tac files #1539

New issue

Closed

opened 2011-09-22 20:05:33 +00:00 by zooko · 14 comments

zooko commented

2011-09-22 20:05:33 +00:00

Per comment:380140:

One thing that we can do right away to ease this is change create-node to stop putting the call to pkg_resources.require() in the tahoe-client.tac.

Per [comment:380140](/tahoe-lafs/trac/issues/1159#issuecomment-380140): One thing that we can do right away to ease this is change create-node to stop putting the call to pkg_resources.require() in the tahoe-client.tac.

zooko added the

labels 2011-09-22 20:05:33 +00:00

zooko added this to the undecided milestone 2011-09-22 20:05:33 +00:00

daira commented

2011-09-24 00:13:12 +00:00

Agreed.

daira modified the milestone from undecided to 1.10.0

2011-09-24 00:13:12 +00:00

daira self-assigned this 2011-09-24 00:13:12 +00:00

zooko commented

2011-09-24 03:06:12 +00:00

Author

There are two calls to pkg_resources.require() in the .tac files that we currently produce. According to changeset:1aed9fcfa1c45538 the patch that added them, this was necessary to make them importable if they were installed in setuptools's "multiversion mode". I don't think we need to support that. We experimented with it (see #530) and removing multiversion mode from our setup.py did not break any of our unit tests, not even test-with-fake-pkg or test-with-fake-dists.

There are two calls to `pkg_resources.require()` in the .tac files [that we currently produce](http://tahoe-lafs.org/trac/tahoe-lafs/browser/trunk/src/allmydata/scripts/create_node.py?annotate=blame&rev=5133#L43). According to changeset:1aed9fcfa1c45538 the patch that added them, this was necessary to make them importable if they were installed in setuptools's "multiversion mode". I don't think we need to support that. We experimented with it (see #530) and removing multiversion mode from our setup.py did not break any of our unit tests, not even test-with-fake-pkg or test-with-fake-dists.

daira commented

2011-09-24 03:48:24 +00:00

Replying to zooko:

There are two calls to pkg_resources.require() in the .tac files that we currently produce. According to changeset:1aed9fcfa1c45538 the patch that added them, this was necessary to make them importable if they were installed in setuptools's "multiversion mode".

Those calls should never have been necessary even for multiversion mode; I think the commit comment was wrong. pkg_resources.require() does not, despite the misleading name and documentation, make a package available that wouldn't already have been available. The set of packages available in the working set is determined at the point of first importing pkg_resources, which happens in the setuptools-generated support script, long before the .tac is executed.

So I guess I'm violently agreeing with your conclusion that these calls aren't necessary ;-)

Replying to [zooko](/tahoe-lafs/trac/issues/1539#issuecomment-386952): > There are two calls to `pkg_resources.require()` in the .tac files [that we currently produce](http://tahoe-lafs.org/trac/tahoe-lafs/browser/trunk/src/allmydata/scripts/create_node.py?annotate=blame&rev=5133#L43). According to changeset:1aed9fcfa1c45538 the patch that added them, this was necessary to make them importable if they were installed in setuptools's "multiversion mode". Those calls should never have been necessary even for multiversion mode; I think the commit comment was wrong. `pkg_resources.require()` does not, despite the misleading name and documentation, make a package available that wouldn't already have been available. The set of packages available in the working set is determined at the point of first importing `pkg_resources`, which happens in the setuptools-generated support script, long before the .tac is executed. So I guess I'm violently agreeing with your conclusion that these calls aren't necessary ;-)

daira modified the milestone from 1.11.0 to 1.10.0

2012-04-01 03:55:22 +00:00

warner commented

2012-05-23 04:58:44 +00:00

Attachment 1539-smaller-tac.diff (1122 bytes) added

remove pkg_resources calls from generated .tac files

**Attachment** 1539-smaller-tac.diff (1122 bytes) added remove pkg_resources calls from generated .tac files

1539-smaller-tac.diff

1.1 KiB

warner commented

2012-05-23 04:59:13 +00:00

something like that?

daira commented

2012-05-23 15:43:43 +00:00

Will review.

Note that import pkg_resources has side effects on sys.path even if pkg_resources.require() is not used; so, removing the import might have an additional effect. I can't immediately remember whether that's desirable or not; need to page this stuff back into my brain.

Will review. Note that `import pkg_resources` has side effects on `sys.path` even if `pkg_resources.require()` is not used; so, removing the import might have an additional effect. I can't immediately remember whether that's desirable or not; need to page this stuff back into my brain.

zooko commented

2012-05-23 16:24:51 +00:00

Author

According to changeset:1aed9fcfa1c45538, I added that so that it would find an allmydata and twisted package tht had been installed in "multi-version mode". I guess that mode of install offered by setuptools doesn't put the installed egg into the sys.path at all until a pkg_resources.require() with a matching version number has been invoked. We experimented with it at one point as a way to work around other problems, but I believe we gave up on using it.

According to changeset:1aed9fcfa1c45538, I added that so that it would find an allmydata and twisted package tht had been installed in "multi-version mode". I guess that mode of install offered by setuptools doesn't put the installed egg into the sys.path at *all* until a `pkg_resources.require()` with a matching version number has been invoked. We experimented with it at one point as a way to work around other problems, but I believe we gave up on using it.

daira commented

2012-09-04 16:11:13 +00:00

Obsolete if we accept #1159.

warner commented

2013-03-21 18:44:48 +00:00

I want this, but won't block 1.10 for it. Kicking it and its cousin #1159 into 1.11.

warner modified the milestone from 1.10.0 to 1.11.0

2013-03-21 18:44:48 +00:00

zooko modified the milestone from soon to 1.11.0

2013-08-28 15:58:55 +00:00

remyroy commented

2014-03-20 15:44:34 +00:00

Owner

I'll be doing the review on this one.

daira commented

2014-03-20 18:23:50 +00:00

I would prefer to keep the pkg_resources import just because:

removing it isn't necessary for the purpose of this patch;
importing pkg_resources can change sys.path, which can change where allmydata is imported from.

If the .tac file is run via tahoe start or tahoe run, then pkg_resources will already have been imported and therefore importing it again will have no effect -- but this is dependent on the current implementation of tahoe start/run. If the .tac were run directly via twistd or another Python application, then this could have an effect. It is never correct to import allmydata and then have the sys.path change as a result of importing pkg_resources, so it is safer to import it first. (When we switch to not using the contents of the .tac files, it won't matter either way, but we're not doing that yet.)

I would prefer to keep the `pkg_resources` import just because: * removing it isn't necessary for the purpose of this patch; * importing `pkg_resources` can change `sys.path`, which can change where `allmydata` is imported from. If the `.tac` file is run via `tahoe start` or `tahoe run`, then `pkg_resources` will already have been imported and therefore importing it again will have no effect -- but this is dependent on the current implementation of `tahoe start`/`run`. If the `.tac` were run directly via `twistd` or another Python application, then this could have an effect. It is never correct to import `allmydata` *and then* have the `sys.path` change as a result of importing `pkg_resources`, so it is safer to import it first. (When we switch to not using the contents of the `.tac` files, it won't matter either way, but we're not doing that yet.)

remyroy commented

2014-03-21 18:40:03 +00:00

Owner

Review of 1539-smaller-tac.diff:

Everything is great except that it breaks the following test: allmydata.test.test_drop_upload.RealTest.test_drop_upload . The patch should be changed so that no test is broken.

daira has an objection to this change so I'm not sure who I should reassign this to.

Review of 1539-smaller-tac.diff: Everything is great except that it breaks the following test: allmydata.test.test_drop_upload.RealTest.test_drop_upload . The patch should be changed so that no test is broken. daira has an objection to this change so I'm not sure who I should reassign this to.

daira commented

2014-09-02 18:03:14 +00:00

I will review this together with #1159.

zooko was unassigned by daira

2014-09-02 18:03:14 +00:00

daira self-assigned this 2014-09-02 18:03:14 +00:00

daira commented

2014-10-27 01:22:17 +00:00

This is wontfix because we decided to stop using the contents of .tac files instead (#1159, fixed in changeset:87a6894e6299148d639279fbd909110ba00d0080).

This is wontfix because we decided to stop using the contents of `.tac` files instead (#1159, fixed in changeset:87a6894e6299148d639279fbd909110ba00d0080).