GN: Add optional default_args variable to .gn.

GN: Add optional default_args variable to .gn.

Allows a project to specify default overrides for declared arguments.
This is useful for subprojects which might declare arguments with default values
that need to be changed for this project.

BUG=588513
Review-Url: https://codereview.chromium.org/2581193003
Cr-Commit-Position: refs/heads/master@{#445259}
Committed: https://chromium.googlesource.com/chromium/src/+/adbf15668dc1beadf9ee73fe1d7784a5aba207c3

[timn, 2016-12-16 21:14:35]

tim@rnc-ag.de changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org

[timn, 2016-12-16 21:14:35]

There's now some duplicated code for parsing/executing files between .gn,
args.gn and default_args.gn.

This could be refactored into a little helper class (like
InputFileManager::InputFileData, to keep ParseNode etc. around), perhaps as a
follow-up CL or as part of this one.

[timn, 2017-01-03 22:14:14]

ping?

[Dirk Pranke, 2017-01-04 03:38:19]

Sorry for the long delay here :( ...

I think the way I was picturing this to work, you'd just add a "default_args"
scope variable to the dotfile, rather than introducing yet another file.

I'd also expect the values set to act as if they'd been inserted into the front
of an args.gn file (i.e., so any entry in the user's actual args.gn file would
override it); it's not clear to me if your implementation does that?

Can you add some tests to make that clear?

[Dirk Pranke, 2017-01-04 03:38:45]

Oh, and thanks for working on this! It'll definitely clean up some things.

[timn, 2017-01-04 05:42:18]

On 2017/01/04 03:38:19, Dirk Pranke wrote:
> Sorry for the long delay here :( ...
> 
> I think the way I was picturing this to work, you'd just add a "default_args"
> scope variable to the dotfile, rather than introducing yet another file.

Ah, I went with another file because it makes composition easier.
This way one can just import() other files containing overrides.

e.g. //default_args.gn imports //build_overrides/webrtc.gni etc.

Depending on the number of overrides this might be cleaner.
Doing something comparable with scopes seems much harder.
(AFAIK +, += etc. don't even work on scopes)

> I'd also expect the values set to act as if they'd been inserted into the
front
> of an args.gn file (i.e., so any entry in the user's actual args.gn file would
> override it); it's not clear to me if your implementation does that?

It does, calling Args::AddArgOverrides() with the same names just overwrites the
previous values.

> Can you add some tests to make that clear?

So far, the Setup class doesn't have any unit tests and unfortunately doesn't
seem to be easily testable either. :(
I could just write some temporary files and call Setup::DoRun() with a temporary
directory, but all other tests I've looked at so far avoid writing temp. files.

A test for Args::AddArgOverrides() current behavior would be easier ;)

[Dirk Pranke, 2017-01-05 02:59:09]

On 2017/01/04 05:42:18, timn wrote:
> On 2017/01/04 03:38:19, Dirk Pranke wrote:
> > Sorry for the long delay here :( ...
> > 
> > I think the way I was picturing this to work, you'd just add a
"default_args"
> > scope variable to the dotfile, rather than introducing yet another file.
> 
> Ah, I went with another file because it makes composition easier.
> This way one can just import() other files containing overrides.
> 
> e.g. //default_args.gn imports //build_overrides/webrtc.gni etc.
> 
> Depending on the number of overrides this might be cleaner.
> Doing something comparable with scopes seems much harder.
> (AFAIK +, += etc. don't even work on scopes)

Good points. Let me think about this some more.

[Dirk Pranke, 2017-01-09 05:06:37]

I haven't had time to think about this enough yet, but it's still on my radar
... sorry for the delay!

[Dirk Pranke, 2017-01-16 02:56:33]

Having thought about this more ...

I feel like, ideally, there shouldn't be a need to import overrides from other
repos; generally speaking, when files declare default_args(), they should pick
appropriate defaults and it'll be rare that project A will want to re-use
project B's overrides.

However, I have no idea how this is going to play out in practice.

It's not clear to me that there's a strong advantage to declaring a scope in the
dotfile, apart from limiting the # of files you have to have. But, it does seem
like having the default_args file match the args.gn format and be able to import
things has a very clean design. So, I guess I'm inclined to agree that the
approach you've taken is preferable.

I'd like brettw@ to review this as well.

Brett?

[brettw, 2017-01-17 20:34:25]

My original reason for not doing a separate file was just performance (I didn't
want to synchronously load another file blocking setup). I admit this probably
isn't too critical.

I think it comes down to how we want this to be used and where the file will go
if we do that. Normally, such files would go into "//build" but we specifically
don't want this file to go in build because that's shared with other projects.
We could call it "//build/chrome_args.gn" or something, but that doesn't seem
very good to me.

I think the obvious place this can go for Chrome is into the top level
directory. But I would really like to avoid adding more files to the top level
directory and this args file doesn't seem like it rises to the level of
importance of the other files there.

I don't think the "sharing between files" use case is very important here. One
would almost never share *all* args with another project and never change them,
so just referencing some random file probably isn't what you want. I suspect it
would be better to import a file with the defaults from a .gni file and override
or add to those as necessary. That can be done fine whether these are in a
separate file or not.

So my conclusion is that I think it would still be better to have a variable in
the dotfile.

---

We should update the documentation in args.cc. In a new paragraph inserted
before "If specified, arguments..." we should add something like:

Next, project-specific overrides are applied. <<<quick description of
variable>>> See "gn help dotfile" for more.

[timn, 2017-01-17 21:53:26]

On 2017/01/17 20:34:25, brettw (ping after 24h) wrote:
> I don't think the "sharing between files" use case is very important here. One
> would almost never share *all* args with another project and never change
them,
> so just referencing some random file probably isn't what you want. I suspect
it
> would be better to import a file with the defaults from a .gni file and
override
> or add to those as necessary. That can be done fine whether these are in a
> separate file or not.

But that would only work with one .gni file, wouldn't it?

My idea was, that at the top the //default_args.gn files of other projects would
be imported:

import("//v8/default_args.gn")
import("//webrtc/default_args.gn")
...
<chromium specific overrides>

This way, if one of these projects needs to change a default of one of its
dependencies (e.g. gtest), they just add it to their default_args file and
Chromium (and other projects) pick it up automatically.
Additionally, the default only needs to be changed inside the project that needs
it, which is less error-prone and easier to reason about.

Hmm, I just realized that this has its own set of problems (multiple import()s
and value collisions):
e.g. with the layout above:
* v8 and webrtc use different values for the same override -> error (would this
be intended? seems nice in theory, but I'm not sure...)
* scopes are used as an override -> error (does that happen?)

I'll upload a new patchset using the variable-approach.

[brettw, 2017-01-18 23:18:31]

Yeah, V8 will configure the defaults for its own args in the declarations for
the args, so those wouldn't be listed in v8/default_args.gn. The only way that
V8's default args could apply to Chrome is to configure V8's dependencies. But
then this overlaps with other uses of those dependencies. I don't think we have
many (if any) such indirect dependencies that are only used by only one deps-ed
in component such that Chrome would want to outsource configuring that
component.

If we do, then we can add a v8/build/gtest_args.gni" which defines some variable
that both V8 and Chrome repos can share to set up their project overrides.

[Dirk Pranke, 2017-01-19 01:43:49]

On 2017/01/18 23:18:31, brettw (ping after 24h) wrote:
> Yeah, V8 will configure the defaults for its own args in the declarations for
> the args, so those wouldn't be listed in v8/default_args.gn. The only way that
> V8's default args could apply to Chrome is to configure V8's dependencies. But
> then this overlaps with other uses of those dependencies. I don't think we
have
> many (if any) such indirect dependencies that are only used by only one
deps-ed
> in component such that Chrome would want to outsource configuring that
> component.
> 
> If we do, then we can add a v8/build/gtest_args.gni" which defines some
variable
> that both V8 and Chrome repos can share to set up their project overrides.

Okay, Brett swung me back to my initial leanings, so I agree with him, let's use
something in the dotfile.

[timn, 2017-01-19 10:12:16]

PTAL, default_args is now a scope.

[brettw, 2017-01-19 18:47:08]

lgtm

https://codereview.chromium.org/2581193003/diff/20001/tools/gn/setup.h
File tools/gn/setup.h (right):

https://codereview.chromium.org/2581193003/diff/20001/tools/gn/setup.h#newcod...
tools/gn/setup.h:153: // Default overrides, specified in the dotfile.
Can you add a comment here that this pointer is owned by the value (if it
exists) in the dotfile_scope_?

[timn, 2017-01-21 02:52:45]

https://codereview.chromium.org/2581193003/diff/20001/tools/gn/setup.h
File tools/gn/setup.h (right):

https://codereview.chromium.org/2581193003/diff/20001/tools/gn/setup.h#newcod...
tools/gn/setup.h:153: // Default overrides, specified in the dotfile.
On 2017/01/19 18:47:08, brettw (ping after 24h) wrote:
> Can you add a comment here that this pointer is owned by the value (if it
> exists) in the dotfile_scope_?

Done.

[timn, 2017-01-21 02:52:54]

The CQ bit was checked by tim@rnc-ag.de

[timn, 2017-01-21 02:52:55]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2581193003/#ps40001
(title: "Address comment.")

[commit-bot: I haz the power, 2017-01-21 02:53:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 03:06:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484967174739290,
"parent_rev": "0010b369bbcf9235abd54ba181ef07b0cf6b7d6f", "commit_rev":
"adbf15668dc1beadf9ee73fe1d7784a5aba207c3"}

[commit-bot: I haz the power, 2017-01-21 03:07:15]

Description was changed from

==========
GN: Add optional default_args variable to .gn.

Allows a project to specify default overrides for declared arguments.
This is useful for subprojects which might declare arguments with default values
that need to be changed for this project.

BUG=588513
==========

to

==========
GN: Add optional default_args variable to .gn.

Allows a project to specify default overrides for declared arguments.
This is useful for subprojects which might declare arguments with default values
that need to be changed for this project.

BUG=588513

Review-Url: https://codereview.chromium.org/2581193003
Cr-Commit-Position: refs/heads/master@{#445259}
Committed:
https://chromium.googlesource.com/chromium/src/+/adbf15668dc1beadf9ee73fe1d77...
==========

[commit-bot: I haz the power, 2017-01-21 03:07:16]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/adbf15668dc1beadf9ee73fe1d77...

[brettw, 2017-01-23 21:01:34]

I just landed the roll for this in https://codereview.chromium.org/2647423002