696535 - "make check" and "pymake check" fail because build/'s own "make check" fails to find manifestparser

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P5)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Get "make check" working

+++ This bug was initially created as a clone of Bug #593585 +++

(In reply to Teemu Mannermaa (:wicked) from bug 593585 comment 27)
> [make check] fails with a "ImportError: No module named manifestparser" 
> root error because it can't find that Python module from the build
> directory. Error comes from build/Makefile.in line 124 that uses a
> relative srcdir to set include path, which ends up not matching due
> to pythonpath.py using execfile() to run the end script and that
> seems to change the cwd.

Sure enough, replacing "pythonpath.py -I" with "PYTHONPATH=" fixes the problem, as demonstrated by this patch.

I do not know why we need pythonpath.py at all. (I checked the commit that added it, but it had the wrong bug number, AFAICT, in the bug.) Seems like it isn't a good idea for performance, but I am sure there is a reason.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 568833 [details] [diff] [review]
Get "make check" working

Review of attachment 568833 [details] [diff] [review]:
-----------------------------------------------------------------

Seems weird, but ok.

Comment 3

[Rain [:rain1] (inactive)]

I thought we used pythonpath.py so that pythonpaths declared in multiple places would compose rather than overwriting each other.

Comment 4

[(not currently active) Ted Mielczarek]

Yeah, this doesn't make any sense to me. pythonpath.py ought to be doing the same thing here, maybe we broke it in some goofy way?

Comment 5

[Benjamin Smedberg]

We did pythonpath partly because people build with PYTHONPATH set up for nonstandard python setups and we were breaking them. But I don't understand the "isn't a good idea for performance" bit: we're only using one python process and pymake should be able to launch it directly (skipping the shell) unlike this version which requires an intermediate shell invocation.

execfile changes the cwd? If this is so, we should just take care to make `paths` in pythonpath use absolute paths instead of relative ones.

I don't like this change.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> we're only using one python
> process and pymake should be able to launch it directly (skipping the shell)
> unlike this version which requires an intermediate shell invocation.

I did not realize that pymake could not skip the exec() for the second invocation.

But, if we have native support for pythonpath in pymake, then I don't see how problems with changing the working directory during execfile() can affect pymake check.

> I don't like this change.

That's cool. I posted the patch primarily to help the build system team identity the problem.

I can't get "make check" to work at all on Windows without this patch and others are having this problem too. Is "make check" just totally busted on Windows (other than the build slaves)? If so, I think this should be closer to P1 than P5.

Comment 7

[Benjamin Smedberg]

What second invocation? pymake has no special smarts about pythonpath, I don't think. The point is that pythonpath doesn't exec() in the Linux sense: it just changes the sys.path environment and then runs the python script the same way the python interpreter would (using execfile). The problem is that we're adding a relative path to sys.path and then changing the CWD.

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Use absolute paths in pythonpath, v1

Comment 9

[Justin Lebar (not reading bugmail)]

Thanks for the fast review!

https://hg.mozilla.org/mozilla-central/rev/a05ecb395410