Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2985853002:
Add gn dependency between ana_debug_dump_proto and ana_config_proto. (Closed)
|
Created:
3 years, 5 months ago by jbudorick Modified:
3 years, 4 months ago Reviewers:
minyue-webrtc CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd gn dependency between ana_debug_dump_proto and ana_config_proto.
BUG=chromium:746106
Review-Url: https://codereview.webrtc.org/2985853002
Cr-Commit-Position: refs/heads/master@{#19158}
Committed: https://chromium.googlesource.com/external/webrtc/+/58f1725ff1feeb665aaa1bc222e6d36148d8fcb7
Patch Set 1 #
Messages
Total messages: 19 (10 generated)
Description was changed from ========== Add gn dependency between ana_debug_dump_proto and ana_config_proto. BUG=chromium:746106 ========== to ========== Add gn dependency between ana_debug_dump_proto and ana_config_proto. TBR=minyue@webrtc.org BUG=chromium:746106 ==========
Description was changed from ========== Add gn dependency between ana_debug_dump_proto and ana_config_proto. TBR=minyue@webrtc.org BUG=chromium:746106 ========== to ========== Add gn dependency between ana_debug_dump_proto and ana_config_proto. BUG=chromium:746106 ==========
jbudorick@chromium.org changed reviewers: + minyue@webrtc.org
The CQ bit was checked by jbudorick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping -- this is flakily breaking official builds, at least.
On 2017/07/26 18:16:46, jbudorick wrote: > ping -- this is flakily breaking official builds, at least. Also please remember that we'll likely need this merged back to M61 as well.
On 2017/07/26 19:36:28, amineer wrote: > On 2017/07/26 18:16:46, jbudorick wrote: > > ping -- this is flakily breaking official builds, at least. > > Also please remember that we'll likely need this merged back to M61 as well. Hi, thanks for the work. and it looks indeed correct. But I would like to understand why it is only flaky.
lgtm
On 2017/07/26 21:11:33, minyue-webrtc wrote: > On 2017/07/26 19:36:28, amineer wrote: > > On 2017/07/26 18:16:46, jbudorick wrote: > > > ping -- this is flakily breaking official builds, at least. > > > > Also please remember that we'll likely need this merged back to M61 as well. > > Hi, thanks for the work. and it looks indeed correct. But I would like to > understand why it is only flaky. With no dependency link between them, ninja's only ordering constraint is that they both must be built before audio_network_adaptor. If it builds ana_config_proto first, config.pb.h exists when building ana_debug_dump_proto and everything works. If it builds ana_debug_dump_proto first, config.pb.h may not exist (I imagine it may already exist when a previous build has run?) and, if it doesn't, ana_debug_dump_proto will fail to build.
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2017/07/26 21:15:43, jbudorick wrote: > On 2017/07/26 21:11:33, minyue-webrtc wrote: > > On 2017/07/26 19:36:28, amineer wrote: > > > On 2017/07/26 18:16:46, jbudorick wrote: > > > > ping -- this is flakily breaking official builds, at least. > > > > > > Also please remember that we'll likely need this merged back to M61 as well. > > > > Hi, thanks for the work. and it looks indeed correct. But I would like to > > understand why it is only flaky. > > With no dependency link between them, ninja's only ordering constraint is that > they both must be built before audio_network_adaptor. If it builds > ana_config_proto first, config.pb.h exists when building ana_debug_dump_proto > and everything works. If it builds ana_debug_dump_proto first, config.pb.h may > not exist (I imagine it may already exist when a previous build has run?) and, > if it doesn't, ana_debug_dump_proto will fail to build. Thanks!
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1501103812194400, "parent_rev":
"74544f9d1b2b3ed2ea3d13cda3d01cd00b4a6661", "commit_rev":
"58f1725ff1feeb665aaa1bc222e6d36148d8fcb7"}
Message was sent while issue was closed.
Description was changed from ========== Add gn dependency between ana_debug_dump_proto and ana_config_proto. BUG=chromium:746106 ========== to ========== Add gn dependency between ana_debug_dump_proto and ana_config_proto. BUG=chromium:746106 Review-Url: https://codereview.webrtc.org/2985853002 Cr-Commit-Position: refs/heads/master@{#19158} Committed: https://chromium.googlesource.com/external/webrtc/+/58f1725ff1feeb665aaa1bc22... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/58f1725ff1feeb665aaa1bc22... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
