Introduce the 'link_dependency' attribute that removes the targets having this

Introduce the 'link_dependency' attribute that removes the targets having this
attribute from the 'none' targets' dependencies.

The intention behind this change is to let the users have static_library or
shared_library targets on which _every_ other executable/shared_library target
must depend, without introducing excessive dependencies on the former ones from
the 'none' metatargets (such excessive dependencies may trigger generator bugs
like https://code.google.com/p/gyp/issues/detail?id=409).

GYP r1860

[Alexander Potapenko, 2014-02-27 11:40:08]

Hi Mark, Brad,

this is another shot at https://code.google.com/p/gyp/issues/detail?id=380,
because the current solution triggers something that I considered a bug in the
ninja generator (https://code.google.com/p/gyp/issues/detail?id=409)

Because we can't conditionally add a dependency for _type=='executable' (and
'shared_library'), the idea is to add that dependency to every target (which is
done now) and remove that dependency if it has the 'link_dependency' attribute
for the 'none' targets.

[Mark Mentovai, 2014-02-27 16:48:30]

Why this instead of fixing the bug in the ninja generator? If the ninja
generator were fixed, would your use case function correctly without having to
add more stuff to the .gyp files?

[Alexander Potapenko, 2014-02-27 17:06:43]

On 2014/02/27 16:48:30, Mark Mentovai wrote:
> Why this instead of fixing the bug in the ninja generator? If the ninja
> generator were fixed, would your use case function correctly without having to
> add more stuff to the .gyp files?

To some extent, yes, but I also remember problems with clossplatform compilation
(http://crbug.com/317670) that appeared after the 'none' targets started
depending on additional libraries from target_defaults.
Anyway, I think this solution reduces the number of unnecessary dependencies.

I'll give the ninja generator fix another try tomorrow.

[Mark Mentovai, 2014-02-27 17:13:24]

The hypothesis on bug 317670 is a little speculative. I’ll put this on ice ’til
you look at the ninja generator bug. If we can get that fixed, we’ll be in a
better position to figure out what’s actually happening in bug 317670.

[Alexander Potapenko, 2014-03-04 12:14:33]

On 2014/02/27 17:13:24, Mark Mentovai wrote:
> The hypothesis on bug 317670 is a little speculative. I’ll put this on ice
’til
> you look at the ninja generator bug. If we can get that fixed, we’ll be in a
> better position to figure out what’s actually happening in bug 317670.


(see https://code.google.com/p/gyp/issues/detail?id=409#c3)
Looks like this isn't a bug in the generator, because there're conflicting
target names in the GYP after the variables substitution.
So we need some attribute to work around this situation.

[Mark Mentovai, 2014-03-04 14:02:20]

OK. In that case, can you write a test for the new link_dependency behavior?

[Alexander Potapenko, 2014-03-04 14:55:27]

On 2014/03/04 14:02:20, Mark Mentovai wrote:
> OK. In that case, can you write a test for the new link_dependency behavior?

Done, PTAL
I've checked that the test fails without the link_dependency=1 attribute.
Looks like gyptest.py defaults to using ninja, but I'm not sure this test will
detect a regression in the case another generator is used.

[Mark Mentovai, 2014-03-04 15:00:10]

LGTM

[Alexander Potapenko, 2014-03-04 15:19:53]

On 2014/03/04 15:00:10, Mark Mentovai wrote:
> LGTM

Can you please land this CL when you get a chance?

[Mark Mentovai, 2014-03-04 15:25:24]

Sure. I committed this as GYP r1860.

[Nico, 2014-03-07 22:36:09]

It looks like this broke tests on various bots:
http://build.chromium.org/f/client/gyp/

[Nico, 2014-03-10 17:35:57]

On 2014/03/07 22:36:09, Nico wrote:
> It looks like this broke tests on various bots:
> http://build.chromium.org/f/client/gyp/

Any progress? If this is difficult to fix, maybe we should revert this for now?

[Alexander Potapenko, 2014-03-11 09:10:13]

On 2014/03/10 17:35:57, Nico wrote:
> On 2014/03/07 22:36:09, Nico wrote:
> > It looks like this broke tests on various bots:
> > http://build.chromium.org/f/client/gyp/
> 
> Any progress? If this is difficult to fix, maybe we should revert this for
now?

I don't quite understand why ninja is being invoked for TESTGYP_FORMAT=cmake.
Anyway, is it ok to just disable the test everywhere except ninja?

Index: test/link-dependency/gyptest-link-dependency.py
===================================================================
--- test/link-dependency/gyptest-link-dependency.py	(revision 1860)
+++ test/link-dependency/gyptest-link-dependency.py	(working copy)
@@ -12,7 +12,7 @@
 
 import TestGyp
 
-test = TestGyp.TestGyp()
+test = TestGyp.TestGyp(formats=['ninja'])
 
 test.run_gyp('test.gyp')
 test.build('test.gyp', 'main')

[Nico, 2014-03-11 16:33:18]

On 2014/03/11 09:10:13, Alexander Potapenko wrote:
> On 2014/03/10 17:35:57, Nico wrote:
> > On 2014/03/07 22:36:09, Nico wrote:
> > > It looks like this broke tests on various bots:
> > > http://build.chromium.org/f/client/gyp/
> > 
> > Any progress? If this is difficult to fix, maybe we should revert this for
> now?
> 
> I don't quite understand why ninja is being invoked for TESTGYP_FORMAT=cmake.
> Anyway, is it ok to just disable the test everywhere except ninja?

It's not a generator-specific patch, is it? The CL description doesn't say that
this only works for ninja either.

Why does this not pass for non-ninja? How hard is it to make it pass? (It seems
to pass with make and xcode already?)

> 
> Index: test/link-dependency/gyptest-link-dependency.py
> ===================================================================
> --- test/link-dependency/gyptest-link-dependency.py	(revision 1860)
> +++ test/link-dependency/gyptest-link-dependency.py	(working copy)
> @@ -12,7 +12,7 @@
>  
>  import TestGyp
>  
> -test = TestGyp.TestGyp()
> +test = TestGyp.TestGyp(formats=['ninja'])
>  
>  test.run_gyp('test.gyp')
>  test.build('test.gyp', 'main')

[Alexander Potapenko, 2014-03-11 17:21:09]

First GYP generates a CMakeLists.txt file which contains the following targets:
 'malloc' - for libmalloc.so
 'main_initial' - for the binary called 'main' depending on libmalloc.so
 'main' - for the meta-target depending on main_initial (and correctly not
depending on libmalloc.so because we've set the link_dependency attribute)

The patch in question isn't generator-specific, that's why it works for other
output formats, and even produces the correct CMakeLists.txt file.
But then CMake generates a build.ninja file for this config, and the presence of
a target called 'main' and the binary called 'main' blows CMake's mind - it just
tries to create two targets with a similar name.

I suspect this is a problem with CMake's Ninja generator being a bit too
straightforward.
GYP's Ninja generator is smart enough to not generate the phony target for the
dummy meta-target if it doesn't have other dependencies, but that's not the case
with CMake.
Ninja stops reporting the error if I remove the phony target.

This situation (as well as https://code.google.com/p/gyp/issues/detail?id=409)
happens because GYP allows users (e.g. Chrome) to have a binary target foo that
produces a binary bar, while there's another target named bar. Various generator
under various circumstances (e.g. when other dependencies are added) may create
rules named 'bar' for both targets.
Because doing so isn't explicitly prohibited, each generator will have to handle
this situation on their own (e.g. mangle the names for phony targets
differently).

Because my patch neither adds the restriction to GYP nor fixes the existing
problems in the CMake generator, but just works around some common case where
the error is possible in the non-CMake land, I don't think we should run the
test under CMake.