GN: add default target.

GN: add default target.

Previously, invoking ninja without specifying a target would cause it to try to
build a number of unused and broken targets carried over from Chrome.  This was
worked around by building an explicit "root" target that contained only the
targets needed by Mojo.  The default (no target) build is now equivalent to
"root".  The "root" target still exists, but is now deprecated.

BUG=https://code.google.com/p/chromium/issues/detail?id=401761
R=jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/e5ac34296f3aae1a632149708b1b5cfd6d6a30e4

[Nick Bray (chromium), 2014-12-16 00:07:20]

ncbray@chromium.org changed reviewers:
+ jamesr@chromium.org

[Nick Bray (chromium), 2014-12-16 00:07:20]

This will make Mojo's default build "just work."

Should we remove the root target and change mojob.py to build the default
target?

[jamesr, 2014-12-16 00:09:41]

Why?

https://codereview.chromium.org/809583002/diff/1/DEPS
File DEPS (right):

https://codereview.chromium.org/809583002/diff/1/DEPS#newcode37
DEPS:37: 'buildtools_revision': '4995faa4a7ad968f1fa1917c26edd5cea295582f',
this is not called out in the change description. we roll buildtools with
everything else to keep in sync, you should not change this by itself

[Nick Bray (chromium), 2014-12-16 00:31:29]

> Why?

Why what?  Currently Mojo's default build is broken and there's already been a
question on the mailing list about it.  When I splice in the NaCl build, it will
potentially be brokener because of unneeded targets in the NaCl build.

> DEPS:37: 'buildtools_revision': '4995faa4a7ad968f1fa1917c26edd5cea295582f',
> this is not called out in the change description. we roll buildtools with
> everything else to keep in sync, you should not change this by itself

It was unclear to be that DEPS was also updated.  Happy to wait for the roll,
however, or push it myself.  The default target is a new GN feature.

[jamesr, 2014-12-16 00:41:35]

The build I have generates a target named "default" that depends on the
generated "all" target which builds all targets that the build system finds. 
This target doesn't build in our tree right now, since we DEPS in things that we
don't control or pull in the full dependency tree of, but that's fine.

What problem does adding another alias solve?  Just build the "root" target like
our bots and developers do and be happy.

[Nick Bray (chromium), 2014-12-16 00:56:27]

On 2014/12/16 00:41:35, jamesr wrote:
> The build I have generates a target named "default" that depends on the
> generated "all" target which builds all targets that the build system finds. 
> This target doesn't build in our tree right now, since we DEPS in things that
we
> don't control or pull in the full dependency tree of, but that's fine.
> 
> What problem does adding another alias solve?  Just build the "root" target
like
> our bots and developers do and be happy.

Once everyone stops using the default build, I guess it doesn't really matter if
it works, but n00bs will expect it to work, try it, get confused, and mail the
list.  Making the default build clean eliminates a headache.

For for "adding an alias":
1) This  target overrides the autogenerated default target.  It's a special
name, not really an alias.
2) The "root" target could be eliminated, I didn't do it immediately because I
didn't know what other entanglements there were other than the build script. 
Hence my initial question in this thread.

[jamesr, 2014-12-16 01:08:47]

What does the new buildtools change about the target "default" relative to
what's in my version (307087)?

I'm not really convinced that changing the alias around will be any better.  The
"all" target will still fail to build, for example.  I think some folks will
read the directions, in which case they won't hit this, and some folks will try
other random things that don't work.

[jamesr, 2014-12-16 01:38:26]

I found the gn patch.  If you want to convert from //root to //default I suppose
I don't mind (although keep //root around for compatibility with people's
existing workflows) but switch everything over if you do.  People will still
build the wrong thing and get a broken compile, I'm sure.

[Nick Bray (chromium), 2014-12-16 01:39:57]

On 2014/12/16 01:08:47, jamesr wrote:
> What does the new buildtools change about the target "default" relative to
> what's in my version (307087)?

See: https://codereview.chromium.org/784413004/

> I'm not really convinced that changing the alias around will be any better. 
The
> "all" target will still fail to build, for example.  I think some folks will
> read the directions, in which case they won't hit this, and some folks will
try
> other random things that don't work.

To be completely explicit:
Yes, "all" is going to be screwed up no matter what is done.  That's a feature
of GN.
When I say default build I mean this: "ninja -C out/Release" - no target.
I see the "root" target as a workaround to the limitations of GN, and those
limitations have been eliminated.
Do you object to making a no-target invocation work without compile errors?  And
potentially eliminating the "root" alias?
This is purely a CL of good citizenship.  If you don't think the no-target build
should be cleaned up, I have other fish to fry.

[jamesr, 2014-12-16 01:42:51]

On 2014/12/16 01:38:26, jamesr wrote:
> I found the gn patch.  If you want to convert from //root to //default I
suppose
> I don't mind (although keep //root around for compatibility with people's
> existing workflows) but switch everything

concretely here "everything" includes mojob.py build and the like.

[Nick Bray (chromium), 2014-12-16 20:45:38]

PTAL
Any other "root"s that need to be changed?  Grep has too many false positives to
be useful.

Dirk: feature being use correctly?

[jamesr, 2014-12-16 20:47:57]

lgtm

I think that's it - there might be other documentation floating around but we
can fix that as we find it.

[Dirk Pranke, 2014-12-16 21:01:14]

On 2014/12/16 20:47:57, jamesr wrote:
> lgtm
> 
> I think that's it - there might be other documentation floating around but we
> can fix that as we find it.

lgmt also; you are using the feature correctly.

Personally, I would not add the "root" wrapper target, and I'd wait to hear what
breaks. Adding in the wrapper is either going to delay finding out about the
breakage, or just leave cruft around that you'll need to clean up later.

[jamesr, 2014-12-16 21:05:58]

On 2014/12/16 21:01:14, Dirk Pranke wrote:
> Personally, I would not add the "root" wrapper target, and I'd wait to hear
what
> breaks.

I suspect it will break developer's workflows, not bots, which will cause
frustration and lost time but we probably won't hear about it.  We've been
telling people to build the 'root' target for many months now.  I don't think
the cruft has any real cost.

[Nick Bray (chromium), 2014-12-16 21:07:34]

> Personally, I would not add the "root" wrapper target, and I'd wait to hear
what
> breaks. Adding in the wrapper is either going to delay finding out about the
> breakage, or just leave cruft around that you'll need to clean up later.

I think the play is: land it as is, announce people should stop using root, and
then remove it after a grace period.  I don't mind the second CL if it saves a
bit of confusion.

[Nick Bray (chromium), 2014-12-16 21:27:28]

Committed patchset #2 (id:20001) manually as
e5ac34296f3aae1a632149708b1b5cfd6d6a30e4 (presubmit successful).