Explicit dependency on executables in proto_library.gni.

Explicit dependency on executables in proto_library.gni.

GN uses wrapper script to invoke protobuf compiler and
script depends on it using deps variable in GN action.
This change makes this script also dependent on protoc
(or plugin) executable explicitly.

BUG=644525
Committed: https://crrev.com/10009d80608ae4f8c40f60a5a768cf00f7169bf4
Cr-Commit-Position: refs/heads/master@{#426374}

[kraynov, 2016-10-18 12:13:58]

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

[commit-bot: I haz the power, 2016-10-18 12:14:18]

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

[kraynov, 2016-10-18 12:35:52]

kraynov@chromium.org changed reviewers:
+ brettw@chromium.org, sdefresne@chromium.org

[kraynov, 2016-10-18 12:35:52]

Hi,
As discussed in internal communication the right approach to make GN action
depend on executable is listing such label in *deps* and also list said binary
in *inputs* variable.
We have annoying bug crbug.com/644525 and this CL might solve this issue which
is suspected to be subtle race condition.
Thank you!

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
File third_party/protobuf/proto_library.gni (left):

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
third_party/protobuf/proto_library.gni:221: invoker.generator_plugin_script,
It's not missing :)
invoker.generator_plugin_script == plugin_path
See line 284 at the right pane.

[commit-bot: I haz the power, 2016-10-18 13:28:41]

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

[commit-bot: I haz the power, 2016-10-18 13:28:42]

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

[sdefresne, 2016-10-18 22:44:55]

lgtm

Please add BUG=644525 to description.

Can you confirm that the protoc compiler is listed in the input of target using
proto_library template? The following command-line should give you the answer:

$ gn gen out/default
$ gn desc out/default
//chrome/browser/profile_resetter:profile_reset_report_proto_gen inputs

This will print "//out/default/protoc" (on Linux or macOS) if the protoc is
correctly listed as input or "Don't know how to display "inputs" for "action"."
otherwise.

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
File third_party/protobuf/proto_library.gni (left):

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
third_party/protobuf/proto_library.gni:116: if (host_os == "win") {
This is done multiple time in this file. I would recommend adding this at the
top of the file:

if (host_os == "win") {
  _host_executable_suffix = ".exe"
} else {
  _host_executable_suffix = ""
}

And then, use:

    plugin_path = get_label_info(plugin_host_label, "root_out_dir") + "/" +
                  get_label_info(plugin_host_label, "name") +
                  _host_executable_suffix

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
third_party/protobuf/proto_library.gni:221: invoker.generator_plugin_script,
On 2016/10/18 12:35:52, kraynov wrote:
> It's not missing :)
> invoker.generator_plugin_script == plugin_path
> See line 284 at the right pane.

Ack.

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
File third_party/protobuf/proto_library.gni (right):

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
third_party/protobuf/proto_library.gni:219: protoc_path =
get_label_info(protoc_label, "root_out_dir") + "/protoc"
    protoc_path = get_label_info(protoc_label, "root_out_dir") + "/protoc" +
        _host_executable_suffix

[kraynov, 2016-10-19 11:01:38]

Description was changed from

==========
Explicit dependency on executables in proto_library.gni.
==========

to

==========
Explicit dependency on executables in proto_library.gni.

GN uses wrapper script to invoke protobuf compiler and
script depends on it using deps variable in GN action.
This change makes this script also dependent on protoc
(or plugin) executable explicitly.

BUG=644525
==========

[kraynov, 2016-10-19 11:04:59]

kraynov@chromium.org changed reviewers:
+ xyzzyz@chromium.org

[kraynov, 2016-10-19 11:04:59]

+xyzzyz

sdefresne@, yes, I confirm that inputs added correctly.

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
File third_party/protobuf/proto_library.gni (left):

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
third_party/protobuf/proto_library.gni:116: if (host_os == "win") {
On 2016/10/18 22:44:54, sdefresne wrote:
> This is done multiple time in this file. I would recommend adding this at the
> top of the file:
> 
> if (host_os == "win") {
>   _host_executable_suffix = ".exe"
> } else {
>   _host_executable_suffix = ""
> }
> 
> And then, use:
> 
>     plugin_path = get_label_info(plugin_host_label, "root_out_dir") + "/" +
>                   get_label_info(plugin_host_label, "name") +
>                   _host_executable_suffix

Done.

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
File third_party/protobuf/proto_library.gni (right):

https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_...
third_party/protobuf/proto_library.gni:219: protoc_path =
get_label_info(protoc_label, "root_out_dir") + "/protoc"
On 2016/10/18 22:44:54, sdefresne wrote:
>     protoc_path = get_label_info(protoc_label, "root_out_dir") + "/protoc" +
>         _host_executable_suffix

Done.

[sdefresne, 2016-10-19 13:19:02]

lgtm

[xyzzyz, 2016-10-19 18:16:52]

lgtm

[brettw, 2016-10-19 19:19:14]

LGTM with change below

https://codereview.chromium.org/2427943002/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/proto_library.gni (right):

https://codereview.chromium.org/2427943002/diff/20001/third_party/protobuf/pr...
third_party/protobuf/proto_library.gni:230: rebase_path(protoc_path,
root_build_dir),
You lost the "./" which is necessary to prevent the path from being searched
when root_out_dir == root_build_dir. This is what the "system" comment is
referring to, but it should probably be improved.

[kraynov, 2016-10-19 22:27:04]

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

[commit-bot: I haz the power, 2016-10-19 22:27:38]

Dry run: 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, 2016-10-19 23:15:48]

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

[commit-bot: I haz the power, 2016-10-19 23:15:49]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[kraynov, 2016-10-19 23:22:54]

https://codereview.chromium.org/2427943002/diff/20001/third_party/protobuf/pr...
File third_party/protobuf/proto_library.gni (right):

https://codereview.chromium.org/2427943002/diff/20001/third_party/protobuf/pr...
third_party/protobuf/proto_library.gni:230: rebase_path(protoc_path,
root_build_dir),
On 2016/10/19 19:19:14, brettw (ping on IM after 24h) wrote:
> You lost the "./" which is necessary to prevent the path from being searched
> when root_out_dir == root_build_dir. This is what the "system" comment is
> referring to, but it should probably be improved.

Many thanks!

[kraynov, 2016-10-20 00:34:37]

The CQ bit was checked by kraynov@chromium.org

[kraynov, 2016-10-20 00:34:37]

The patchset sent to the CQ was uploaded after l-g-t-m from xyzzyz@chromium.org,
sdefresne@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2427943002/#ps40001
(title: "never system protoc")

[commit-bot: I haz the power, 2016-10-20 00:35:22]

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, 2016-10-20 02:04:31]

Description was changed from

==========
Explicit dependency on executables in proto_library.gni.

GN uses wrapper script to invoke protobuf compiler and
script depends on it using deps variable in GN action.
This change makes this script also dependent on protoc
(or plugin) executable explicitly.

BUG=644525
==========

to

==========
Explicit dependency on executables in proto_library.gni.

GN uses wrapper script to invoke protobuf compiler and
script depends on it using deps variable in GN action.
This change makes this script also dependent on protoc
(or plugin) executable explicitly.

BUG=644525
==========

[commit-bot: I haz the power, 2016-10-20 02:04:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:14:31]

Description was changed from

==========
Explicit dependency on executables in proto_library.gni.

GN uses wrapper script to invoke protobuf compiler and
script depends on it using deps variable in GN action.
This change makes this script also dependent on protoc
(or plugin) executable explicitly.

BUG=644525
==========

to

==========
Explicit dependency on executables in proto_library.gni.

GN uses wrapper script to invoke protobuf compiler and
script depends on it using deps variable in GN action.
This change makes this script also dependent on protoc
(or plugin) executable explicitly.

BUG=644525

Committed: https://crrev.com/10009d80608ae4f8c40f60a5a768cf00f7169bf4
Cr-Commit-Position: refs/heads/master@{#426374}
==========

[commit-bot: I haz the power, 2016-10-21 13:14:32]

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