Fix None target type with Ninja build.

Fix None target type with Ninja build.

GYP doesn't produce a valid Ninja rule for 'none' target type.
These targets are used to make fake dependencies.

Here is a minimal GYP file to show the issue:

{
  'targets': [
    {
      'target_name': 'dummy',
      'type': 'none',
      'sources': [
        'test.h',
      ],
    },
  ],
}

There is a dummy.ninja file, but there is no rules named 'dummy'.
When using the 'msvs-ninja' build, MSVS complains about a missing target
and fails to compile the project.

Patch from etienneb@chromium.org.

BUG=
R=scottmg@chromium.org

Committed: https://code.google.com/p/gyp/source/detail?r=1883

[etienneb, 2014-03-19 19:41:39]

PTAL.

Could you launch the try bot.
And if it's ok, commit it.

[scottmg, 2014-03-19 19:46:56]

https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (left):

https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:1150: # TODO(evan): don't call this function for
'none' target types, as
i think it would be better to write the phony here, does that work ok?

[etienneb, 2014-03-19 19:53:34]

toughs?

https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (left):

https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:1150: # TODO(evan): don't call this function for
'none' target types, as
Nop, because none get filtered before by
 if link_deps or self.target.actions_stamp or actions_depends:

We may change the previous condition to accept 'none' and
hoist the code into the WriteTarget. That will works too.


On 2014/03/19 19:46:56, scottmg wrote:
> i think it would be better to write the phony here, does that work ok?

[scottmg, 2014-03-19 20:04:43]

https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (left):

https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:1150: # TODO(evan): don't call this function for
'none' target types, as
On 2014/03/19 19:53:34, etienneb wrote:
> Nop, because none get filtered before by
>  if link_deps or self.target.actions_stamp or actions_depends:
> 
> We may change the previous condition to accept 'none' and
> hoist the code into the WriteTarget. That will works too.

ok, i'm not sure what the best way to get to WriteTarget is, but i think the
phony should be in the subninja, rather than in the toplevel ninja anyway.

> 
> On 2014/03/19 19:46:56, scottmg wrote:
> > i think it would be better to write the phony here, does that work ok?
>

[etienneb, 2014-03-19 20:21:10]

Here is the second version like you may prefer it.
I do not have any preferences on it.

https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py
File pylib/gyp/generator/ninja.py (left):

https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py...
pylib/gyp/generator/ninja.py:1150: # TODO(evan): don't call this function for
'none' target types, as
I don't get this one.
It's inside the subninja.


On 2014/03/19 20:04:43, scottmg wrote:
> On 2014/03/19 19:53:34, etienneb wrote:
> > Nop, because none get filtered before by
> >  if link_deps or self.target.actions_stamp or actions_depends:
> > 
> > We may change the previous condition to accept 'none' and
> > hoist the code into the WriteTarget. That will works too.
> 
> ok, i'm not sure what the best way to get to WriteTarget is, but i think the
> phony should be in the subninja, rather than in the toplevel ninja anyway.
> 
> > 
> > On 2014/03/19 19:46:56, scottmg wrote:
> > > i think it would be better to write the phony here, does that work ok?
> > 
> 
>

[scottmg, 2014-03-19 20:46:27]

On 2014/03/19 20:21:10, etienneb wrote:
> Here is the second version like you may prefer it.
> I do not have any preferences on it.
> 
> https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py
> File pylib/gyp/generator/ninja.py (left):
> 
>
https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py...
> pylib/gyp/generator/ninja.py:1150: # TODO(evan): don't call this function for
> 'none' target types, as
> I don't get this one.
> It's inside the subninja.

Oh, I'm sorry, I thought that's why you were getting the abs_sources. ps#1 looks
better then, but just change it to be

      self.ninja.build(spec['target_name'], 'phony')

There's no need to depend on the unused 'sources' in a 'none'.

> 
> 
> On 2014/03/19 20:04:43, scottmg wrote:
> > On 2014/03/19 19:53:34, etienneb wrote:
> > > Nop, because none get filtered before by
> > >  if link_deps or self.target.actions_stamp or actions_depends:
> > > 
> > > We may change the previous condition to accept 'none' and
> > > hoist the code into the WriteTarget. That will works too.
> > 
> > ok, i'm not sure what the best way to get to WriteTarget is, but i think the
> > phony should be in the subninja, rather than in the toplevel ninja anyway.
> > 
> > > 
> > > On 2014/03/19 19:46:56, scottmg wrote:
> > > > i think it would be better to write the phony here, does that work ok?
> > > 
> > 
> >

[etienneb, 2014-03-20 14:15:04]

Revert to patch one.
And I disagree with removing the sources.

Here is the output with and without the dependencies:

D:\src\syzygy\src>ninja -C out\Default dummy
ninja: Entering directory `out\Default'
ninja: error: '..\..\test.h', needed by 'dummy', missing and no known rule to
make it

D:\src\syzygy\src>ninja -C out\Default dummy
ninja: Entering directory `out\Default'
ninja: no work to do.


It's supposed to be an error to have a missing file.
This is NOT validated gyp-generation time.

By looking at the "phony rules" at
  http://martine.github.io/ninja/manual.html.

"phony can also be used to create dummy targets for files which may not
 exist at build time. If a phony build statement is written without any
 dependencies, the target will be considered out of date if it does not
 exist. Without a phony build statement, Ninja will report an error if
 the file does not exist and is required by the build."

Thus, the sources may NOT exists at static time and can be generated,
BUT must be present when the rules is executed.


On 2014/03/19 20:46:27, scottmg wrote:
> On 2014/03/19 20:21:10, etienneb wrote:
> > Here is the second version like you may prefer it.
> > I do not have any preferences on it.
> > 
> >
https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py
> > File pylib/gyp/generator/ninja.py (left):
> > 
> >
>
https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py...
> > pylib/gyp/generator/ninja.py:1150: # TODO(evan): don't call this function
for
> > 'none' target types, as
> > I don't get this one.
> > It's inside the subninja.
> 
> Oh, I'm sorry, I thought that's why you were getting the abs_sources. ps#1
looks
> better then, but just change it to be
> 
>       self.ninja.build(spec['target_name'], 'phony')
> 
> There's no need to depend on the unused 'sources' in a 'none'.
> 
> > 
> > 
> > On 2014/03/19 20:04:43, scottmg wrote:
> > > On 2014/03/19 19:53:34, etienneb wrote:
> > > > Nop, because none get filtered before by
> > > >  if link_deps or self.target.actions_stamp or actions_depends:
> > > > 
> > > > We may change the previous condition to accept 'none' and
> > > > hoist the code into the WriteTarget. That will works too.
> > > 
> > > ok, i'm not sure what the best way to get to WriteTarget is, but i think
the
> > > phony should be in the subninja, rather than in the toplevel ninja anyway.
> > > 
> > > > 
> > > > On 2014/03/19 19:46:56, scottmg wrote:
> > > > > i think it would be better to write the phony here, does that work ok?
> > > > 
> > > 
> > >

[scottmg, 2014-03-20 17:07:16]

On 2014/03/20 14:15:04, etienneb wrote:
> Revert to patch one.
> And I disagree with removing the sources.
> 
> Here is the output with and without the dependencies:
> 
> D:\src\syzygy\src>ninja -C out\Default dummy
> ninja: Entering directory `out\Default'
> ninja: error: '..\..\test.h', needed by 'dummy', missing and no known rule to
> make it
> 
> D:\src\syzygy\src>ninja -C out\Default dummy
> ninja: Entering directory `out\Default'
> ninja: no work to do.
> 
> 
> It's supposed to be an error to have a missing file.
> This is NOT validated gyp-generation time.

It's my understanding that 'sources' doesn't have any meaning in a 'type':
'none' target. So, if you want it to error out somewhere, it should be during
parsing the gyp input on seeing 'sources', not when running ninja.

But, as there shouldn't be sources in there, having ninja generate dependencies
on them doesn't make sense.

> 
> By looking at the "phony rules" at
>   http://martine.github.io/ninja/manual.html.
> 
> "phony can also be used to create dummy targets for files which may not
>  exist at build time. If a phony build statement is written without any
>  dependencies, the target will be considered out of date if it does not
>  exist. Without a phony build statement, Ninja will report an error if
>  the file does not exist and is required by the build."
> 
> Thus, the sources may NOT exists at static time and can be generated,
> BUT must be present when the rules is executed.
> 
> 
> On 2014/03/19 20:46:27, scottmg wrote:
> > On 2014/03/19 20:21:10, etienneb wrote:
> > > Here is the second version like you may prefer it.
> > > I do not have any preferences on it.
> > > 
> > >
> https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py
> > > File pylib/gyp/generator/ninja.py (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py...
> > > pylib/gyp/generator/ninja.py:1150: # TODO(evan): don't call this function
> for
> > > 'none' target types, as
> > > I don't get this one.
> > > It's inside the subninja.
> > 
> > Oh, I'm sorry, I thought that's why you were getting the abs_sources. ps#1
> looks
> > better then, but just change it to be
> > 
> >       self.ninja.build(spec['target_name'], 'phony')
> > 
> > There's no need to depend on the unused 'sources' in a 'none'.
> > 
> > > 
> > > 
> > > On 2014/03/19 20:04:43, scottmg wrote:
> > > > On 2014/03/19 19:53:34, etienneb wrote:
> > > > > Nop, because none get filtered before by
> > > > >  if link_deps or self.target.actions_stamp or actions_depends:
> > > > > 
> > > > > We may change the previous condition to accept 'none' and
> > > > > hoist the code into the WriteTarget. That will works too.
> > > > 
> > > > ok, i'm not sure what the best way to get to WriteTarget is, but i think
> the
> > > > phony should be in the subninja, rather than in the toplevel ninja
anyway.
> > > > 
> > > > > 
> > > > > On 2014/03/19 19:46:56, scottmg wrote:
> > > > > > i think it would be better to write the phony here, does that work
ok?
> > > > > 
> > > > 
> > > >

[etienneb, 2014-03-20 17:18:38]

On 2014/03/20 17:07:16, scottmg wrote:
> On 2014/03/20 14:15:04, etienneb wrote:
> > Revert to patch one.
> > And I disagree with removing the sources.
> > 
> > Here is the output with and without the dependencies:
> > 
> > D:\src\syzygy\src>ninja -C out\Default dummy
> > ninja: Entering directory `out\Default'
> > ninja: error: '..\..\test.h', needed by 'dummy', missing and no known rule
to
> > make it
> > 
> > D:\src\syzygy\src>ninja -C out\Default dummy
> > ninja: Entering directory `out\Default'
> > ninja: no work to do.
> > 
> > 
> > It's supposed to be an error to have a missing file.
> > This is NOT validated gyp-generation time.
> 
> It's my understanding that 'sources' doesn't have any meaning in a 'type':
> 'none' target. So, if you want it to error out somewhere, it should be during
> parsing the gyp input on seeing 'sources', not when running ninja.
> 
> But, as there shouldn't be sources in there, having ninja generate
dependencies
> on them doesn't make sense.
> 
> > 
> > By looking at the "phony rules" at
> >   http://martine.github.io/ninja/manual.html.
> > 
> > "phony can also be used to create dummy targets for files which may not
> >  exist at build time. If a phony build statement is written without any
> >  dependencies, the target will be considered out of date if it does not
> >  exist. Without a phony build statement, Ninja will report an error if
> >  the file does not exist and is required by the build."
> > 
> > Thus, the sources may NOT exists at static time and can be generated,
> > BUT must be present when the rules is executed.
> > 
> > 
> > On 2014/03/19 20:46:27, scottmg wrote:
> > > On 2014/03/19 20:21:10, etienneb wrote:
> > > > Here is the second version like you may prefer it.
> > > > I do not have any preferences on it.
> > > > 
> > > >
> >
https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py
> > > > File pylib/gyp/generator/ninja.py (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py...
> > > > pylib/gyp/generator/ninja.py:1150: # TODO(evan): don't call this
function
> > for
> > > > 'none' target types, as
> > > > I don't get this one.
> > > > It's inside the subninja.
> > > 
> > > Oh, I'm sorry, I thought that's why you were getting the abs_sources. ps#1
> > looks
> > > better then, but just change it to be
> > > 
> > >       self.ninja.build(spec['target_name'], 'phony')
> > > 
> > > There's no need to depend on the unused 'sources' in a 'none'.
> > > 
> > > > 
> > > > 
> > > > On 2014/03/19 20:04:43, scottmg wrote:
> > > > > On 2014/03/19 19:53:34, etienneb wrote:
> > > > > > Nop, because none get filtered before by
> > > > > >  if link_deps or self.target.actions_stamp or actions_depends:
> > > > > > 
> > > > > > We may change the previous condition to accept 'none' and
> > > > > > hoist the code into the WriteTarget. That will works too.
> > > > > 
> > > > > ok, i'm not sure what the best way to get to WriteTarget is, but i
think
> > the
> > > > > phony should be in the subninja, rather than in the toplevel ninja
> anyway.
> > > > > 
> > > > > > 
> > > > > > On 2014/03/19 19:46:56, scottmg wrote:
> > > > > > > i think it would be better to write the phony here, does that work
> ok?
> > > > > > 
> > > > > 
> > > > >

In that case, the above example should give a gyp-time error because test.h
doesn't exists.
Which is not the case. Thus, the build silently ignore the missing input file at
gyp-time and at ninja-time.

[etienneb, 2014-03-20 18:09:43]

On 2014/03/20 17:18:38, etienneb wrote:
> On 2014/03/20 17:07:16, scottmg wrote:
> > On 2014/03/20 14:15:04, etienneb wrote:
> > > Revert to patch one.
> > > And I disagree with removing the sources.
> > > 
> > > Here is the output with and without the dependencies:
> > > 
> > > D:\src\syzygy\src>ninja -C out\Default dummy
> > > ninja: Entering directory `out\Default'
> > > ninja: error: '..\..\test.h', needed by 'dummy', missing and no known rule
> to
> > > make it
> > > 
> > > D:\src\syzygy\src>ninja -C out\Default dummy
> > > ninja: Entering directory `out\Default'
> > > ninja: no work to do.
> > > 
> > > 
> > > It's supposed to be an error to have a missing file.
> > > This is NOT validated gyp-generation time.
> > 
> > It's my understanding that 'sources' doesn't have any meaning in a 'type':
> > 'none' target. So, if you want it to error out somewhere, it should be
during
> > parsing the gyp input on seeing 'sources', not when running ninja.
> > 
> > But, as there shouldn't be sources in there, having ninja generate
> dependencies
> > on them doesn't make sense.
> > 
> > > 
> > > By looking at the "phony rules" at
> > >   http://martine.github.io/ninja/manual.html.
> > > 
> > > "phony can also be used to create dummy targets for files which may not
> > >  exist at build time. If a phony build statement is written without any
> > >  dependencies, the target will be considered out of date if it does not
> > >  exist. Without a phony build statement, Ninja will report an error if
> > >  the file does not exist and is required by the build."
> > > 
> > > Thus, the sources may NOT exists at static time and can be generated,
> > > BUT must be present when the rules is executed.
> > > 
> > > 
> > > On 2014/03/19 20:46:27, scottmg wrote:
> > > > On 2014/03/19 20:21:10, etienneb wrote:
> > > > > Here is the second version like you may prefer it.
> > > > > I do not have any preferences on it.
> > > > > 
> > > > >
> > >
> https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py
> > > > > File pylib/gyp/generator/ninja.py (left):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/203303011/diff/1/pylib/gyp/generator/ninja.py...
> > > > > pylib/gyp/generator/ninja.py:1150: # TODO(evan): don't call this
> function
> > > for
> > > > > 'none' target types, as
> > > > > I don't get this one.
> > > > > It's inside the subninja.
> > > > 
> > > > Oh, I'm sorry, I thought that's why you were getting the abs_sources.
ps#1
> > > looks
> > > > better then, but just change it to be
> > > > 
> > > >       self.ninja.build(spec['target_name'], 'phony')
> > > > 
> > > > There's no need to depend on the unused 'sources' in a 'none'.
> > > > 
> > > > > 
> > > > > 
> > > > > On 2014/03/19 20:04:43, scottmg wrote:
> > > > > > On 2014/03/19 19:53:34, etienneb wrote:
> > > > > > > Nop, because none get filtered before by
> > > > > > >  if link_deps or self.target.actions_stamp or actions_depends:
> > > > > > > 
> > > > > > > We may change the previous condition to accept 'none' and
> > > > > > > hoist the code into the WriteTarget. That will works too.
> > > > > > 
> > > > > > ok, i'm not sure what the best way to get to WriteTarget is, but i
> think
> > > the
> > > > > > phony should be in the subninja, rather than in the toplevel ninja
> > anyway.
> > > > > > 
> > > > > > > 
> > > > > > > On 2014/03/19 19:46:56, scottmg wrote:
> > > > > > > > i think it would be better to write the phony here, does that
work
> > ok?
> > > > > > > 
> > > > > > 
> > > > > >
> 
> In that case, the above example should give a gyp-time error because test.h
> doesn't exists.
> Which is not the case. Thus, the build silently ignore the missing input file
at
> gyp-time and at ninja-time.

As pointed out by Scott, msvs backend does not check the sources.
So, I move to the same semantic.

[scottmg, 2014-03-20 19:14:20]

otherwise, LGTM

https://codereview.chromium.org/203303011/diff/80001/test/ninja/none-rules/no...
File test/ninja/none-rules/none-rules.gyp (right):

https://codereview.chromium.org/203303011/diff/80001/test/ninja/none-rules/no...
test/ninja/none-rules/none-rules.gyp:10: 'sources': [ 'readme.cc' ],
remove this and the file i guess?

[etienneb, 2014-03-20 20:15:38]

done.

But, you need to launch try bots, and commit when it's green.
I'm not a committer.

https://codereview.chromium.org/203303011/diff/80001/test/ninja/none-rules/no...
File test/ninja/none-rules/none-rules.gyp (right):

https://codereview.chromium.org/203303011/diff/80001/test/ninja/none-rules/no...
test/ninja/none-rules/none-rules.gyp:10: 'sources': [ 'readme.cc' ],
On 2014/03/20 19:14:22, scottmg wrote:
> remove this and the file i guess?

Done.

[scottmg, 2014-03-25 01:00:12]

Committed patchset #10 manually as r1883 (presubmit successful).

[Torne, 2014-04-09 11:24:25]

The gyp-win32 bot has been failing two tests most of the time (but not 100% of
the time) since this CL landed, and the trybot results on the patch also show
the same failure. I really need to roll gyp into chromium for unrelated reasons,
so I'm going to revert this change and see if it fixes the win32 tests.

[Torne, 2014-04-09 11:24:52]

On 2014/04/09 11:24:25, Torne wrote:
> The gyp-win32 bot has been failing two tests most of the time (but not 100% of
> the time) since this CL landed, and the trybot results on the patch also show
> the same failure. I really need to roll gyp into chromium for unrelated
reasons,
> so I'm going to revert this change and see if it fixes the win32 tests.

Sorry, forgot to paste the failing tests:

Failed the following 2 tests:
	test\ninja\solibs_avoid_relinking\gyptest-solibs-avoid-relinking.py
	test\win\gyptest-link-restat-importlib.py