Closed
Bug 585012
Opened 14 years ago
Closed 8 years ago
Invoke Preprocessor.py as a pymake native command
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: ted, Assigned: rain1)
References
(Depends on 1 open bug)
Details
(Whiteboard: [pymake])
Attachments
(1 file, 1 obsolete file)
18.25 KB,
patch
|
ted
:
review-
|
Details | Diff | Splinter Review |
We should invoke Preprocessor.py as a pymake native command to avoid the overhead of spawning a new Python interpreter every time we run it.
Whiteboard: [pymake]
Assignee | ||
Comment 1•12 years ago
|
||
I have what I believe to be a working patch for this.
Assignee: nobody → sagarwal
Assignee | ||
Comment 2•12 years ago
|
||
1. Switch over Preprocessor.py to use -I instead of passing in the defines on the command line. That was because one of the defines was MALLOC_H=<malloc.h>, which contains the blacklisted < and > characters. 2. Define the autoconf include file (mozilla-config.h etc) in one spot and refer to that everywhere. 3. Since Preprocessor.py doesn't remove C comments from the autoconf include file and its output can be in a format where C comments aren't acceptable, use #if 0 blocks for comments instead. This is a pretty simple change. 4. Remove blank lines from autoconf include files. I've switched over the Preprocessor.py in rules.mk to a native command, and that should cover a significant number of invocations. The others can be switched over in a followup bug.
Attachment #654509 -
Flags: review?(ted.mielczarek)
Comment 3•12 years ago
|
||
I'd prefer you separate the pymake native command part and the use -I part. Then, instead of actually doing the -I part, I'd suggest moving config.status's function call into a small function, so that it can be imported, and have Preprocessor.py import the defines from config.status. It would need to know where config.status is, though.
Assignee | ||
Comment 4•12 years ago
|
||
That would be great, but it'd need us to reinvoke pymake using the virtualenv python every time we run it. Can we do this for now and start using ConfigStatus later?
Comment 5•12 years ago
|
||
(In reply to Siddharth Agarwal [:sid0] from comment #4) > That would be great, but it'd need us to reinvoke pymake using the > virtualenv python every time we run it. Can we do this for now and start > using ConfigStatus later? Note it's config.status that's needed, not ConfigStatus.
Reporter | ||
Comment 6•12 years ago
|
||
Comment on attachment 654509 [details] [diff] [review] patch v1 Review of attachment 654509 [details] [diff] [review]: ----------------------------------------------------------------- Apparently this patch needs to be rewritten after bug 785871. ::: config/Preprocessor.py @@ +204,5 @@ > metavar="VAR[=VAL]", help='Define a variable') > p.add_option('-U', action='callback', callback=handleU, type="string", > metavar="VAR", help='Undefine a variable') > p.add_option('-F', action='callback', callback=handleF, type="string", > + metavar="FILTER", help=' Enable the specified filter') Is this an intentional change?
Attachment #654509 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 7•12 years ago
|
||
No, that wasn't intentional.
Assignee | ||
Comment 8•12 years ago
|
||
Rewritten to use config.status instead. glandium, could you make sure I have the semantics correct?
Attachment #654509 -
Attachment is obsolete: true
Attachment #663785 -
Flags: review?(ted.mielczarek)
Attachment #663785 -
Flags: feedback?(mh+mozilla)
Reporter | ||
Comment 9•12 years ago
|
||
Comment on attachment 663785 [details] [diff] [review] patch v2 Review of attachment 663785 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/Preprocessor.py @@ +169,5 @@ > pass > > + def loadConfigStatus(self, path): > + with open(path) as f: > + config = imp.load_module('config', f, path, ('', 'r', imp.PY_SOURCE)) I thought bug 785871 made this unnecessary. Shouldn't you be able to just import buildconfig ? ::: config/tests/unit-Preprocessor.py @@ +608,5 @@ > +#endif > +""") > + self.pp.loadConfigStatus("config.status") > + self.pp.do_include(f) > + self.assertEqual(self.pp.out.getvalue(), nit: trailing whitespace
Attachment #663785 -
Flags: review?(ted.mielczarek) → review-
Comment 10•12 years ago
|
||
(In reply to Ted Mielczarek [:ted] from comment #9) > Comment on attachment 663785 [details] [diff] [review] > patch v2 > > Review of attachment 663785 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: config/Preprocessor.py > @@ +169,5 @@ > > pass > > > > + def loadConfigStatus(self, path): > > + with open(path) as f: > > + config = imp.load_module('config', f, path, ('', 'r', imp.PY_SOURCE)) > > I thought bug 785871 made this unnecessary. Shouldn't you be able to just > import buildconfig ? Unfortunately, not in this specific case, because Preprocessor.py is also used in js/src, and there is no virtualenv there.
Assignee | ||
Comment 11•12 years ago
|
||
That's one reason. The other is that we (still) don't run pymake from the virtualenv.
Comment 12•12 years ago
|
||
(In reply to Siddharth Agarwal [:sid0] from comment #11) > That's one reason. The other is that we (still) don't run pymake from the > virtualenv. I added pymake to the virtualenv today. However, as long as users invoke client.mk from the command-line, there's still a chance we're not in the virtualenv. Of course, if mach ever replaces client.mk, it should be much, much easier to inject the virtualenv into the existing process (build out virtualenv if missing and adjust sys.path). The only piece you need to worry about is Python binary mismatch. Even then you could compare the current Python binary with that in the virtualenv and ensure they are identical, spawning off a new process with the proper Python if they aren't.
Updated•12 years ago
|
Attachment #663785 -
Flags: feedback?(mh+mozilla)
Reporter | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•