Last Comment Bug 696535 - "make check" and "pymake check" fail because build/'s own "make check" fails to find manifestparser
: "make check" and "pymake check" fail because build/'s own "make check" fails ...
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Windows 7
: P5 major (vote)
: ---
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
: 696214 (view as bug list)
Depends on:
Blocks: 593585
  Show dependency treegraph
 
Reported: 2011-10-21 17:51 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2012-02-01 14:12 PST (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Get "make check" working (801 bytes, patch)
2011-10-21 17:51 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
khuey: review+
benjamin: feedback-
Details | Diff | Splinter Review
Use absolute paths in pythonpath, v1 (2.06 KB, patch)
2011-12-12 12:34 PST, Justin Lebar (not reading bugmail)
benjamin: review+
Details | Diff | Splinter Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-21 17:51:11 PDT
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.
Comment 1 Justin Lebar (not reading bugmail) 2011-10-22 07:37:50 PDT
*** Bug 696214 has been marked as a duplicate of this bug. ***
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-24 05:08:38 PDT
Comment on attachment 568833 [details] [diff] [review]
Get "make check" working

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

Seems weird, but ok.
Comment 3 Siddharth Agarwal [:sid0] (inactive) 2011-10-24 05:13:18 PDT
I thought we used pythonpath.py so that pythonpaths declared in multiple places would compose rather than overwriting each other.
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-10-24 08:01:29 PDT
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 [:bsmedberg] 2011-10-24 08:07:32 PDT
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?) 2011-10-26 09:10:39 PDT
(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 [:bsmedberg] 2011-10-26 09:18:36 PDT
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) 2011-12-12 12:34:34 PST
Created attachment 581012 [details] [diff] [review]
Use absolute paths in pythonpath, v1
Comment 9 Justin Lebar (not reading bugmail) 2011-12-12 13:32:28 PST
Thanks for the fast review!

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

Note You need to log in before you can comment on or make changes to this bug.