Add support for customizing GN's args text.

Add support for customizing GN's args text.

It would be nice if, when you ran `gn args out/foo`, you could get
some sample text that was actually customized to the build that
you have, rather than referencing dummy arguments like 'enable_doom_melon'.

This CL adds support for that. If you set `arg_file_template_path` in
your dotfile, then GN will use the contents of that file as the sample
text that it passes to the editor.

R=brettw@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2514333005
Cr-Commit-Position: refs/heads/master@{#454750}
Committed: https://chromium.googlesource.com/chromium/src/+/eacf0f430d80615f07da8bc4094bb981771a155c

[Dirk Pranke, 2016-11-22 04:27:39]

What do you think?

https://codereview.chromium.org/2514333005/diff/1/.gn
File .gn (right):

https://codereview.chromium.org/2514333005/diff/1/.gn#newcode12
.gn:12: arg_file_template = "//build/sample_arg_file.gn"
We don't want to actually land the change to this file until after GN is rolled,
but I'm including it for illustrative purposes for now.

https://codereview.chromium.org/2514333005/diff/1/build/sample_arg_file.gn
File build/sample_arg_file.gn (right):

https://codereview.chromium.org/2514333005/diff/1/build/sample_arg_file.gn#ne...
build/sample_arg_file.gn:6: #   use_goma = false
This is kind of a dumb file name, but I didn't have any better ideas, so I'm
open to suggestions. I'm also open to suggestions for the contents of this file.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc
File tools/gn/command_args.cc (right):

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc#ne...
tools/gn/command_args.cc:257: return 1;
Should I print an err to stdout here if this read fails?

[brettw, 2016-11-22 16:14:33]

https://codereview.chromium.org/2514333005/diff/1/build/sample_arg_file.gn
File build/sample_arg_file.gn (right):

https://codereview.chromium.org/2514333005/diff/1/build/sample_arg_file.gn#ne...
build/sample_arg_file.gn:2: # and their default values (as of when `gn args` was
run):
I think you you delete this second line. The first line is descriptive enough,
and the second line is misleading (is_component_build doesn't default to true if
you flip is_debug).

If we want to add something else, I'd add:
  Run "gn args --list <build_dir>" to see available args.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc
File tools/gn/command_args.cc (right):

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc#ne...
tools/gn/command_args.cc:248:
build_settings.GetFullPath(build_settings.arg_file_template_path())
Indent more.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc#ne...
tools/gn/command_args.cc:254:
build_settings.GetFullPath(build_settings.arg_file_template_path());
Indent mode (maybe this will have to wrap, sadly).

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc#ne...
tools/gn/command_args.cc:257: return 1;
On 2016/11/22 04:27:39, Dirk Pranke wrote:
> Should I print an err to stdout here if this read fails?

No, but I would prefer if it fell through to the default contents.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc#ne...
tools/gn/command_args.cc:263: "#   is_debug = false\n"
Can you remove the examples in this case?

https://codereview.chromium.org/2514333005/diff/1/tools/gn/setup.cc
File tools/gn/setup.cc (right):

https://codereview.chromium.org/2514333005/diff/1/tools/gn/setup.cc#newcode109
tools/gn/setup.cc:109: arg_file_template [optional]
These parameters are alphabetical. Can you put this at the top?

https://codereview.chromium.org/2514333005/diff/1/tools/gn/setup.cc#newcode110
tools/gn/setup.cc:110: Label of a file that contains the text that should be
used as the
Label -> File name (labels are used for targets not files).

https://codereview.chromium.org/2514333005/diff/1/tools/gn/setup.cc#newcode746
tools/gn/setup.cc:746: dotfile_scope_.GetValue("arg_file_template", true);
Indent more.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/setup.cc#newcode753
tools/gn/setup.cc:753: if
(!base::PathExists(path.Resolve(build_settings_.root_path()))) {
I don't think we should be doing filesystem operations at this point. This will
almost never be used and you already have some error handling when it is. This
code is always called for every run.

[Dirk Pranke, 2016-11-22 18:41:41]

Patchset #3 (id:40001) has been deleted

[Dirk Pranke, 2016-11-22 18:41:52]

Updated. Please take another look?

https://codereview.chromium.org/2514333005/diff/1/build/sample_arg_file.gn
File build/sample_arg_file.gn (right):

https://codereview.chromium.org/2514333005/diff/1/build/sample_arg_file.gn#ne...
build/sample_arg_file.gn:2: # and their default values (as of when `gn args` was
run):
On 2016/11/22 16:14:32, brettw (ping on IM after 24h) wrote:
> I think you you delete this second line. The first line is descriptive enough,
> and the second line is misleading (is_component_build doesn't default to true
if
> you flip is_debug).
> 
> If we want to add something else, I'd add:
>   Run "gn args --list <build_dir>" to see available args.

Done.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc
File tools/gn/command_args.cc (right):

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc#ne...
tools/gn/command_args.cc:248:
build_settings.GetFullPath(build_settings.arg_file_template_path())
On 2016/11/22 16:14:32, brettw (ping on IM after 24h) wrote:
> Indent more.

Done.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc#ne...
tools/gn/command_args.cc:254:
build_settings.GetFullPath(build_settings.arg_file_template_path());
On 2016/11/22 16:14:32, brettw (ping on IM after 24h) wrote:
> Indent mode (maybe this will have to wrap, sadly).

Reworked to be shorter.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc#ne...
tools/gn/command_args.cc:257: return 1;
On 2016/11/22 16:14:32, brettw (ping on IM after 24h) wrote:
> On 2016/11/22 04:27:39, Dirk Pranke wrote:
> > Should I print an err to stdout here if this read fails?
> 
> No, but I would prefer if it fell through to the default contents.

Done. I'm not sure if I'm wild about that, but it's probably also not usually
going to happen.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/command_args.cc#ne...
tools/gn/command_args.cc:263: "#   is_debug = false\n"
On 2016/11/22 16:14:32, brettw (ping on IM after 24h) wrote:
> Can you remove the examples in this case?

Done.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/setup.cc
File tools/gn/setup.cc (right):

https://codereview.chromium.org/2514333005/diff/1/tools/gn/setup.cc#newcode109
tools/gn/setup.cc:109: arg_file_template [optional]
On 2016/11/22 16:14:32, brettw (ping on IM after 24h) wrote:
> These parameters are alphabetical. Can you put this at the top?

Done. I thought I checked for that but clearly misread something.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/setup.cc#newcode110
tools/gn/setup.cc:110: Label of a file that contains the text that should be
used as the
On 2016/11/22 16:14:32, brettw (ping on IM after 24h) wrote:
> Label -> File name (labels are used for targets not files).

Changed "Label" to "Path to", instead, and fixed buildconfig to do the same (it
also said label, which is where I got this from).

https://codereview.chromium.org/2514333005/diff/1/tools/gn/setup.cc#newcode746
tools/gn/setup.cc:746: dotfile_scope_.GetValue("arg_file_template", true);
On 2016/11/22 16:14:32, brettw (ping on IM after 24h) wrote:
> Indent more.

Done.

https://codereview.chromium.org/2514333005/diff/1/tools/gn/setup.cc#newcode753
tools/gn/setup.cc:753: if
(!base::PathExists(path.Resolve(build_settings_.root_path()))) {
On 2016/11/22 16:14:32, brettw (ping on IM after 24h) wrote:
> I don't think we should be doing filesystem operations at this point. This
will
> almost never be used and you already have some error handling when it is. This
> code is always called for every run.

Okay. I moved the error handling code.

[brettw, 2016-12-05 18:10:35]

lgtm

[Dirk Pranke, 2017-03-02 01:22:08]

I wonder what the odds are that this still applies and builds ...

[Dirk Pranke, 2017-03-02 01:46:46]

On 2017/03/02 01:22:08, Dirk Pranke wrote:
> I wonder what the odds are that this still applies and builds ...

Bad, apparently. I'll rebase and try to get this landed.

[Dirk Pranke, 2017-03-04 01:39:37]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2017-03-04 01:39:37]

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/2514333005/#ps100001
(title: "update docs")

[commit-bot: I haz the power, 2017-03-04 01:39:53]

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-03-04 02:32:48]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488591577054460,
"parent_rev": "7aed9e267aedf0f2c98ed3c802613ddd080546d1", "commit_rev":
"eacf0f430d80615f07da8bc4094bb981771a155c"}

[commit-bot: I haz the power, 2017-03-04 02:33:23]

Description was changed from

==========
Add support for customizing GN's args text.

It would be nice if, when you ran `gn args out/foo`, you could get
some sample text that was actually customized to the build that
you have, rather than referencing dummy arguments like 'enable_doom_melon'.

This CL adds support for that. If you set `arg_file_template_path` in
your dotfile, then GN will use the contents of that file as the sample
text that it passes to the editor.

R=brettw@chromium.org
BUG=
==========

to

==========
Add support for customizing GN's args text.

It would be nice if, when you ran `gn args out/foo`, you could get
some sample text that was actually customized to the build that
you have, rather than referencing dummy arguments like 'enable_doom_melon'.

This CL adds support for that. If you set `arg_file_template_path` in
your dotfile, then GN will use the contents of that file as the sample
text that it passes to the editor.

R=brettw@chromium.org
BUG=

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

[commit-bot: I haz the power, 2017-03-04 02:33:24]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/eacf0f430d80615f07da8bc4094b...