homepage

While trying to debug a testsuite crash, I discovered that a faulty module from the system wide directories was being picked up. The module in question was sgmlop.so, which was being imported by Lib/xmlrpclib.py. (BTW isn't it considered bad form for a module within the Python distribution to load a third party module?)
In my opinion, the main issue here is that while testing a Python build, no out-of-tree modules should be able to be imported in any case, in order to avoid pollution of the testing environment. Yet AFAICT, there is no place where the sys.path is manipulated to remove the out-of-tree directories. I believe that the proper thing to do would be to allow only the build tree module directories in the search path.
Any comments/patches would be appreciated.
Best Regards,
Theodoros Kalamatianos

Theodoros,
Can you provide a short script that reproduces this?

I noticed the same problem in many tests.
E.g. test___all__ fails when pyxml is installed.

One way to "fix" this would be to have make test run the tests with -j1 and pass in the -S and -s flags, and then have regrtest special case test_site and remove those flags for the run of that single test.
An interesting facet of this proposal in that it actually isolates the tests better. But it will also slow down the test suite run a bit. Since the tests are already run twice by make test, that may not matter all that much (that is, it's a start-the-run-and-walk-away situation anyway).
Note that all tests except test_trace currently pass with -S -s.

Here is a proof of concept patch if anyone wants to play with it. Note that a higher value could be used for the j option; multiple threads help even on uniprocessor systems since a bunch of the tests spend time waiting around.
The patch removes the '-l' option from the make test run. I'm not sure that matters, since we do our best to fix memory leaks before shipping a release, and we don't use make test to do our leak testing (as far as I know).
I'm not sure this is an appropriate solution, so I won't apply this unless I get some favorable votes from committers and/or packagers. 
One interesting thing is that several additional tests show up as altering the execution environment when run this way. That bears investigation at some point, but is an orthogonal issue.
As noted, test_trace does not pass, but only one test within it fails, so that ought to be easy enough to fix. Also note that this "fix" would only be applicable to 3.2 and 2.7.

I find the solution (running every test in a subprocess) a bit too drastic for the problem. How about a modified approach: run regrtest with -S, and have it create a subprocess for test_site only?

Apparently this patch isn't sufficient for test___all__.
Please create empty _xmlplus directory (without __init__.py) in site-packages directory appropriate for given sys.prefix (e.g. /usr/lib/python3.2/site-packages/_xmlplus for sys.prefix='/usr').
The output of `make test` contains:
LD_LIBRARY_PATH=/var/tmp/portage/dev-lang/python-3.2_pre20110102/work/python-3.2_pre20110102: ./python -Wd -E -bb -S -s ./Lib/test/regrtest.py -j1 
== CPython 3.2b2+ (py3k:87612, Jan 3 2011, 18:30:27) [GCC 4.4.5]
== Linux-2.6.34-tuxonice-r8-AFTA-x86_64-Intel-R-_Pentium-R-_Dual_CPU_T2390_@_1.86GHz-with-gentoo-2.0.1 little-endian
== /var/tmp/portage/dev-lang/python-3.2_pre20110102/work/python-3.2_pre20110102/build/test_python_2301
[ 1/349] test_grammar
[ 2/349] test_opcodes
[ 3/349] test_dict
[ 4/349] test_builtin
[ 5/349] test_exceptions
[ 6/349] test_types
[ 7/349] test_unittest
[ 8/349] test_doctest
[ 9/349] test_doctest2
[ 10/349] test___all__
Warning -- sys.path was modified by test___all__
test test___all__ failed -- Traceback (most recent call last):
 File "/var/tmp/portage/dev-lang/python-3.2_pre20110102/work/python-3.2_pre20110102/Lib/test/test___all__.py", line 101, in test_all
 self.check_all(modname)
 File "/var/tmp/portage/dev-lang/python-3.2_pre20110102/work/python-3.2_pre20110102/Lib/test/test___all__.py", line 28, in check_all
 raise FailedImport(modname)
 File "/var/tmp/portage/dev-lang/python-3.2_pre20110102/work/python-3.2_pre20110102/Lib/contextlib.py", line 35, in __exit__
 next(self.gen)
 File "/var/tmp/portage/dev-lang/python-3.2_pre20110102/work/python-3.2_pre20110102/Lib/test/support.py", line 623, in _filterwarnings
 raise AssertionError("unhandled warning %s" % reraise[0])
AssertionError: unhandled warning {message : ImportWarning("Not importing directory '/usr/lib/python3.2/site-packages/_xmlplus': missing __init__.py",), category : 'ImportWarning', filename : '/var/tmp/portage/dev-lang/python-3.2_pre20110102/work/python-3.2_pre20110102/Lib/xml/__init__.py', lineno : 26, line : None}
[ 11/349] test___future__
[ 12/349] test__locale
[ 13/349] test_abc

I agree with Martin here.
Also, since test_site is a special case here, perhaps it could arrange to launch its test cases in a subprocess? I don't think complicating regrtest is the most maintainable approach.

The java/openjdk tests allow setting an attribute `samevm' for running a specific test. maybe something like this could be used for some problematic tests which occasionally hang on some buildds?

Yeah, making a generic way to put specific tests into a subprocess run sounds like a better solution.
But...
The xml error in test___all__ is due to the fact that test___all__ imports site. So even with the above we'd need to special case site in test___all__. This probably answers the question of why more tests show as changing the environment when run with -S -s, so there are probably other tests that would need to be touched as well.
This is becoming a bit of a jerry-rig. Anyone have a better idea? A way to identify the "system paths" (or, conversely, identify the paths *not* added by site) and delete them in regrtest before running the tests might be cleaner.
Or another idea: change site so that it does not execute on import, but instead the machinery that currently imports test site runs a 'setup' function after it does the import. If test_site were then to test the setup function and take care to restore sys.path when it was done, I think things would work as expected if all tests were run with -S -s. Of course, this would break all the custom site.py's out there, so it is probably a non-starter of an idea.
In the meantime, test___all__ could perhaps be made more robust in the face of import errors/warnings for those few modules that import from external libraries (xml, logging...anything else?)

Le lundi 03 janvier 2011 à 19:37 +0000, R. David Murray a écrit :
> Or another idea: change site so that it does not execute on import,
> but instead the machinery that currently imports test site runs a
> 'setup' function after it does the import.
I'm not sure this means anything :) Importing a module automatically
entails running its top-level code (that's the only way it can create
functions, classes, etc.).
However, I agree that in the default site.py, we could stop calling
main() at the top-level and instead call it from the importing code.
> Of course, this would break all the custom site.py's out there, so it
> is probably a non-starter of an idea.
Are there many of them? And usually, you would take the original site.py
and modify it a little. Starting from scratch sounds crazy.
> In the meantime, test___all__ could perhaps be made more robust in the
> face of import errors/warnings for those few modules that import from
> external libraries (xml, logging...anything else?)
Well, ideally, these modules should stop importing external libraries
implicitly. Give them a dedicated function for that instead, that the
user can call if they want to.

Ah, I hadn't looked closely enough at site.py to realize that the init work was being done by a 'main()' call. Given that, just moving the main call out should be relatively unlikely to break any custom site.py. Worse case would presumably be main() getting called twice, which could be annoying but probably not catastrophic. One would like to think that anyone customizing site.py would redo the customization for a major release and would notice. (And no, I don't know how common customized site.py's are.)
Logging already only does its import if you construct an NTEventLogHandler, so that's probably safe enough. Changing xml to require a function call might make some people mad, and other people happy...
I wonder if there are any other stdlib modules that do these kinds of external imports. I suppose we can just fix xml and wait for additional bug reports :)
I'm not sure either of these changes (site.py-call or xml-activate-xmlplus) should be backported, though, assuming we go ahead with them.

I'm attaching the patch, which works for me.
- New, private variable (_PYTHONNOSITEPACKAGES) disables addition of site-packages directories to sys.path.
- regrtest.py always runs test_site.py in a subprocess.

Note that site’s behavior is a bit different in 3.3 thanks to #11591. See http://docs.python.org/dev/library/site#site.main 

The advantage of an environment variable is that it is by default inherited by subprocesses.
(These patches no longer apply cleanly. I will create updated patches.)

Issue #16817 discusses a similar problem arising with test___all__. The patches have to be updated to provide a generic way to delegate certain tests to separate subprocesses.
Arfrever, it's not necessary to update all the patches at once. Start with the one for 3.2 and after we figure out it's acceptable, it should be easy to convert to the other branches.

@Arfrever will you create updated patches as stated in msg136552 ?

I update my patches every time they stop applying, but I do not feel that it is necessary to frequently attach new patches here.

I did a quick merge of Arfrever’s 2.7 patch with current code (mainly pgo flags). Unfortunately it didn’t fix my problem with unhandled ImportWarning, but here it is in case it is useful.