Support script response files in GN.

Support script response files in GN.

Exposes Ninja's response file capabilities to GN actions so that long command lines can be passed to scripts without manually calling write_file.

Change the domain_reliability bake_in_configs script to use this facility as a test. There are now command-line flags that support both reading the .gypi file and passing a response file. This patch adds the BUILD rule for the new-style response file, but doesn't use it (pending a GN binary push).

Removes running actions through gyp-win-tool on Windows. This is a bunch of hardcoded commands that are not necessary to my knowledge.

Fixes an out-of-date cpu_arch reference in the mini installer build file I noticed (in a NaCl block that's not run yet).

Committed: https://crrev.com/d6763a7390d6ca727c090a403d5dbefa94c5d79b
Cr-Commit-Position: refs/heads/master@{#358968}

[brettw, 2015-11-04 08:58:15]

Description was changed from

==========
Support script response files in GN.

Exposes Ninja's response file capabilities to GN actions so that long
command lines can be passed to scripts without manually calling write_file.
==========

to

==========
Support script response files in GN.

Exposes Ninja's response file capabilities to GN actions so that long
command lines can be passed to scripts without manually calling write_file.

Change the domain_reliability bake_in_configs script to use this facility as a
test. There are now command-line flags that support both reading the .gypi file
and passing a response file. This patch adds the BUILD rule for the new-style
response file, but doesn't use it (pending a GN binary push).

Removes running actions through gyp-win-tool on Windows. This is a bunch of
hardcoded commands that are not necessary to my knowledge.
==========

[brettw, 2015-11-04 08:58:16]

brettw@chromium.org changed reviewers:
+ scottmg@chromium.org

[brettw, 2015-11-04 08:58:55]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-04 08:59:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1430043002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1430043002/20001

[brettw, 2015-11-04 09:02:24]

This isn't quite ready for final review yet since it still needs tests. But I
uploaded today's checkpoint to check with you whether you think I can remove
using gyp-win-tool (and the associated environment stuff) for actions on
Windows.

If we have actions that depend on environment variables, with this change the
script will see the current environment rather than the one when GN was run.
Since GN expects to be able to run all the time, the only thing that should be
affected are the things that we manually tweak in the environment, which are
only MSVC stuff which won't be used by Python scripts.

Please let me know if you think my analysis is correct. It really sucks to have
to hardcode gyp-win-tool into GN itself, as this is a pretty weird special case.

[commit-bot: I haz the power, 2015-11-04 09:44:36]

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

[commit-bot: I haz the power, 2015-11-04 09:44:36]

Dry run: 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_...)

[scottmg, 2015-11-04 22:35:33]

The only things that I can recall that were actions that needed the environment
were some Blink steps (and maybe some NaCl steps?) that expected to find cl.exe
in the PATH, so it needed the environment setup.

I expect NaCl wouldn't need that in a GN world.

I'm not sure if some Blink things still use a compiler for preprocessing in IDLs
or something, but I think that was all rewritten at some point, so not sure if
it's necessary there or not any more.

https://codereview.chromium.org/1430043002/diff/20001/components/domain_relia...
File components/domain_reliability/bake_in_configs.py (right):

https://codereview.chromium.org/1430043002/diff/20001/components/domain_relia...
components/domain_reliability/bake_in_configs.py:517: help="File containing
whitespace separated names of " +
Don't need + at the end of these lines.

https://codereview.chromium.org/1430043002/diff/20001/components/domain_relia...
components/domain_reliability/bake_in_configs.py:542: 
Remove blank line.

https://codereview.chromium.org/1430043002/diff/20001/components/domain_relia...
components/domain_reliability/bake_in_configs.py:546: 
And here.

https://codereview.chromium.org/1430043002/diff/20001/tools/gn/action_target_...
File tools/gn/action_target_generator.cc (right):

https://codereview.chromium.org/1430043002/diff/20001/tools/gn/action_target_...
tools/gn/action_target_generator.cc:78: SUBSTITUTION_RSP_FILE_NAME) !=
required_args_substitutions.end();
this is weird formatting, did clang-format do this?

https://codereview.chromium.org/1430043002/diff/20001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/1430043002/diff/20001/tools/gn/variables.cc#n...
tools/gn/variables.cc:1193: "    inputs = [ ... huge list of files ... ]\n"
I'm not sure if this is handled somehow, but gyp uses $in_newline rather than
$in for the linker specifically. Using $in puts the whole thing on the same
line, but the linker will (used to?) crap out at ... 128k or 256k long lines so
the .objs have to be put on multiple lines.

It shouldn't matter for actions but if you want to use it more generally.

[scottmg, 2015-11-04 23:05:55]

Coincidentally I just got this CL
https://codereview.chromium.org/1415983005/diff/1/tools/win_as.py which uses
cl.exe. So I think whatever that is still probably needs to get
environment.x86/64 when run.

[brettw, 2015-11-05 02:13:19]

https://codereview.chromium.org/1430043002/diff/20001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/1430043002/diff/20001/tools/gn/variables.cc#n...
tools/gn/variables.cc:1193: "    inputs = [ ... huge list of files ... ]\n"
These inputs are different than the Ninja inputs that correspond to $in, so I
don't think there's any way for this to include newlines. I was thinking for a
while about putting each arg in the response file on a separate line which means
ew don't have to do escaping or quoting and the file may be easier to read from
a script. But it seems there's not actually a way to express newlines in Ninja
files, so I did the Posix escaping + shlex thing (at least Python has an easy
built-in way to parse these).

[brettw, 2015-11-05 02:25:03]

Good find, maybe I can just change that script to manually load the environment
file itself.

[brettw, 2015-11-05 13:34:07]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-05 13:34:28]

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

[brettw, 2015-11-05 13:47:40]

New patch up with tests. Now that we don't do special weird stuff on Windows,
the tests got a lot simpler.

I will wait until I can fix that NaCl script and roll NaCl deps before
committing.

[brettw, 2015-11-05 13:47:44]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-05 13:48:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1430043002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1430043002/60001

[commit-bot: I haz the power, 2015-11-05 15:00:47]

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

[commit-bot: I haz the power, 2015-11-05 15:00:48]

Dry run: This issue passed the CQ dry run.

[scottmg, 2015-11-05 17:40:01]

lgtm

https://codereview.chromium.org/1430043002/diff/60001/components/domain_relia...
File components/domain_reliability/BUILD.gn (right):

https://codereview.chromium.org/1430043002/diff/60001/components/domain_relia...
components/domain_reliability/BUILD.gn:22: # TODO(brettw) enable once GN
supports response_file_contets for actions.
"contents"

[scottmg, 2015-11-05 17:40:39]

On 2015/11/05 02:13:19, brettw wrote:
> https://codereview.chromium.org/1430043002/diff/20001/tools/gn/variables.cc
> File tools/gn/variables.cc (right):
> 
>
https://codereview.chromium.org/1430043002/diff/20001/tools/gn/variables.cc#n...
> tools/gn/variables.cc:1193: "    inputs = [ ... huge list of files ... ]\n"
> These inputs are different than the Ninja inputs that correspond to $in, so I
> don't think there's any way for this to include newlines. I was thinking for a
> while about putting each arg in the response file on a separate line which
means
> ew don't have to do escaping or quoting and the file may be easier to read
from
> a script. But it seems there's not actually a way to express newlines in Ninja
> files, so I did the Posix escaping + shlex thing (at least Python has an easy
> built-in way to parse these).

Seems reasonable. I was thinking more of passing them directly to cl or whatever
in the future, but for actions going through python this seems fine.

[brettw, 2015-11-10 22:55:41]

Description was changed from

==========
Support script response files in GN.

Exposes Ninja's response file capabilities to GN actions so that long
command lines can be passed to scripts without manually calling write_file.

Change the domain_reliability bake_in_configs script to use this facility as a
test. There are now command-line flags that support both reading the .gypi file
and passing a response file. This patch adds the BUILD rule for the new-style
response file, but doesn't use it (pending a GN binary push).

Removes running actions through gyp-win-tool on Windows. This is a bunch of
hardcoded commands that are not necessary to my knowledge.
==========

to

==========
Support script response files in GN.

Exposes Ninja's response file capabilities to GN actions so that long command
lines can be passed to scripts without manually calling write_file.

Change the domain_reliability bake_in_configs script to use this facility as a
test. There are now command-line flags that support both reading the .gypi file
and passing a response file. This patch adds the BUILD rule for the new-style
response file, but doesn't use it (pending a GN binary push).

Removes running actions through gyp-win-tool on Windows. This is a bunch of
hardcoded commands that are not necessary to my knowledge.

Fixes an out-of-date cpu_arch reference in the mini installer build file I
noticed (in a NaCl block that's not run yet).
==========

[brettw, 2015-11-10 22:55:44]

The CQ bit was checked by brettw@chromium.org

[brettw, 2015-11-10 22:55:45]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1430043002/#ps80001
(title: "review comment, random build fix")

[commit-bot: I haz the power, 2015-11-10 22:56:56]

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

[brettw, 2015-11-10 22:57:01]

For the record, it seems win_as pointed out by Scott already runs through
gyp-win-tool manually so no updates are required.

[commit-bot: I haz the power, 2015-11-11 00:12:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-11 00:12:59]

Patchset 5 (id:??) landed as
https://crrev.com/d6763a7390d6ca727c090a403d5dbefa94c5d79b
Cr-Commit-Position: refs/heads/master@{#358968}