ninja: land generator for ninja files

ninja: land generator for ninja files

This generator already works for Chrome and is used by a number of
people, but it still needs some work before it could be considered
fully functional.

Ninja is semantically very similar to make.  My plan is to massage
make.py and ninja.py into similar shapes and eventually merge them.

Committed: http://code.google.com/p/gyp/source/detail?r=1009

[Evan Martin, 2011-08-19 18:12:21]

i think this is ready to go.

[Nico, 2011-08-19 18:21:25]

Only looked at the test files so far.

You probably want to add ninja to these 2 lines in ./gyptest.py:

      'linux2':   ['make'],
      'linux3':   ['make'],

Once this landed, you also want to ping bradnelson and ask him to add the ninja
tests to go/gypbot on linux.

http://codereview.chromium.org/7696003/diff/2001/test/actions/gyptest-all.py
File test/actions/gyptest-all.py (right):

http://codereview.chromium.org/7696003/diff/2001/test/actions/gyptest-all.py#...
test/actions/gyptest-all.py:22: # as a means to making something run on every
build.  That makes some pretty
Remove 2nd sentence.

http://codereview.chromium.org/7696003/diff/2001/test/copies-link/gyptest-cop...
File test/copies-link/gyptest-copies-link.py (right):

http://codereview.chromium.org/7696003/diff/2001/test/copies-link/gyptest-cop...
test/copies-link/gyptest-copies-link.py:13: # This test doesn't actually test
what it thinks it's testing.  It copies a
Hm, fix the test instead? Or remove it? (in a different cl)

http://codereview.chromium.org/7696003/diff/2001/test/generator-output/gyptes...
File test/generator-output/gyptest-actions.py (right):

http://codereview.chromium.org/7696003/diff/2001/test/generator-output/gyptes...
test/generator-output/gyptest-actions.py:13: test =
TestGyp.TestGyp(formats=['!ninja'])
Why !ninja here?

http://codereview.chromium.org/7696003/diff/2001/test/generator-output/gyptes...
File test/generator-output/gyptest-copies.py (right):

http://codereview.chromium.org/7696003/diff/2001/test/generator-output/gyptes...
test/generator-output/gyptest-copies.py:13: test =
TestGyp.TestGyp(formats=['!ninja'])
same question

http://codereview.chromium.org/7696003/diff/2001/test/generator-output/gyptes...
File test/generator-output/gyptest-relocate.py (right):

http://codereview.chromium.org/7696003/diff/2001/test/generator-output/gyptes...
test/generator-output/gyptest-relocate.py:14: test =
TestGyp.TestGyp(formats=['!ninja'])
…you get the idea

http://codereview.chromium.org/7696003/diff/2001/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

http://codereview.chromium.org/7696003/diff/2001/test/lib/TestGyp.py#newcode475
test/lib/TestGyp.py:475: # XXX due to phony rules, we always think we have work
to do.
XXX?

[Evan Martin, 2011-08-19 18:37:13]

(other comments addressed, PTAL)

http://codereview.chromium.org/7696003/diff/2001/test/actions/gyptest-all.py
File test/actions/gyptest-all.py (right):

http://codereview.chromium.org/7696003/diff/2001/test/actions/gyptest-all.py#...
test/actions/gyptest-all.py:22: # as a means to making something run on every
build.  That makes some pretty
On 2011/08/19 18:21:25, Nico wrote:
> Remove 2nd sentence.

Hm, do you not like the wording, or what it is saying?  It is the note I wrote
to myself about why it is hard to make this test work (I spent some time
trying).

http://codereview.chromium.org/7696003/diff/2001/test/copies-link/gyptest-cop...
File test/copies-link/gyptest-copies-link.py (right):

http://codereview.chromium.org/7696003/diff/2001/test/copies-link/gyptest-cop...
test/copies-link/gyptest-copies-link.py:13: # This test doesn't actually test
what it thinks it's testing.  It copies a
On 2011/08/19 18:21:25, Nico wrote:
> Hm, fix the test instead? Or remove it? (in a different cl)

Yeah.  Do you think I should do it before I finish this patch?  It's kind of an
orthogonal issue.  (You might have noticed I've been fixing similar issues in
other gyp tests lately -- this one is harder to fix.)

[Nico, 2011-08-19 19:07:48]

not done with WriteRules and read only halfway through writetarget, but lunch
calls

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:3: # Copyright (c) 2010 Google Inc. All rights
reserved.
2011

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:57: command = $cc -MMD -MF $out.d $defines
$includes $cflags $cflags_cc $
I think the inconsistency of calling the flags cflags_c is better than the
potentially confusion caused by cflags_cc not being the gyp ccflags

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:127: #   into the equivalent ninja path.
Why not NinjaFromGypPath() and NinjaOutputFromGypPath()? That seems easier to
read and understand to me.

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:144: See the above discourse on path
conversions."""
nit: It's a bit weird to have a docstring refer to a comment, since docstrings
show up in help() in the repl, but comments don't. I can't think of a better
way, though.

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:162: # reuse any global directory.
assert not
generator_default_variables['INTERMEDIATE_DIR'].startswith(generator_default_variables['SHARED_INTERMEDIATE_DIR'])?

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:169: path_dir, path_filename = os.path.split(path)
Usually, filename = dirname + basename, so I'd call this path_basename

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:175: def StampPath(self, name):
(If you rename the two functions above, this would become NinjaStampPath())

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:201: prebuild_deps = [x for x, _ in deps if x]
Is this the same as `filter(None, deps.keys())`? If so, I think that's clearer

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:225: final_deps = link_deps or sources_predepends
or prebuild
`sources_predepends or prebuild` is repeated here might want to put that into a
variable (yes, it's an extra line and not as "nice", but it makes it impossible
for someone to change just one of the two lines saying 'sources_predepends or
prebuild' now)

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:341: filename = os.path.split(path)[1]
(if you say 'basename' above, do so here too)

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:371: # if ext in ('h', 'hxx'):
is this a todo? something else?

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:393: #   file and writes a stamp file): add them to
implicit_deps
nit break: before the "and" instead of before the "file" :-P

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/ninja_syntax.py
File pylib/gyp/ninja_syntax.py (right):

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/ninja_syntax.py#ne...
pylib/gyp/ninja_syntax.py:17: def __init__(self, output, width=78):
78 :-)

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/ninja_syntax.py#ne...
pylib/gyp/ninja_syntax.py:25: for line in textwrap.wrap(text, self.width - 2):
s/2/len('# ')/?

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/ninja_syntax.py#ne...
pylib/gyp/ninja_syntax.py:97: # We only have one special metacharacter: '$'.
:-)

[Evan Martin, 2011-08-19 20:26:43]

PTAL, I think I addressed all your feedback by either doing your suggestion or
arguing with you below ;)

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:3: # Copyright (c) 2010 Google Inc. All rights
reserved.
On 2011/08/19 19:07:48, Nico wrote:
> 2011

Heh, shows how long I've been sitting on this patch.  :(

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:127: #   into the equivalent ninja path.
On 2011/08/19 19:07:48, Nico wrote:
> Why not NinjaFromGypPath() and NinjaOutputFromGypPath()? That seems easier to
> read and understand to me.

I renamed them GypPathToNinja and GypPathToUniqueOutuput (I like the symmetry of
the shared prefix, since they both take the same input form)

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:175: def StampPath(self, name):
On 2011/08/19 19:07:48, Nico wrote:
> (If you rename the two functions above, this would become NinjaStampPath())

I renamed the other functions, but since stamps never occur except in a
build-path context (there is no translation from some equivalent gyp concept) I
think this name is ok.

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:201: prebuild_deps = [x for x, _ in deps if x]
On 2011/08/19 19:07:48, Nico wrote:
> Is this the same as `filter(None, deps.keys())`? If so, I think that's clearer

Unfortunately, deps is a list of tuples.

I rewrote it to be less functional.

http://codereview.chromium.org/7696003/diff/2001/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:225: final_deps = link_deps or sources_predepends
or prebuild
On 2011/08/19 19:07:48, Nico wrote:
> `sources_predepends or prebuild` is repeated here might want to put that into
a
> variable (yes, it's an extra line and not as "nice", but it makes it
impossible
> for someone to change just one of the two lines saying 'sources_predepends or
> prebuild' now)

I think I had originally used a variable here and switched to this because I
couldn't think of a good name for the variable.  It's conceptually something
like "the last thing we did so far".

[Nico, 2011-08-19 22:45:10]

LGTM

All I ever used ninja for is to build ninja, and I've only read but not written
ninja files, so it's not the most qualified LGTM in the world.

http://codereview.chromium.org/7696003/diff/2001/test/actions/gyptest-all.py
File test/actions/gyptest-all.py (right):

http://codereview.chromium.org/7696003/diff/2001/test/actions/gyptest-all.py#...
test/actions/gyptest-all.py:22: # as a means to making something run on every
build.  That makes some pretty
On 2011/08/19 18:37:13, Evan Martin wrote:
> On 2011/08/19 18:21:25, Nico wrote:
> > Remove 2nd sentence.
> 
> Hm, do you not like the wording, or what it is saying?  It is the note I wrote
> to myself about why it is hard to make this test work (I spent some time
> trying).

It didn't add any signal to the comment for me, just made it longer. If it helps
you, keep it.

http://codereview.chromium.org/7696003/diff/2001/test/copies-link/gyptest-cop...
File test/copies-link/gyptest-copies-link.py (right):

http://codereview.chromium.org/7696003/diff/2001/test/copies-link/gyptest-cop...
test/copies-link/gyptest-copies-link.py:13: # This test doesn't actually test
what it thinks it's testing.  It copies a
On 2011/08/19 18:37:13, Evan Martin wrote:
> On 2011/08/19 18:21:25, Nico wrote:
> > Hm, fix the test instead? Or remove it? (in a different cl)
> 
> Yeah.  Do you think I should do it before I finish this patch?  It's kind of
an
> orthogonal issue.  (You might have noticed I've been fixing similar issues in
> other gyp tests lately -- this one is harder to fix.)

I'd do this first, but the other way round is fine, too.

http://codereview.chromium.org/7696003/diff/78/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (right):

http://codereview.chromium.org/7696003/diff/78/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:68: command = rm -f $out && ar rcsT $out $in
Should you `assert system_test.ArSupportsThinArchives(), 'ninja needs an ar that
supports thin archives'` somewhere early?

http://codereview.chromium.org/7696003/diff/78/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:163: assert not
generator_default_variables['SHARED_INTERMEDIATE_DIR'].startswith(
80 cols

http://codereview.chromium.org/7696003/diff/78/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:412: extra_deps.add(input)
dpranke will not appreciate this

http://codereview.chromium.org/7696003/diff/78/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:414: # XXX Chrome-specific HACK.  Chrome runs this
lastchange rule on
s/XXX/TODO/

http://codereview.chromium.org/7696003/diff/78/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:480: target = StripPrefix(target, 'lib')
Remark: In this file there are several "if spec['type'] == ... : ..." code
snippets. I think it might be cleaner to have a Product class that has prefix(),
suffix(), fullname() etc methods and a FromType() static factory method; that
would make the writer smaller. (Kind of like I'm trying to get all the mac
bundle logic into one class, but now that class is intermingled with xcode
setting management). Not something for this CL, but maybe something to keep in
mind for both this and the make generator to make things simpler in the future.

http://codereview.chromium.org/7696003/diff/78/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:499: print 'pdir', path
Are you intentionally writing to stdout here? (probably not)

http://codereview.chromium.org/7696003/diff/78/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:524: # should be scoped to the subninja.
I like the word "subninja"

http://codereview.chromium.org/7696003/diff/78/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:538: args[i] = arg
This could be in a separate function maybe

http://codereview.chromium.org/7696003/diff/78/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:571: if config_name is None:
and len(target_list) > 0? :-P (maybe that's handled by gyp higher up, didn't
check)