headless: Replace is_headless gn setting with an args template

headless: Replace is_headless gn setting with an args template

Replace the is_headless setting with a GN args template file which
explicitly lists the settings needed to build the headless
configuration. To make this work, some of the existing settings need to
be moved into declare_args to make them overridable.

BUG=546953
Committed: https://crrev.com/2df951b02d7d03cd35d64e7bfc725eb24e1fd4b3
Cr-Commit-Position: refs/heads/master@{#384549}

[Sami, 2016-03-30 14:35:37]

Description was changed from

==========
headless: Replace is_headless gn setting with an args template

Replace the is_headless setting with a GN args template file which
explicitly lists the settings needed to build the headless
configuration. To make this work, some of the existing settings need to
be moved into declare_args to make them overridable.

BUG=546953
==========

to

==========
headless: Replace is_headless gn setting with an args template

Replace the is_headless setting with a GN args template file which
explicitly lists the settings needed to build the headless
configuration. To make this work, some of the existing settings need to
be moved into declare_args to make them overridable.

BUG=546953
==========

[Sami, 2016-03-30 14:35:37]

skyostil@chromium.org changed reviewers:
+ dpranke@chromium.org

[Sami, 2016-03-30 14:36:09]

PTAL. I'm not sure if there's another alternative to moving the overridden
settings into declare_args()?

[Dirk Pranke, 2016-03-31 01:47:30]

skyostil wrote:
> PTAL. I'm not sure if there's another alternative to moving the
> overridden settings into declare_args()?

Nope, that is in fact what we want you to do.

lgtm w/ a couple of nits. Thanks!

(I didn't realize we already had //build/config/headless.gni; I'll have to smack
jam@ for reviewing that one :).

https://codereview.chromium.org/1845473003/diff/1/build/args/headless.gn
File build/args/headless.gn (right):

https://codereview.chromium.org/1845473003/diff/1/build/args/headless.gn#newc...
build/args/headless.gn:10: target_os = "linux"
I don't think you need to set this one.

https://codereview.chromium.org/1845473003/diff/1/headless/README.md
File headless/README.md (right):

https://codereview.chromium.org/1845473003/diff/1/headless/README.md#newcode16
headless/README.md:16: $ gn gen out/Release
//build/args/headless.gn in this patchset will produce a debug build
(is_debug=true by default), so you should probably either change that or change
this doc to out/Debug (or out/Default).

[Dirk Pranke, 2016-03-31 01:50:44]

See also my comments in 

https://codereview.chromium.org/1845473003/

That CL and this one aren't going to play nicely together.

[Sami, 2016-03-31 09:42:26]

skyostil@chromium.org changed reviewers:
+ asanka@chromium.org, hubbe@chromium.org

[Sami, 2016-03-31 09:43:02]

Thank Dirk. We agreed to land this first and rebase Juan's patch on top of it.

Adding some owners:

asanka:
  net/BUILD.gn
hubbe:
  media/media_options.gni

PTAL.

https://codereview.chromium.org/1845473003/diff/1/build/args/headless.gn
File build/args/headless.gn (right):

https://codereview.chromium.org/1845473003/diff/1/build/args/headless.gn#newc...
build/args/headless.gn:10: target_os = "linux"
On 2016/03/31 01:47:30, Dirk Pranke wrote:
> I don't think you need to set this one.

Removed.

https://codereview.chromium.org/1845473003/diff/1/headless/README.md
File headless/README.md (right):

https://codereview.chromium.org/1845473003/diff/1/headless/README.md#newcode16
headless/README.md:16: $ gn gen out/Release
On 2016/03/31 01:47:30, Dirk Pranke wrote:
> //build/args/headless.gn in this patchset will produce a debug build
> (is_debug=true by default), so you should probably either change that or
change
> this doc to out/Debug (or out/Default).

Ah, good point. Changed this to talk about debug instead.

[hubbe, 2016-03-31 17:23:40]

This seems like a terrible idea.

Not only will headless mode break anytime anybody edits one of these GN files,
but also, does it even work? I mean, I assume that headless.gn gets parsed last
so that it can override variables, but that means that all effects of changing
these variables have to be accounted for in headless.gn, right?  How sure are
you that there isn't an if(use_alsa) somewhere that was missed?  How sure are
you that there won't be one in the future?

[Dirk Pranke, 2016-03-31 17:37:18]

On 2016/03/31 17:23:40, hubbe wrote:
> This seems like a terrible idea.
> 
> Not only will headless mode break anytime anybody edits one of these GN files,
> but also, does it even work? I mean, I assume that headless.gn gets parsed
last
> so that it can override variables, but that means that all effects of changing
> these variables have to be accounted for in headless.gn, right?  How sure are
> you that there isn't an if(use_alsa) somewhere that was missed?  How sure are
> you that there won't be one in the future?

I'm sorry, I'm not following you here at all ...

The way this works is that your args.gn contains an
'import("//build/args/headless.gn")' 
line; that works just like an #include, so this is equivalent to specifying them

directly in args.gn.

Values that are set in args.gn are read up front but applied at the end of the
declare_args() block that declares each variable.

Does this clear something up? If not, maybe you can give an example of the
sort of thing you're worried about?

[asanka, 2016-03-31 17:48:06]

lgtm

https://codereview.chromium.org/1845473003/diff/20001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1845473003/diff/20001/net/BUILD.gn#newcode51
net/BUILD.gn:51: # It needs configuration (krb5.conf and so on).
Nit: Comments in declare_args() are used to describe the option and show up in
'gn args --list' output. In this case the comment should describe what
'use_kerberos' does and optionally explain the requirements for setting it to
true.

[Sami, 2016-03-31 17:56:38]

https://codereview.chromium.org/1845473003/diff/20001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1845473003/diff/20001/net/BUILD.gn#newcode51
net/BUILD.gn:51: # It needs configuration (krb5.conf and so on).
On 2016/03/31 17:48:06, asanka wrote:
> Nit: Comments in declare_args() are used to describe the option and show up in
> 'gn args --list' output. In this case the comment should describe what
> 'use_kerberos' does and optionally explain the requirements for setting it to
> true.

Good point. I've clarified the comment (to the best of my understanding).

[hubbe, 2016-03-31 17:58:35]

On 2016/03/31 17:37:18, Dirk Pranke wrote:
> On 2016/03/31 17:23:40, hubbe wrote:
> > This seems like a terrible idea.
> > 
> > Not only will headless mode break anytime anybody edits one of these GN
files,
> > but also, does it even work? I mean, I assume that headless.gn gets parsed
> last
> > so that it can override variables, but that means that all effects of
changing
> > these variables have to be accounted for in headless.gn, right?  How sure
are
> > you that there isn't an if(use_alsa) somewhere that was missed?  How sure
are
> > you that there won't be one in the future?
> 
> I'm sorry, I'm not following you here at all ...
> 
> The way this works is that your args.gn contains an
> 'import("//build/args/headless.gn")' 
> line; that works just like an #include, so this is equivalent to specifying
them
> 
> directly in args.gn.
> 
> Values that are set in args.gn are read up front but applied at the end of the
> declare_args() block that declares each variable.
> 
> Does this clear something up? If not, maybe you can give an example of the
> sort of thing you're worried about?

I don't think I understand how declare_args() work, but I trust that you do.

However, I'm still worried that maintaining headless.gn is going
to be pretty painful. Let's say for instance that I decide to add support for
JACK sound
server, obviously it should also be turned off in headless mode, but there is
nothing
in media_options.gni to tell me that. Thus headless mode fails (or adds extra
cruft) until
someone adds use_jack=false to the headless.gn file.

[Dirk Pranke, 2016-03-31 18:15:32]

On 2016/03/31 17:58:35, hubbe wrote:
> On 2016/03/31 17:37:18, Dirk Pranke wrote:
> > On 2016/03/31 17:23:40, hubbe wrote:
> > > This seems like a terrible idea.
> > > 
> > > Not only will headless mode break anytime anybody edits one of these GN
> files,
> > > but also, does it even work? I mean, I assume that headless.gn gets parsed
> > last
> > > so that it can override variables, but that means that all effects of
> changing
> > > these variables have to be accounted for in headless.gn, right?  How sure
> are
> > > you that there isn't an if(use_alsa) somewhere that was missed?  How sure
> are
> > > you that there won't be one in the future?
> > 
> > I'm sorry, I'm not following you here at all ...
> > 
> > The way this works is that your args.gn contains an
> > 'import("//build/args/headless.gn")' 
> > line; that works just like an #include, so this is equivalent to specifying
> them
> > 
> > directly in args.gn.
> > 
> > Values that are set in args.gn are read up front but applied at the end of
the
> > declare_args() block that declares each variable.
> > 
> > Does this clear something up? If not, maybe you can give an example of the
> > sort of thing you're worried about?
> 
> I don't think I understand how declare_args() work, but I trust that you do.
> 
> However, I'm still worried that maintaining headless.gn is going
> to be pretty painful. Let's say for instance that I decide to add support for
> JACK sound
> server, obviously it should also be turned off in headless mode, but there is
> nothing
> in media_options.gni to tell me that. Thus headless mode fails (or adds extra
> cruft) until
> someone adds use_jack=false to the headless.gn file.

And you're saying that if there was other references to is_headless in 
media_options.gni then you would be more likely to take that into account?

I suppose that's possible, but I don't know that that's the common case of
how people add features. In other words, I think, most commonly, when
people add a feature they add it either everywhere (on by default), or 
handle just a couple cases, and then people on the other platforms find
out because things break and they update the build files.

To some extent, having the right bots be on the CQ guards against this,
and having assertions in the build files can help as well depending on 
the situation. 

The //build/args template approach is still really an experiment; we've
really only used it for blimp so far, and that's pretty early. Ideally we'd
switch cast to this as well, and possibly some other configs. But, if
this ends up not working we can change it, and I think it's easier
to go from template -> scattering if (is_headless) everywhere than
the other direction.

[hubbe, 2016-03-31 18:32:40]

On 2016/03/31 18:15:32, Dirk Pranke wrote:
> On 2016/03/31 17:58:35, hubbe wrote:
> > On 2016/03/31 17:37:18, Dirk Pranke wrote:
> > > On 2016/03/31 17:23:40, hubbe wrote:
> > > > This seems like a terrible idea.
> > > > 
> > > > Not only will headless mode break anytime anybody edits one of these GN
> > files,
> > > > but also, does it even work? I mean, I assume that headless.gn gets
parsed
> > > last
> > > > so that it can override variables, but that means that all effects of
> > changing
> > > > these variables have to be accounted for in headless.gn, right?  How
sure
> > are
> > > > you that there isn't an if(use_alsa) somewhere that was missed?  How
sure
> > are
> > > > you that there won't be one in the future?
> > > 
> > > I'm sorry, I'm not following you here at all ...
> > > 
> > > The way this works is that your args.gn contains an
> > > 'import("//build/args/headless.gn")' 
> > > line; that works just like an #include, so this is equivalent to
specifying
> > them
> > > 
> > > directly in args.gn.
> > > 
> > > Values that are set in args.gn are read up front but applied at the end of
> the
> > > declare_args() block that declares each variable.
> > > 
> > > Does this clear something up? If not, maybe you can give an example of the
> > > sort of thing you're worried about?
> > 
> > I don't think I understand how declare_args() work, but I trust that you do.
> > 
> > However, I'm still worried that maintaining headless.gn is going
> > to be pretty painful. Let's say for instance that I decide to add support
for
> > JACK sound
> > server, obviously it should also be turned off in headless mode, but there
is
> > nothing
> > in media_options.gni to tell me that. Thus headless mode fails (or adds
extra
> > cruft) until
> > someone adds use_jack=false to the headless.gn file.
> 
> And you're saying that if there was other references to is_headless in 
> media_options.gni then you would be more likely to take that into account?
> 
> I suppose that's possible, but I don't know that that's the common case of
> how people add features. In other words, I think, most commonly, when
> people add a feature they add it either everywhere (on by default), or 
> handle just a couple cases, and then people on the other platforms find
> out because things break and they update the build files.
> 
> To some extent, having the right bots be on the CQ guards against this,
> and having assertions in the build files can help as well depending on 
> the situation. 
> 
> The //build/args template approach is still really an experiment; we've
> really only used it for blimp so far, and that's pretty early. Ideally we'd
> switch cast to this as well, and possibly some other configs. But, if
> this ends up not working we can change it, and I think it's easier
> to go from template -> scattering if (is_headless) everywhere than
> the other direction.

I think these sort of things are dependent on having good flags.
For instance, if there was a "no_sound_output" flag, and headless.gn used that
flag,
then adding JACK support is unlikely to cause any problems.

Anyways, I'm not going to stop you from checking this in, but reserve the right
to say
"I told you so" later. :)

LGTM

[Dirk Pranke, 2016-03-31 18:34:42]

On 2016/03/31 18:32:40, hubbe wrote:
> I think these sort of things are dependent on having good flags.
> For instance, if there was a "no_sound_output" flag, and headless.gn used that
> flag,
> then adding JACK support is unlikely to cause any problems.

Yup, I would much rather see us moving towards good capability flags and
good feature flags.

> Anyways, I'm not going to stop you from checking this in, but reserve the
right
> to say
> "I told you so" later. :)

I am quite prepared for that possibility!

[Sami, 2016-04-01 09:27:37]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-04-01 09:27:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/1845473003/#ps40001
(title: "Better documentation.")

[commit-bot: I haz the power, 2016-04-01 09:27:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845473003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845473003/40001

[commit-bot: I haz the power, 2016-04-01 10:02:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-01 10:02:05]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sami, 2016-04-01 10:16:48]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2016-04-01 10:17:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845473003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845473003/40001

[commit-bot: I haz the power, 2016-04-01 11:33:52]

Description was changed from

==========
headless: Replace is_headless gn setting with an args template

Replace the is_headless setting with a GN args template file which
explicitly lists the settings needed to build the headless
configuration. To make this work, some of the existing settings need to
be moved into declare_args to make them overridable.

BUG=546953
==========

to

==========
headless: Replace is_headless gn setting with an args template

Replace the is_headless setting with a GN args template file which
explicitly lists the settings needed to build the headless
configuration. To make this work, some of the existing settings need to
be moved into declare_args to make them overridable.

BUG=546953
==========

[commit-bot: I haz the power, 2016-04-01 11:33:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-01 11:36:09]

Description was changed from

==========
headless: Replace is_headless gn setting with an args template

Replace the is_headless setting with a GN args template file which
explicitly lists the settings needed to build the headless
configuration. To make this work, some of the existing settings need to
be moved into declare_args to make them overridable.

BUG=546953
==========

to

==========
headless: Replace is_headless gn setting with an args template

Replace the is_headless setting with a GN args template file which
explicitly lists the settings needed to build the headless
configuration. To make this work, some of the existing settings need to
be moved into declare_args to make them overridable.

BUG=546953

Committed: https://crrev.com/2df951b02d7d03cd35d64e7bfc725eb24e1fd4b3
Cr-Commit-Position: refs/heads/master@{#384549}
==========

[commit-bot: I haz the power, 2016-04-01 11:36:10]

Patchset 3 (id:??) landed as
https://crrev.com/2df951b02d7d03cd35d64e7bfc725eb24e1fd4b3
Cr-Commit-Position: refs/heads/master@{#384549}