Add an option to prune unwanted targets

Add an option to prune targets

Normally, when a target in file1.gyp depends on a target in file2.gyp,
gyp will generate projects for all targets in file2.gyp, even if the
target in file1.gyp does not depend on all of them.

This patch adds a '--root-target' command-line option, which allows users
to specify a list of "root" targets in the dependency tree.  When this
list is provided, gyp will only generate projects for targets that
are deep dependencies of these root targets.

As an example, when used in the chromium repository, running 'gyp_chromium'
with 'content.gyp' generates 397 subninja files.  With '--root-target=content'
specified, this is reduced to just 260 subninja files.

BUG=
R=mark@chromium.org

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

[Shezan Baig (Bloomberg), 2013-09-26 21:40:01]

Adding a new option to reduce the number of subninja files generated.

[scottmg, 2013-09-26 22:11:07]

Seems fine. I'll defer to Nico and Mark on whether they have any reservations
about this as a feature.

https://codereview.chromium.org/24803004/diff/1/pylib/gyp/input.py
File pylib/gyp/input.py (right):

https://codereview.chromium.org/24803004/diff/1/pylib/gyp/input.py#newcode2636
pylib/gyp/input.py:2636: # Normalize paths everywhere.  This is important
because paths will be
Why did this need to move?

https://codereview.chromium.org/24803004/diff/1/pylib/gyp/input.py#newcode2701
pylib/gyp/input.py:2701: [targets, flat_list] = PruneUnwantedTargets(
[ ] around targets, flat_list is odd here. Just return a tuple from the
function.

https://codereview.chromium.org/24803004/diff/1/test/prune_targets/lib.cc
File test/prune_targets/lib.cc (right):

https://codereview.chromium.org/24803004/diff/1/test/prune_targets/lib.cc#new...
test/prune_targets/lib.cc:5: void libfunc1()
nit; move braces to function line

[Mark Mentovai, 2013-09-26 22:57:18]

We’ve got a couple of concepts here: files and targets. The existing
architecture basically prunes files based on inputs that are files, because
files that aren’t reachable won’t be loaded at all. You’re now adding an option
to prune targets based on inputs that are files, which seems a little lop-sided
to me. I would prefer to see this implemented as a way to prune targets based on
target inputs, which would be treated as “roots” to the dependency tree, with
the potential to specify more than one target as a “root.” For the behavior that
you’ve implemented here, you would be able to specify an implicit “all” target.
In other respects, specifying a single target as a root for pruning purposes is
more flexible.

[Torne, 2013-09-27 10:05:01]

So the Android generator already has a generator flag "limit_to_target_all"
which I think does the same/similar thing (it stops output being generated for
targets which would not be built by "make all"). If these are going to end up
having the exact same effect then I can switch the Android uses of the generator
flag over to this and then remove that code. If they aren't going to end up
having the exact same effect, then we should check what effect is more
sensible.. having two versions of this that work differently seems bad.

[Shezan Baig (Bloomberg), 2013-09-27 13:02:02]

On 2013/09/27 10:05:01, Torne wrote:
> So the Android generator already has a generator flag "limit_to_target_all"
> which I think does the same/similar thing (it stops output being generated for
> targets which would not be built by "make all"). If these are going to end up
> having the exact same effect then I can switch the Android uses of the
generator
> flag over to this and then remove that code. If they aren't going to end up
> having the exact same effect, then we should check what effect is more
> sensible.. having two versions of this that work differently seems bad.

Thanks for this pointer, I wasn't aware of it.  I'll take a look at the android
flag, and compare the behavior.

@Scott, the "Normalize paths everywhere" code had to be moved out because I am
reusing the normalized build_paths as keys in the PruneUnwantedTargets function,
so instead of normalizing them again there, I figured it would be better to just
do it once.  However, right now I'm modifying the patch to use a list of root
targets, instead of build files (based on Mark's comment), so I don't think this
will be necessary anymore, so I can move it back to where it was.

[Shezan Baig (Bloomberg), 2013-09-27 15:08:26]

PTAL.  Instead of a boolean '--prune-targets', gyp now accepts a list of
'--target=TARGET' arguments.  These are the root targets and gyp will only
generate projects for these targets and their deep dependencies.

@Torne, I looked at the flag in the android generator, and yes, it does work
similar to patchset 1 in this CL.  However, I feel that patchset 2, which is
based on Mark's comments, is more flexible, and has the added benefit of
automatically working on other generators.  WDYT?

[Torne, 2013-09-27 15:12:40]

On 2013/09/27 15:08:26, Shezan Baig (Bloomberg) wrote:
> PTAL.  Instead of a boolean '--prune-targets', gyp now accepts a list of
> '--target=TARGET' arguments.  These are the root targets and gyp will only
> generate projects for these targets and their deep dependencies.
> 
> @Torne, I looked at the flag in the android generator, and yes, it does work
> similar to patchset 1 in this CL.  However, I feel that patchset 2, which is
> based on Mark's comments, is more flexible, and has the added benefit of
> automatically working on other generators.  WDYT?

So if I were to change my config to use --target=all instead of -G
limit_to_target_all=1 it would do the same thing (to the best of your
understanding)? If so then that sounds great to me, and I can switch our config
over and then remove the limit_to_target_all code. I didn't mean I don't want
you to do this because there's already a gross hack in one generator, I want to
burn down our gross hacks without losing functionality ;)

It does seem a little weird that --target=all results in not all possible
targets being generated, but the fact that "all" doesn't actually mean "all
possible targets" is an existing quirk of gyp that's nothing to do with this CL,
so hey ;)

[Mark Mentovai, 2013-09-27 15:18:26]

I’ll be out until Tuesday, I won’t be able to look again until then.

[Shezan Baig (Bloomberg), 2013-09-27 15:22:39]

On 2013/09/27 15:12:40, Torne wrote:
> So if I were to change my config to use --target=all instead of -G
> limit_to_target_all=1 it would do the same thing (to the best of your
> understanding)?


Not really, no.  The specified target must be an actual target name in one of
the gyp files.  So in theory, you *could* have a gyp file, with an "all" target
of type "none" that just depends on the targets that you actually want.

Alternatively, I can make "all" a magic keyword that just expands the list of
root targets to the list returned by gyp.common.AllTargets (for all build
files).  I'm open to doing this if it is desired.

[Torne, 2013-09-27 15:23:29]

On 2013/09/27 15:12:40, Torne wrote:
> On 2013/09/27 15:08:26, Shezan Baig (Bloomberg) wrote:
> > PTAL.  Instead of a boolean '--prune-targets', gyp now accepts a list of
> > '--target=TARGET' arguments.  These are the root targets and gyp will only
> > generate projects for these targets and their deep dependencies.
> > 
> > @Torne, I looked at the flag in the android generator, and yes, it does work
> > similar to patchset 1 in this CL.  However, I feel that patchset 2, which is
> > based on Mark's comments, is more flexible, and has the added benefit of
> > automatically working on other generators.  WDYT?
> 
> So if I were to change my config to use --target=all instead of -G
> limit_to_target_all=1 it would do the same thing (to the best of your
> understanding)? If so then that sounds great to me, and I can switch our
config
> over and then remove the limit_to_target_all code. I didn't mean I don't want
> you to do this because there's already a gross hack in one generator, I want
to
> burn down our gross hacks without losing functionality ;)

Er, rather I probably mean --target=All which is what the real target at the
root of our dependencies is called, "all" is not a real gyp target, just a
target in the generated makefiles. :)

> It does seem a little weird that --target=all results in not all possible
> targets being generated, but the fact that "all" doesn't actually mean "all
> possible targets" is an existing quirk of gyp that's nothing to do with this
CL,
> so hey ;)

[Torne, 2013-09-27 15:32:07]

On 2013/09/27 15:22:39, Shezan Baig (Bloomberg) wrote:
> On 2013/09/27 15:12:40, Torne wrote:
> > So if I were to change my config to use --target=all instead of -G
> > limit_to_target_all=1 it would do the same thing (to the best of your
> > understanding)?
> 
> 
> Not really, no.  The specified target must be an actual target name in one of
> the gyp files.  So in theory, you *could* have a gyp file, with an "all"
target
> of type "none" that just depends on the targets that you actually want.

Yeah, sorry, see my other post; I meant "All" which is a real target that does
exactly that.

> Alternatively, I can make "all" a magic keyword that just expands the list of
> root targets to the list returned by gyp.common.AllTargets (for all build
> files).  I'm open to doing this if it is desired.

I don't think this is necessary, it's fine as it is.

[Shezan Baig (Bloomberg), 2013-09-27 15:54:51]

On 2013/09/27 15:32:07, Torne wrote:
> On 2013/09/27 15:22:39, Shezan Baig (Bloomberg) wrote:
> > On 2013/09/27 15:12:40, Torne wrote:
> > > So if I were to change my config to use --target=all instead of -G
> > > limit_to_target_all=1 it would do the same thing (to the best of your
> > > understanding)?
> > 
> > 
> > Not really, no.  The specified target must be an actual target name in one
of
> > the gyp files.  So in theory, you *could* have a gyp file, with an "all"
> target
> > of type "none" that just depends on the targets that you actually want.
> 
> Yeah, sorry, see my other post; I meant "All" which is a real target that does
> exactly that.
> 
> > Alternatively, I can make "all" a magic keyword that just expands the list
of
> > root targets to the list returned by gyp.common.AllTargets (for all build
> > files).  I'm open to doing this if it is desired.
> 
> I don't think this is necessary, it's fine as it is.


Great! :)

[Mark Mentovai, 2013-10-01 18:37:05]

https://codereview.chromium.org/24803004/diff/9001/pylib/gyp/__init__.py
File pylib/gyp/__init__.py (right):

https://codereview.chromium.org/24803004/diff/9001/pylib/gyp/__init__.py#newc...
pylib/gyp/__init__.py:340: parser.add_option('-T', '--target',
dest='root_targets', action='append',
--target isn’t a descriptive enough long name. If this is about roots for
pruning, then the name should probably include at least one of those words.

https://codereview.chromium.org/24803004/diff/9001/pylib/gyp/__init__.py#newc...
pylib/gyp/__init__.py:342: help='Prune out targets that are not deep
dependencies '
Prune is fine, you don’t need to prune out.

https://codereview.chromium.org/24803004/diff/9001/pylib/gyp/__init__.py#newc...
pylib/gyp/__init__.py:343: 'of TARGET')
Can you bump this over so that the ' lines up with the one above, to make it
seem less like a distinct argument? Maybe if you get rid of the “out” or reword
it a little bit you can make it concise enough that it doesn’t need to wrap at
all.

https://codereview.chromium.org/24803004/diff/9001/test/prune_targets/lib_ind...
File test/prune_targets/lib_indirect.cc (right):

https://codereview.chromium.org/24803004/diff/9001/test/prune_targets/lib_ind...
test/prune_targets/lib_indirect.cc:5: void libfuncIndirect() {
Follow the C++ style guide for naming: libfunc_indirect.

[Shezan Baig (Bloomberg), 2013-10-01 19:24:26]

On 2013/10/01 18:37:05, Mark Mentovai wrote:
> https://codereview.chromium.org/24803004/diff/9001/pylib/gyp/__init__.py
> File pylib/gyp/__init__.py (right):
> 
>
https://codereview.chromium.org/24803004/diff/9001/pylib/gyp/__init__.py#newc...
> pylib/gyp/__init__.py:340: parser.add_option('-T', '--target',
> dest='root_targets', action='append',
> --target isn’t a descriptive enough long name. If this is about roots for
> pruning, then the name should probably include at least one of those words.
> 
>
https://codereview.chromium.org/24803004/diff/9001/pylib/gyp/__init__.py#newc...
> pylib/gyp/__init__.py:342: help='Prune out targets that are not deep
> dependencies '
> Prune is fine, you don’t need to prune out.
> 
>
https://codereview.chromium.org/24803004/diff/9001/pylib/gyp/__init__.py#newc...
> pylib/gyp/__init__.py:343: 'of TARGET')
> Can you bump this over so that the ' lines up with the one above, to make it
> seem less like a distinct argument? Maybe if you get rid of the “out” or
reword
> it a little bit you can make it concise enough that it doesn’t need to wrap at
> all.
> 
>
https://codereview.chromium.org/24803004/diff/9001/test/prune_targets/lib_ind...
> File test/prune_targets/lib_indirect.cc (right):
> 
>
https://codereview.chromium.org/24803004/diff/9001/test/prune_targets/lib_ind...
> test/prune_targets/lib_indirect.cc:5: void libfuncIndirect() {
> Follow the C++ style guide for naming: libfunc_indirect.

Patch updated -- ptal.  Thanks!

[Mark Mentovai, 2013-10-01 20:49:37]

LGTM

[Shezan Baig (Bloomberg), 2013-10-01 20:58:57]

Committed patchset #3 manually as r1745 (presubmit successful).

[Shezan Baig (Bloomberg), 2013-10-01 23:17:49]

On 2013/10/01 20:58:57, Shezan Baig (Bloomberg) wrote:
> Committed patchset #3 manually as r1745 (presubmit successful).

There was a problem with the test on xcode.  I will revert this:
https://codereview.chromium.org/25398003/