585012 - Invoke Preprocessor.py as a pymake native command

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

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.

Comment 1

[Rain [:rain1] (inactive)]

I have what I believe to be a working patch for this.

Comment 2

[Rain [:rain1] (inactive)]

Attachment: 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.

Comment 3

[Mike Hommey [:glandium]]

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.

Comment 4

[Rain [:rain1] (inactive)]

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

[Mike Hommey [:glandium]]

(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 6

[(not currently active) Ted Mielczarek]

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?

Comment 7

[Rain [:rain1] (inactive)]

No, that wasn't intentional.

Comment 8

[Rain [:rain1] (inactive)]

Attachment: patch v2

Rewritten to use config.status instead. glandium, could you make sure I have the semantics correct?

Comment 9

[(not currently active) Ted Mielczarek]

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

Comment 10

[Mike Hommey [:glandium]]

(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.

Comment 11

[Rain [:rain1] (inactive)]

That's one reason. The other is that we (still) don't run pymake from the virtualenv.

Comment 12

[Gregory Szorc [:gps]]

(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.