Invoke Preprocessor.py as a pymake native command

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
9 years ago
10 months ago

People

(Reporter: ted, Assigned: sid0)

Tracking

(Depends on: 1 bug)

Trunk
x86
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [pymake])

Attachments

(1 attachment, 1 obsolete attachment)

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.
I have what I believe to be a working patch for this.
Assignee: nobody → sagarwal
Depends on: 784910
Created attachment 654509 [details] [diff] [review]
patch v1

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)
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.
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?
(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.
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)
No, that wasn't intentional.
Depends on: 785871
Created attachment 663785 [details] [diff] [review]
patch v2

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)
Depends on: 793477
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-
(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.
That's one reason. The other is that we (still) don't run pymake from the virtualenv.
(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.
Attachment #663785 - Flags: feedback?(mh+mozilla)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX

Updated

10 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.