Fix make and ninja backends to sensibly handle duplicate target names in different directories

Fix make and ninja backends to sensibly handle duplicate target names in different directories 

Currently, if two multiple targets in different directories use the same name, 
gyp's behaviour depends on the particular backend and is confusing at best. See 
bug for details. 

This change fixes the make and ninja backends to ... 
- always build all targets when duplicate targets exist 
- use the correct action/rule/copy when such duplicate targets define 
actions/rules/copies with the same name 

It also adds a test for this. 

BUG=gyp:270
TEST=test/same-target-name-different-directory/ 
Committed: https://code.google.com/p/gyp/source/detail?r=1415

[Steve Block, 2012-05-29 12:17:53]

This path might require a little cleaning-up before it's landed, and I'm happy
to expand the test too, but wanted to get agreement that this is the right
direction.

[Mark Mentovai, 2012-05-29 14:05:43]

I suggest having this reviewed by someone more familiar with the make and ninja
generators.

[Steve Block, 2012-05-29 14:11:15]

+Nico for review

Mark, let me know if you're not happy with the general approach

[Nico, 2012-05-29 14:17:00]

What's wrong with just disallowing this, as the bug originally suggested? I read
the thread, and I didn't see a reason there either.

[Steve Block, 2012-05-29 14:38:08]

Mark seemed to be strongly against the idea - 'I don't think it's worthwhile or
desirable to reject same-name unqualified targets outright.'

From our (Android backend) perspective, we can work with either allowing or
disallowing duplicate names. We simply want to agree upon a consistent way to
handle them. This is probably best discussed on the bug. Is there anybody else
we should loop in?

[Steve Block, 2012-05-30 09:46:17]

We'd love to reach consensus on this so we can move on.

[Nico, 2012-06-04 23:10:20]

Talked to Mark today, he convinced me that we should move forward with this.

I patched this in on mac and tried to do a build and got:


hummer:src thakis$ time ~/src/ninja/ninja -C out/Release/ chrome
ninja: Entering directory `out/Release/'
ninja: ERROR:
obj/third_party/WebKit/Source/WebCore/WebCore.gyp/webcore_bindings_sources.ninja:990:
expected newline, got lexing error
    webcore_bindings_sources.target.third_party/WebKit/Source/WebCore/We...
                                               ^ near here

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

http://codereview.chromium.org/10447063/diff/1/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:497: name = re.sub(r'[^a-zA-Z0-9_]', '_', '%s_%s' %
(self.qualified_target,
nit: indent looks weird

[Steve Block, 2012-06-07 10:17:40]

> I patched this in on mac and tried to do a build and got:
It looks like this is due to an existing issue which my change now hits - I have
a fix at http://codereview.chromium.org/10535052

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

http://codereview.chromium.org/10447063/diff/1/pylib/gyp/generator/ninja.py#n...
pylib/gyp/generator/ninja.py:497: name = re.sub(r'[^a-zA-Z0-9_]', '_', '%s_%s' %
(self.qualified_target,
On 2012/06/04 23:10:20, Nico wrote:
> nit: indent looks weird

Done.

[Steve Block, 2012-06-07 17:25:06]

Fix make and ninja backends to sensibly handle duplicate target names in
different directories 

Currently, if two multiple targets in different directories use the same name, 
gyp's behaviour depends on the particular backend and is confusing at best. See 
bug for details. 

This change fixes the make and ninja backends to ... 
- always build all targets when duplicate targets exist 
- use the correct action/rule/copy when such duplicate targets define 
actions/rules/copies with the same name 

It also adds a test for this. 

BUG=gyp:270
TEST=test/same-target-name-different-directory/

[Steve Block, 2012-06-07 17:39:19]

Bots seem happy, ready for another look

[Nico, 2012-06-07 19:19:29]

http://codereview.chromium.org/10447063/diff/10005/test/same-target-name-diff...
File test/same-target-name-different-directory/gyptest-all.gyp (right):

http://codereview.chromium.org/10447063/diff/10005/test/same-target-name-diff...
test/same-target-name-different-directory/gyptest-all.gyp:1: #!/usr/bin/env
python
This needs to be called 'gyptest-all.py', else `python gyptest.py -a` won't run
it. That's why you have green trybots; if I run this test locally I get:


hummer:gyp thakis$ python gyptest.py
test/same-target-name-different-directory/gyptest-all.gyp 
PYTHONPATH=/Volumes/MacintoshHD2/src/chrome-git/src/tools/gyp/test/lib
TESTGYP_FORMAT=make
/usr/bin/python test/same-target-name-different-directory/gyptest-all.gyp
PASSED
TESTGYP_FORMAT=ninja
/usr/bin/python test/same-target-name-different-directory/gyptest-all.gyp
PASSED
TESTGYP_FORMAT=xcode
/usr/bin/python test/same-target-name-different-directory/gyptest-all.gyp
None returned 1
STDOUT =========================================================================
Build settings from command line:
    SYMROOT = $SRCROOT/build


STDERR =========================================================================
Unknown target: target
Usage: xcodebuild [-project <projectname>] [-activetarget] [-alltargets]
[-target <targetname>]... [-parallelizeTargets] [-activeconfiguration]
[-configuration <configurationname>] [-sdk <sdkfullpath>|<sdkname>] [-xcconfig
<filepath>] [<buildsetting>=<value>]... [<buildaction>]...
       xcodebuild [-version [-sdk [<sdkfullpath>|<sdkname>] [<item>] ]
       xcodebuild [-showsdks]
       xcodebuild [-find <binary> -sdk <sdkfullpath>|<sdkname>]
       xcodebuild [-list]

FAILED test at line 423 of
/Volumes/MacintoshHD2/src/chrome-git/src/Tools/gyp/test/lib/TestCommon.py
(_complete)
	from line 540 of
/Volumes/MacintoshHD2/src/chrome-git/src/Tools/gyp/test/lib/TestCommon.py (run)
	from line 244 of
/Volumes/MacintoshHD2/src/chrome-git/src/Tools/gyp/test/lib/TestGyp.py (run)
	from line 792 of
/Volumes/MacintoshHD2/src/chrome-git/src/Tools/gyp/test/lib/TestGyp.py (build)
	from line 20 of test/same-target-name-different-directory/gyptest-all.gyp

Failed the following test:
	test/same-target-name-different-directory/gyptest-all.gyp

[Nico, 2012-06-07 19:23:56]

Can you add a "same rule name" case to the test too please?

[Steve Block, 2012-06-08 12:20:18]

> Can you add a "same rule name" case to the test too please?

That reveals another bug in the make backend - fixing in
http://codereview.chromium.org/10538055

http://codereview.chromium.org/10447063/diff/10005/test/same-target-name-diff...
File test/same-target-name-different-directory/gyptest-all.gyp (right):

http://codereview.chromium.org/10447063/diff/10005/test/same-target-name-diff...
test/same-target-name-different-directory/gyptest-all.gyp:1: #!/usr/bin/env
python
Argh, not sure how the name got mangled. I don't have a mac machine here, so
didn't see that failure locally. Will fix.

[Steve Block, 2012-06-08 15:35:54]

Ready for another look

[Nico, 2012-06-08 19:31:02]

lgtm

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

http://codereview.chromium.org/10447063/diff/17006/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:506: name = '%s_%s' % (self.qualified_target,
action['action_name'])
This will make target 'a' with rule 'b_c' clash with target 'a/b''s rule 'c'.

Not sure if you want to worry about that though.

http://codereview.chromium.org/10447063/diff/17006/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:1095: rule_name = re.sub('[^a-zA-Z0-9_]', '_',
rule_name)
Likewise, one issue with this is that 'a+b' and 'a*b' will produce clashing rule
names.

[Steve Block, 2012-06-11 09:33:55]

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

http://codereview.chromium.org/10447063/diff/17006/pylib/gyp/generator/ninja....
pylib/gyp/generator/ninja.py:506: name = '%s_%s' % (self.qualified_target,
action['action_name'])
Yes, but I think it's unlikely we'll hit such problems, and it makes sense to
keep things simple until we have a need to handle such a case.

[Steve Block, 2012-06-11 09:42:18]

Fix make and ninja backends to sensibly handle duplicate target names in
different directories 

Currently, if two multiple targets in different directories use the same name, 
gyp's behaviour depends on the particular backend and is confusing at best. See 
bug for details. 

This change fixes the make and ninja backends to ... 
- always build all targets when duplicate targets exist 
- use the correct action/rule/copy when such duplicate targets define 
actions/rules/copies with the same name 

It also adds a test for this. 

BUG=gyp:270
TEST=test/same-target-name-different-directory/