can't test on buildslaves that we don't depend on pywin32 #1334

New Issue

tahoe-lafs · 2011-01-21T22:36:56Z

davidsarah commented

2011-01-21 22:36:56 +00:00

#1274 removed Tahoe's direct and indirect dependencies on pywin32.

However, it appears that buildbot depends on pywin32 on Windows, so we can't uninstall it on the Windows buildslaves to automatically test that it isn't needed (and avoid regressions).

#1274 removed Tahoe's direct and indirect dependencies on pywin32. However, it appears that buildbot depends on pywin32 on Windows, so we can't uninstall it on the Windows buildslaves to automatically test that it isn't needed (and avoid regressions).

tahoe-lafs added the

labels 2011-01-21 22:36:56 +00:00

tahoe-lafs added this to the undecided milestone 2011-01-21 22:36:56 +00:00

tahoe-lafs changed title from ~~can't run buildbots without pywin32 installed~~ to can't run buildslaves without pywin32 installed

2011-01-21 22:37:29 +00:00

davidsarah commented

2011-07-20 21:50:28 +00:00

After looking at buildbot's dependencies on pywin32 (including via Twisted, for process control), I doubt this will be fixed any time soon. We'll just have to live with having to run the test suite manually in order to check that we don't depend on pywin32.

tahoe-lafs added the

wontfix

label 2011-07-20 21:50:28 +00:00

davidsarah closed this issue

2011-07-20 21:50:28 +00:00

davidsarah commented

2011-07-20 21:58:35 +00:00

Replying to davidsarah:

After looking at buildbot's dependencies on pywin32 (including via Twisted, for process control), I doubt this will be fixed any time soon. We'll just have to live with having to run the test suite manually in order to check that we don't depend on pywin32.

Oh hang on, maybe it's not necessary that pywin32 not be installed in order for us to check that we don't import it. We can have a test that runs last and does self.failIfIn("pywin32", sys.modules).

Replying to [davidsarah](/tahoe-lafs/trac-2024-07-25/issues/1334#issuecomment-82279): > After looking at buildbot's dependencies on pywin32 (including via Twisted, for process control), I doubt this will be fixed any time soon. We'll just have to live with having to run the test suite manually in order to check that we don't depend on pywin32. Oh hang on, maybe it's not necessary that pywin32 not be installed in order for us to check that we don't import it. We can have a test that runs last and does `self.failIfIn("pywin32", sys.modules)`.

tahoe-lafs removed the

wontfix

label 2011-07-20 21:58:35 +00:00

tahoe-lafs modified the milestone from undecided to 1.9.0

2011-07-20 21:58:35 +00:00

davidsarah reopened this issue

2011-07-20 21:58:35 +00:00

tahoe-lafs changed title from ~~can't run buildslaves without pywin32 installed~~ to can't test on buildslaves that we don't depend on pywin32

2011-07-20 21:59:35 +00:00

davidsarah commented

2011-07-21 01:56:01 +00:00

Replying to [davidsarah]comment:3:

Oh hang on, maybe it's not necessary that pywin32 not be installed in order for us to check that we don't import it. We can have a test that runs last and does self.failIfIn("pywin32", sys.modules).

Unfortunately it wasn't as simple as that. Several of our dependencies (for example, Twisted) will try to import pywin32 modules, even though they tolerate unavailability of those modules. So we have to block loading of pywin32 (and do so fairly early, before we've tried to import any such dependencies) in order to be able to test that we don't need it.

Replying to [davidsarah]comment:3: > Oh hang on, maybe it's not necessary that pywin32 not be installed in order for us to check that we don't import it. We can have a test that runs last and does `self.failIfIn("pywin32", sys.modules)`. Unfortunately it wasn't as simple as that. Several of our dependencies (for example, Twisted) will try to import pywin32 modules, even though they tolerate unavailability of those modules. So we have to block loading of pywin32 (and do so fairly early, before we've tried to import any such dependencies) in order to be able to test that we don't need it.

davidsarah commented

2011-07-21 01:56:55 +00:00

Attachment pywin32-just-say-no.darcs.patch (19164 bytes) added

Prevent pywin32 modules from loading, and test at the end of the test suite that they haven't. refs #1274. fixes #1334'

**Attachment** pywin32-just-say-no.darcs.patch (19164 bytes) added Prevent pywin32 modules from loading, and test at the end of the test suite that they haven't. refs #1274. fixes #1334'

pywin32-just-say-no.darcs.patch

19 KiB

davidsarah commented

2011-07-21 02:00:51 +00:00

It's really ugly that pywin32-just-say-no.darcs.patch adds to the complexity of src/allmydata/*init*.py, but I couldn't find any other way to do it (in particular, source:src/allmydata/windows/fixups.py is too late because pywin32 modules would already have been loaded by then).

It's really ugly that [pywin32-just-say-no.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-2d97-d80f-5af4-72364d7bdce0) adds to the complexity of `src/allmydata/*init*.py`, but I couldn't find any other way to do it (in particular, source:src/allmydata/windows/fixups.py is too late because pywin32 modules would already have been loaded by then).

zooko commented

2011-07-21 20:05:12 +00:00

I'm not sure if it is worth it to add complexity to source:src/allmydata/init.py in order to test this. We could also test it by having a buildslave with no pywin32 on it, right? Maybe also in concert with a "desert island build" test which goes red if the build system downloads anything.

In general I've recently been trying to find ways to test code without altering the code to add test hooks, although if this is worth it in specific then I don't object.

I'm not sure if it is worth it to add complexity to source:src/allmydata/*init*.py in order to test this. We could also test it by having a buildslave with no pywin32 on it, right? Maybe also in concert with a "desert island build" test which goes red if the build system downloads anything. In *general* I've recently been trying to find ways to test code without altering the code to add test hooks, although if this is worth it in specific then I don't object.

davidsarah commented

2011-07-22 01:53:54 +00:00

Replying to zooko:

I'm not sure if it is worth it to add complexity to source:src/allmydata/init.py in order to test this. We could also test it by having a buildslave with no pywin32 on it, right?

No, because the part of buildbot that runs on the buildslaves depends on pywin32. The only other way to test this, I think, would be to make pywin32 available to the buildbot code without being installed, and without being available to subprocesses. There are two ways to do that that I can think of:

add an entry for pywin32 to sys.path in the startup script for the buildslave;
add an entry for pywin32 to PYTHONPATH when starting the buildslave (or globally), but then in the buildbot step that runs the Tahoe test suite, remove that entry from the PYTHONPATH passed to the subprocess.

Replying to [zooko](/tahoe-lafs/trac-2024-07-25/issues/1334#issuecomment-82289): > I'm not sure if it is worth it to add complexity to source:src/allmydata/*init*.py in order to test this. We could also test it by having a buildslave with no pywin32 on it, right? No, because the part of buildbot that runs on the buildslaves depends on pywin32. The only other way to test this, I think, would be to make pywin32 available to the buildbot code without being installed, and without being available to subprocesses. There are two ways to do that that I can think of: 1. add an entry for pywin32 to `sys.path` in the startup script for the buildslave; 2. add an entry for pywin32 to `PYTHONPATH` when starting the buildslave (or globally), but then in the buildbot step that runs the Tahoe test suite, remove that entry from the `PYTHONPATH` passed to the subprocess.

david-sarah@jacaranda.org commented

2011-07-22 03:47:23 +00:00

In changeset:6e0607f4e05b159c:

misc/build_helpers/run_trial.py: ensure that pywin32 is not on the sys.path when running the test suite. Includes some temporary debugging printouts that will be removed. refs #1334

In changeset:6e0607f4e05b159c: ``` misc/build_helpers/run_trial.py: ensure that pywin32 is not on the sys.path when running the test suite. Includes some temporary debugging printouts that will be removed. refs #1334 ```

david-sarah@jacaranda.org commented

2011-07-22 04:52:09 +00:00

In changeset:91c7cf9007ae5b5d:

misc/build_helpers/run_trial.py: undo change to block pywin32 (it didn't work because run_trial.py is no longer used). refs #1334

In changeset:91c7cf9007ae5b5d: ``` misc/build_helpers/run_trial.py: undo change to block pywin32 (it didn't work because run_trial.py is no longer used). refs #1334 ```

davidsarah commented

2011-07-24 00:46:36 +00:00

I no longer favour the approach used in the patch of blocking loading of pywin32 in allmydata/*init*.py. It's too ugly and intrusive to install an import hook just to avoid loading a library that we don't want to have to care about any more.

I have suggested to our win32 buildslave operators, Freestorm and Dcoder, the following changes to their set up, that should achieve a similar effect of only allowing pywin32 to be used by the initial buildslave process:

Assume that the Python directory is C:\Python27 (if not then adjust the paths below). Stop the buildslave, then do:

ren C:\Python27\Lib\site-packages\pywin32.pth pywin32.pth.disabled

Edit C:\Python27\Scripts\buildbot (using an editor that can handle Unix line endings). Add the following lines just after the !# line:

import os, sys
site = os.path.join(os.path.basename(sys.executable), 'Lib', 'site-packages')
sys.path += [os.path.join(site, d) for d in
             ['win32', 'win32\\lib', 'Pythonwin']]

Similarly, in C:\Python27\Scripts\buildbot_service.py, after 'import threading' but before 'import pywintypes', add:

site = os.path.join(os.path.basename(sys.executable), 'Lib', 'site-packages')
sys.path += [os.path.join(site, d) for d in
             ['win32', 'win32\\lib', 'Pythonwin']]

(i.e. the same thing without the 'import os, sys').

The names of the buildbot and buildbot_service.py scripts might be different depending on the version of Buildbot. (There are some references to a 'buildslave' script in the docs which wasn't present in my copy, but if it exists in yours then it will need a similar modification.) You may want to back up the old versions of the scripts before editing them.

To check that pywin32 can't be imported by code other than buildbot, do:

python -c "import win32api; print win32api"

That should give an `ImportError`. Now restart the buildslave and check that it still works.

Freestorm and Dcoder haven't got back to me yet.

I no longer favour the approach used in the patch of blocking loading of pywin32 in `allmydata/*init*.py`. It's too ugly and intrusive to install an import hook just to avoid loading a library that we don't want to have to care about any more. I have suggested to our win32 buildslave operators, Freestorm and Dcoder, the following changes to their set up, that should achieve a similar effect of only allowing pywin32 to be used by the initial buildslave process: ---- Assume that the Python directory is `C:\Python27` (if not then adjust the paths below). Stop the buildslave, then do: ``` ren C:\Python27\Lib\site-packages\pywin32.pth pywin32.pth.disabled ``` Edit `C:\Python27\Scripts\buildbot` (using an editor that can handle Unix line endings). Add the following lines just after the `!#` line: ``` import os, sys site = os.path.join(os.path.basename(sys.executable), 'Lib', 'site-packages') sys.path += [os.path.join(site, d) for d in ['win32', 'win32\\lib', 'Pythonwin']] ``` Similarly, in `C:\Python27\Scripts\buildbot_service.py`, after '`import threading`' but before '`import pywintypes`', add: ``` site = os.path.join(os.path.basename(sys.executable), 'Lib', 'site-packages') sys.path += [os.path.join(site, d) for d in ['win32', 'win32\\lib', 'Pythonwin']] ``` (i.e. the same thing without the '`import os, sys`'). The names of the `buildbot` and `buildbot_service.py` scripts might be different depending on the version of Buildbot. (There are some references to a '`buildslave`' script in the docs which wasn't present in my copy, but if it exists in yours then it will need a similar modification.) You may want to back up the old versions of the scripts before editing them. To check that pywin32 can't be imported by code other than buildbot, do: ``` python -c "import win32api; print win32api" ``` That should give an `ImportError`. Now restart the buildslave and check that it still works. ---- Freestorm and Dcoder haven't got back to me yet.

zooko commented

2011-07-24 03:53:00 +00:00

For future reference, the "exocet" tool from Allen Short is an importer (which I think means it replaces the thing that currently implements the import keyword) which can do things like blacklist modules. Might ease the sort of hack that we're currently deciding not to do to the longsuffering src/allmydata/*init*.py.

For future reference, the ["exocet"](https://launchpad.net/exocet) tool from Allen Short is an importer (which I think means it replaces the thing that currently implements the `import` keyword) which can do things like blacklist modules. Might ease the sort of hack that we're currently deciding not to do to the longsuffering `src/allmydata/*init*.py`.

davidsarah commented

2011-07-24 17:39:19 +00:00

Replying to zooko:

For future reference, the "exocet" tool from Allen Short is an importer (which I think means it replaces the thing that currently implements the import keyword) which can do things like blacklist modules.

Well, that's only 6 lines in pywin32-just-say-no.darcs.patch:

class BlockPywin32Finder(object):
    def find_module(self, fullname, path=None):
        if fullname.split('.')[0] in pywin32modules:
            raise ImportError(fullname)
        return None
sys.meta_path.insert(0, BlockPywin32Finder())

If 6 lines is too complicated, then dependency on another tool certainly is!

Replying to [zooko](/tahoe-lafs/trac-2024-07-25/issues/1334#issuecomment-82294): > For future reference, the ["exocet"](https://launchpad.net/exocet) tool from Allen Short is an importer (which I think means it replaces the thing that currently implements the `import` keyword) which can do things like blacklist modules. Well, that's only 6 lines in [pywin32-just-say-no.darcs.patch](/tahoe-lafs/trac-2024-07-25/attachments/000078ac-2d97-d80f-5af4-72364d7bdce0): ``` class BlockPywin32Finder(object): def find_module(self, fullname, path=None): if fullname.split('.')[0] in pywin32modules: raise ImportError(fullname) return None sys.meta_path.insert(0, BlockPywin32Finder()) ``` If 6 lines is too complicated, then dependency on another tool certainly is!

davidsarah commented

2011-07-24 17:44:40 +00:00

Not to mention that exocet tries to import modules from pywin32 :-)

Not to mention that [exocet tries to import modules from pywin32](http://bazaar.launchpad.net/~washort/exocet/katamari/view/head:/exocet/_win32.py#L18) :-)

zooko commented

2011-07-25 17:38:05 +00:00

I didn't mean we would use exocet solely to avoid those six lines. It may be a thing that we prefer to use in general, if avoiding these six lines turns out to be one of several benefits that it brings. I've opened use exocet instead of the builtin Python module loader: #1443.