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

RESOLVED FIXED

Status

()

Core
Build Config
P5
major
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: briansmith, Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox11 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 568833 [details] [diff] [review]
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.
Attachment #568833 - Flags: review?(khuey)
Attachment #568833 - Flags: feedback?(benjamin)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 696214
Comment on attachment 568833 [details] [diff] [review]
Get "make check" working

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

Seems weird, but ok.
Attachment #568833 - Flags: review?(khuey) → review+
I thought we used pythonpath.py so that pythonpaths declared in multiple places would compose rather than overwriting each other.
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?
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.
Attachment #568833 - Flags: feedback?(benjamin) → feedback-
(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.
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.
(Assignee)

Comment 8

6 years ago
Created attachment 581012 [details] [diff] [review]
Use absolute paths in pythonpath, v1
Attachment #581012 - Flags: review?(benjamin)
Attachment #581012 - Flags: review?(benjamin) → review+
(Assignee)

Comment 9

6 years ago
Thanks for the fast review!

https://hg.mozilla.org/mozilla-central/rev/a05ecb395410
Assignee: nobody → justin.lebar+bug
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox11: --- → fixed
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Attachment #568833 - Attachment is obsolete: true
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.