Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629353004:
Added gn target for rtc_event_log2rtp_dump. (Closed)
|
Created:
3 years, 11 months ago by ivoc Modified:
3 years, 10 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded gn target for rtc_event_log2rtp_dump.
BUG=webrtc:6191
Review-Url: https://codereview.webrtc.org/2629353004
Cr-Commit-Position: refs/heads/master@{#16483}
Committed: https://chromium.googlesource.com/external/webrtc/+/54b6e982dfc9c2269f450738365e7bc2e8193d3b
Patch Set 1 #
Total comments: 2
Patch Set 2 : Review feedback. #
Total comments: 8
Patch Set 3 : Review comments. #Patch Set 4 : Added system_wrappers:field_trial_default to deps #Messages
Total messages: 29 (12 generated)
ivoc@webrtc.org changed reviewers: + kjellander@webrtc.org
Hi Henrik, I added a gn target for rtc_event_log2rtp_dump. PTAL.
lgtm with comment addressed. https://codereview.webrtc.org/2629353004/diff/1/webrtc/logging/BUILD.gn File webrtc/logging/BUILD.gn (right): https://codereview.webrtc.org/2629353004/diff/1/webrtc/logging/BUILD.gn#newco... webrtc/logging/BUILD.gn:110: if (rtc_enable_protobuf && rtc_include_tests) { Please just add a if (rtc_include_tests) block inside the already existing rtc_enable_protobuf block above.
Thanks! https://codereview.webrtc.org/2629353004/diff/1/webrtc/logging/BUILD.gn File webrtc/logging/BUILD.gn (right): https://codereview.webrtc.org/2629353004/diff/1/webrtc/logging/BUILD.gn#newco... webrtc/logging/BUILD.gn:110: if (rtc_enable_protobuf && rtc_include_tests) { On 2017/01/16 07:49:53, kjellander_webrtc OOO to Jan19 wrote: > Please just add a if (rtc_include_tests) block inside the already existing > rtc_enable_protobuf block above. Done.
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2629353004/#ps20001 (title: "Review feedback.")
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
Try jobs failed on following builders: android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...) android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/20302)
https://codereview.webrtc.org/2629353004/diff/20001/webrtc/logging/BUILD.gn File webrtc/logging/BUILD.gn (right): https://codereview.webrtc.org/2629353004/diff/20001/webrtc/logging/BUILD.gn#n... webrtc/logging/BUILD.gn:115: ":rtc_event_log_api", The trybots seems to say you need to add a dependency on ../system_wrappers:metrics_default
https://codereview.webrtc.org/2629353004/diff/20001/webrtc/logging/BUILD.gn File webrtc/logging/BUILD.gn (right): https://codereview.webrtc.org/2629353004/diff/20001/webrtc/logging/BUILD.gn#n... webrtc/logging/BUILD.gn:115: ":rtc_event_log_api", The trybots seems to say you need to add a dependency on ../system_wrappers:metrics_default
https://codereview.webrtc.org/2629353004/diff/20001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2629353004/diff/20001/webrtc/BUILD.gn#newcode380 webrtc/BUILD.gn:380: "logging:rtc_event_log2rtp_dump", rtc_unittests is a test binary, but rtc_event_log2rtp_dump is a stand-alone tool. Is this the recommended way to add build rules for tools?
https://codereview.webrtc.org/2629353004/diff/20001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2629353004/diff/20001/webrtc/BUILD.gn#newcode380 webrtc/BUILD.gn:380: "logging:rtc_event_log2rtp_dump", On 2017/02/06 13:25:27, terelius wrote: > rtc_unittests is a test binary, but rtc_event_log2rtp_dump is a stand-alone > tool. Is this the recommended way to add build rules for tools? No, and I guess it's not very clear for tools that are not inside webrtc/tools. I suggest you do it similar to other tests, i.e. put it at line 292 instead. https://codereview.webrtc.org/2629353004/diff/20001/webrtc/logging/BUILD.gn File webrtc/logging/BUILD.gn (right): https://codereview.webrtc.org/2629353004/diff/20001/webrtc/logging/BUILD.gn#n... webrtc/logging/BUILD.gn:115: ":rtc_event_log_api", On 2017/01/19 07:23:47, kjellander_webrtc wrote: > The trybots seems to say you need to add a dependency on > ../system_wrappers:metrics_default They're usually right :) https://codereview.webrtc.org/2629353004/diff/20001/webrtc/logging/BUILD.gn#n... webrtc/logging/BUILD.gn:126: suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] Is it possible to fix these warnings or do you inherit a lot of the dependencies?
https://codereview.webrtc.org/2629353004/diff/20001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2629353004/diff/20001/webrtc/BUILD.gn#newcode380 webrtc/BUILD.gn:380: "logging:rtc_event_log2rtp_dump", On 2017/02/07 06:59:09, kjellander_webrtc wrote: > On 2017/02/06 13:25:27, terelius wrote: > > rtc_unittests is a test binary, but rtc_event_log2rtp_dump is a stand-alone > > tool. Is this the recommended way to add build rules for tools? > > No, and I guess it's not very clear for tools that are not inside webrtc/tools. > I suggest you do it similar to other tests, i.e. put it at line 292 instead. Done, I did put it in an rtc_enable_protobuf block though. https://codereview.webrtc.org/2629353004/diff/20001/webrtc/logging/BUILD.gn File webrtc/logging/BUILD.gn (right): https://codereview.webrtc.org/2629353004/diff/20001/webrtc/logging/BUILD.gn#n... webrtc/logging/BUILD.gn:115: ":rtc_event_log_api", On 2017/02/07 06:59:09, kjellander_webrtc wrote: > On 2017/01/19 07:23:47, kjellander_webrtc wrote: > > The trybots seems to say you need to add a dependency on > > ../system_wrappers:metrics_default > > They're usually right :) Done. https://codereview.webrtc.org/2629353004/diff/20001/webrtc/logging/BUILD.gn#n... webrtc/logging/BUILD.gn:126: suppressed_configs += [ "//build/config/clang:find_bad_constructs" ] On 2017/02/07 06:59:09, kjellander_webrtc wrote: > Is it possible to fix these warnings or do you inherit a lot of the > dependencies? The warnings don't trigger on the code in this tool, but on the dependencies.
lgtm
The CQ bit was checked by ivoc@webrtc.org
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
Try jobs failed on following builders: android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2629353004/#ps60001 (title: "Added system_wrappers:field_trial_default to deps")
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
Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/5996)
The CQ bit was checked by ivoc@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486541789254770,
"parent_rev": "7798501d7a20129d45889aac6854f00a55f20445", "commit_rev":
"54b6e982dfc9c2269f450738365e7bc2e8193d3b"}
Message was sent while issue was closed.
Description was changed from ========== Added gn target for rtc_event_log2rtp_dump. BUG=webrtc:6191 ========== to ========== Added gn target for rtc_event_log2rtp_dump. BUG=webrtc:6191 Review-Url: https://codereview.webrtc.org/2629353004 Cr-Commit-Position: refs/heads/master@{#16483} Committed: https://chromium.googlesource.com/external/webrtc/+/54b6e982dfc9c2269f4507383... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/54b6e982dfc9c2269f4507383... |
