Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(71)

Issue 2427943002: Explicit dependency on executables in proto_library.gni. (Closed)

Created:
2 years, 10 months ago by kraynov
Modified:
2 years, 10 months ago
Reviewers:
xyzzyz, sdefresne, brettw
CC:
chromium-reviews, Primiano Tucci (use gerrit)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 6

Patch Set 2 : nit #

Total comments: 2

Patch Set 3 : never system protoc #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -21 lines) Patch
M third_party/protobuf/proto_library.gni View 1 2 6 chunks +28 lines, -21 lines 0 comments Download

Messages

Total messages: 25 (15 generated)
kraynov
Hi, As discussed in internal communication the right approach to make GN action depend on ...
2 years, 10 months ago (2016-10-18 12:35:52 UTC) #4
sdefresne
lgtm Please add BUG=644525 to description. Can you confirm that the protoc compiler is listed ...
2 years, 10 months ago (2016-10-18 22:44:55 UTC) #7
kraynov
+xyzzyz sdefresne@, yes, I confirm that inputs added correctly. https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_library.gni File third_party/protobuf/proto_library.gni (left): https://codereview.chromium.org/2427943002/diff/1/third_party/protobuf/proto_library.gni#oldcode116 third_party/protobuf/proto_library.gni:116: ...
2 years, 10 months ago (2016-10-19 11:04:59 UTC) #10
sdefresne
lgtm
2 years, 10 months ago (2016-10-19 13:19:02 UTC) #11
xyzzyz
lgtm
2 years, 10 months ago (2016-10-19 18:16:52 UTC) #12
brettw
LGTM with change below https://codereview.chromium.org/2427943002/diff/20001/third_party/protobuf/proto_library.gni File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2427943002/diff/20001/third_party/protobuf/proto_library.gni#newcode230 third_party/protobuf/proto_library.gni:230: rebase_path(protoc_path, root_build_dir), You lost the ...
2 years, 10 months ago (2016-10-19 19:19:14 UTC) #13
kraynov
https://codereview.chromium.org/2427943002/diff/20001/third_party/protobuf/proto_library.gni File third_party/protobuf/proto_library.gni (right): https://codereview.chromium.org/2427943002/diff/20001/third_party/protobuf/proto_library.gni#newcode230 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 ...
2 years, 10 months ago (2016-10-19 23:22:54 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2427943002/40001
2 years, 10 months ago (2016-10-20 00:35:22 UTC) #21
commit-bot: I haz the power
Committed patchset #3 (id:40001)
2 years, 10 months ago (2016-10-20 02:04:33 UTC) #23
commit-bot: I haz the power
2 years, 10 months ago (2016-10-21 13:14:32 UTC) #25
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/10009d80608ae4f8c40f60a5a768cf00f7169bf4
Cr-Commit-Position: refs/heads/master@{#426374}

Powered by Google App Engine
This is Rietveld 408576698