Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1059)

Issue 7034007: Teach gyp about configurable filter commands, add "pymod_do_main" filter (Closed)

Created:
9 years, 7 months ago by Nico
Modified:
9 years, 7 months ago
CC:
gyp-developer_googlegroups.com
Visibility:
Public.

Description

Teach gyp about configurable filter commands, add "pymod_do_main" filter The syntax for gyp commands was "<(command)", which will execute "command" in a shell. Now it's "<filter(command)", and if filter is omitted command is run in a shell. Else, it depends on "filter" what happens. Add the filter "pymod_do_main", which loads the python module that's the first part of "command", passes the rest of "command" to that module's DoMain() function, and then uses the result of this function. That's roughly equivalent to `<(python module.py parameters)`, but can be a lot faster for modules that have nontrivial startup time (because they are now imported only once by gyp). This speeds up `build/gyp_chromium` by 8s on my system (with the change to gyp_chromium in http://codereview.chromium.org/7035004). BUG=http://crbug.com/82230 Comitted: http://code.google.com/p/gyp/source/detail?r=924

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 9

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : '' #

Total comments: 8

Patch Set 8 : '' #

Total comments: 2

Patch Set 9 : '' #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -22 lines) Patch
M pylib/gyp/input.py View 1 2 3 4 5 6 7 8 4 chunks +55 lines, -22 lines 5 comments Download

Messages

Total messages: 21 (0 generated)
Nico
Not very general / pretty, but very effective.
9 years, 7 months ago (2011-05-16 04:18:08 UTC) #1
tony
I defer the actual review to Mark. I'm not sure if we want a grit/chromium ...
9 years, 7 months ago (2011-05-16 16:24:05 UTC) #2
bradn
Hmmn, given how common these are, I'd expect a big win. How much speedup does ...
9 years, 7 months ago (2011-05-16 16:35:50 UTC) #3
Nico
On 2011/05/16 16:35:50, bradn wrote: > Hmmn, given how common these are, I'd expect a ...
9 years, 7 months ago (2011-05-16 16:39:28 UTC) #4
Mark Mentovai
http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/2001/pylib/gyp/input.py#newcode26 pylib/gyp/input.py:26: import grit_info # TODO: make optional What does “TODO: ...
9 years, 7 months ago (2011-05-16 16:51:34 UTC) #5
Mark Mentovai
Oh, yeah, once it’s refactored I’m probably going to say something like “can you make ...
9 years, 7 months ago (2011-05-16 17:06:16 UTC) #6
Evan Martin
Hacky hack is hacky. How about an alternative implementation: extend gyp syntax that currently lets ...
9 years, 7 months ago (2011-05-16 19:37:06 UTC) #7
Nico
On 2011/05/16 19:37:06, Evan Martin wrote: > Hacky hack is hacky. > > How about ...
9 years, 7 months ago (2011-05-16 23:05:27 UTC) #8
Evan Martin
On 2011/05/16 23:05:27, Nico wrote: > if cmd[0] == 'python': > module = __import__(cmd[1].rstrip('.py')) > ...
9 years, 7 months ago (2011-05-16 23:07:24 UTC) #9
tony
On 2011/05/16 23:07:24, Evan Martin wrote: > On 2011/05/16 23:05:27, Nico wrote: > > if ...
9 years, 7 months ago (2011-05-16 23:09:55 UTC) #10
Nico
Less hacky, more complicated patch is up. I updated the CL description to describe what ...
9 years, 7 months ago (2011-05-24 19:43:56 UTC) #11
Mark Mentovai
http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode539 pylib/gyp/input.py:539: # command string. Currently, only 'python' is supported. Except ...
9 years, 7 months ago (2011-05-24 21:54:57 UTC) #12
Nico
http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode539 pylib/gyp/input.py:539: # command string. Currently, only 'python' is supported. On ...
9 years, 7 months ago (2011-05-24 22:02:18 UTC) #13
Mark Mentovai
http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode654 pylib/gyp/input.py:654: # passing "param eters" as a single string argument. ...
9 years, 7 months ago (2011-05-24 22:04:16 UTC) #14
Mark Mentovai
http://codereview.chromium.org/7034007/diff/14001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/14001/pylib/gyp/input.py#newcode667 pylib/gyp/input.py:667: raise Exception("Unknown command string '%s' in action '%s'." % ...
9 years, 7 months ago (2011-05-24 22:06:25 UTC) #15
Nico
http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/12001/pylib/gyp/input.py#newcode654 pylib/gyp/input.py:654: # passing "param eters" as a single string argument. ...
9 years, 7 months ago (2011-05-24 22:09:52 UTC) #16
Mark Mentovai
LGTM. You may want to wait for the others to chime in too.
9 years, 7 months ago (2011-05-24 22:24:10 UTC) #17
tony
LGTM2 http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py#newcode22 pylib/gyp/input.py:22: import shlex Nit: alphabetize http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py#newcode678 pylib/gyp/input.py:678: (p_stdout, p_stderr) ...
9 years, 7 months ago (2011-05-24 23:31:39 UTC) #18
TVL
http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py#newcode651 pylib/gyp/input.py:651: if command_string == 'pymod_do_main': drive by - should we ...
9 years, 7 months ago (2011-05-24 23:44:03 UTC) #19
Nico
Thanks! Mark, can you land this? http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py File pylib/gyp/input.py (right): http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py#newcode651 pylib/gyp/input.py:651: if command_string == ...
9 years, 7 months ago (2011-05-25 00:41:26 UTC) #20
Mark Mentovai
9 years, 7 months ago (2011-05-25 02:12:34 UTC) #21
http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py
File pylib/gyp/input.py (right):

http://codereview.chromium.org/7034007/diff/15002/pylib/gyp/input.py#newcode20
pylib/gyp/input.py:20: import shlex
shlex was already in here. LGTM for a quick follow-up to remove the duplicate.

Powered by Google App Engine
This is Rietveld 408576698