Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(634)

Issue 2717083002: Enable GN check for webrtc/base (Closed)

Created:
3 years, 10 months ago by kjellander_webrtc
Modified:
3 years, 8 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc, mbonadei
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Enable GN check for webrtc/base It's not possible to enable it for the rtc_base_approved target but since a larger refactoring is ongoing for webrtc/base this CL doesn't attempt to fix that. Changes made: * Move webrtc/system_wrappers/include/stringize_macros.h into webrtc/base:rtc_base_approved_unittests (and corresponding unit test to rtc_base_approved_unittests). * Move md5digest.* from rtc_base_approved to rtc_base_test_utils target. * Move webrtc/system_wrappers/include/stringize_macros.h (+test) into webrtc/base. * Remove unused use include of webrtc/base/fileutils.h in webrtc/base/pathutils.cc BUG=webrtc:6828, webrtc:3806, webrtc:7480 NOTRY=True Review-Url: https://codereview.webrtc.org/2717083002 Cr-Commit-Position: refs/heads/master@{#17766} Committed: https://chromium.googlesource.com/external/webrtc/+/ed754e71ae8866db641677073274e86fe704eeac

Patch Set 1 #

Patch Set 2 : Rebased #

Patch Set 3 : Remove .gn change to run chromium trybots #

Patch Set 4 : Fix GN error #

Patch Set 5 : Restore .gn change again #

Total comments: 8

Patch Set 6 : Rebased #

Total comments: 7

Patch Set 7 : Addressed review comments and moved things around #

Patch Set 8 : Updated comment #

Patch Set 9 : PS without .gn change for Chromium trybots #

Patch Set 10 : Restored .gn again #

Patch Set 11 : Added dependency on base:rtc_base_tests_utils for neteq_rtp_fuzzer #

Total comments: 5

Patch Set 12 : Removed webrtc_common dep #

Patch Set 13 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -84 lines) Patch
M .gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 11 chunks +26 lines, -2 lines 0 comments Download
M webrtc/base/location.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M webrtc/base/pathutils.cc View 1 2 3 4 5 6 1 chunk +0 lines, -1 line 0 comments Download
A + webrtc/base/stringize_macros.h View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
A + webrtc/base/stringize_macros_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M webrtc/common_audio/resampler/sinc_resampler_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/audio_coding/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/system_wrappers/BUILD.gn View 1 2 3 4 5 6 2 chunks +0 lines, -2 lines 0 comments Download
D webrtc/system_wrappers/include/stringize_macros.h View 1 2 3 4 5 6 1 chunk +0 lines, -38 lines 0 comments Download
D webrtc/system_wrappers/source/stringize_macros_unittest.cc View 1 2 3 4 5 6 1 chunk +0 lines, -35 lines 0 comments Download
M webrtc/test/fuzzers/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/voice_engine/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 39 (15 generated)
kjellander_webrtc
3 years, 9 months ago (2017-03-08 08:37:48 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2717083002/80001
3 years, 9 months ago (2017-03-08 09:53:42 UTC) #5
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...
3 years, 9 months ago (2017-03-08 09:53:44 UTC) #7
perkj_webrtc
Tommi, please? should webrtc_common be moved to rtc_base_approved if this deps already exis?. Or should ...
3 years, 9 months ago (2017-03-08 12:24:11 UTC) #9
tommi
https://codereview.webrtc.org/2717083002/diff/80001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2717083002/diff/80001/webrtc/base/BUILD.gn#newcode279 webrtc/base/BUILD.gn:279: deps += [ "..:webrtc_common" ] base_approved shouldn't have any ...
3 years, 9 months ago (2017-03-08 13:36:46 UTC) #10
kjellander_webrtc
https://codereview.webrtc.org/2717083002/diff/80001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2717083002/diff/80001/webrtc/base/BUILD.gn#newcode279 webrtc/base/BUILD.gn:279: deps += [ "..:webrtc_common" ] On 2017/03/08 13:36:45, tommi ...
3 years, 9 months ago (2017-03-08 13:56:04 UTC) #11
kjellander_webrtc
Please have another look
3 years, 8 months ago (2017-03-30 19:32:29 UTC) #12
tommi
https://codereview.webrtc.org/2717083002/diff/80001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2717083002/diff/80001/webrtc/base/BUILD.gn#newcode86 webrtc/base/BUILD.gn:86: # :rtc_base_approved -> :rtc_base -> :rtc_base_approved where is the ...
3 years, 8 months ago (2017-03-31 08:38:29 UTC) #13
kjellander_webrtc
Answered tommi's questions and added webrtc:3806. Per: Please review as this is blocking downstream work ...
3 years, 8 months ago (2017-04-04 04:23:09 UTC) #15
perkj_webrtc
To unblock your work: lgtm But please address the comments below and discuss rtc_base_approved with ...
3 years, 8 months ago (2017-04-04 09:30:59 UTC) #16
tommi
https://codereview.webrtc.org/2717083002/diff/80001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2717083002/diff/80001/webrtc/base/BUILD.gn#newcode86 webrtc/base/BUILD.gn:86: # :rtc_base_approved -> :rtc_base -> :rtc_base_approved On 2017/04/04 04:23:09, ...
3 years, 8 months ago (2017-04-04 09:53:14 UTC) #17
kjellander_webrtc
Adding nisse@ for more input. It seems easiest if we just merge system_wrappers into rtc_base_approved. ...
3 years, 8 months ago (2017-04-10 12:54:02 UTC) #19
nisse-webrtc
On 2017/04/10 12:54:02, kjellander_webrtc wrote: > Adding nisse@ for more input. > > It seems ...
3 years, 8 months ago (2017-04-11 07:29:47 UTC) #20
kjellander_webrtc
New patch set up that addresses 3 of the 4 violations. I'm responding to review ...
3 years, 8 months ago (2017-04-12 11:51:45 UTC) #21
kjellander_webrtc
On 2017/04/11 07:29:47, nisse-webrtc wrote: > On 2017/04/10 12:54:02, kjellander_webrtc wrote: > > Adding nisse@ ...
3 years, 8 months ago (2017-04-12 12:24:41 UTC) #24
nisse-webrtc
lgtm
3 years, 8 months ago (2017-04-18 10:32:54 UTC) #25
kjellander_webrtc
tommi: can you have a final look?
3 years, 8 months ago (2017-04-19 09:33:42 UTC) #27
kjellander_webrtc
Adding comment about webrtc_common usage per tommi's request. https://codereview.webrtc.org/2717083002/diff/200001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2717083002/diff/200001/webrtc/base/BUILD.gn#newcode294 webrtc/base/BUILD.gn:294: deps ...
3 years, 8 months ago (2017-04-19 10:58:47 UTC) #29
tommi
https://codereview.webrtc.org/2717083002/diff/200001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2717083002/diff/200001/webrtc/base/BUILD.gn#newcode294 webrtc/base/BUILD.gn:294: deps += [ "..:webrtc_common" ] do we still need ...
3 years, 8 months ago (2017-04-19 13:45:01 UTC) #30
kjellander_webrtc
I removed webrtc_common dep in new patch set now. https://codereview.webrtc.org/2717083002/diff/200001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2717083002/diff/200001/webrtc/base/BUILD.gn#newcode294 webrtc/base/BUILD.gn:294: ...
3 years, 8 months ago (2017-04-19 13:57:51 UTC) #31
tommi
On 2017/04/19 13:57:51, kjellander_webrtc wrote: > I removed webrtc_common dep in new patch set now. ...
3 years, 8 months ago (2017-04-19 15:26:47 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2717083002/240001
3 years, 8 months ago (2017-04-19 15:35:01 UTC) #35
commit-bot: I haz the power
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/external/webrtc/+/ed754e71ae8866db641677073274e86fe704eeac
3 years, 8 months ago (2017-04-19 15:37:43 UTC) #38
mbonadei
3 years, 8 months ago (2017-04-24 19:12:40 UTC) #39
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.webrtc.org/2838683002/ by mbonadei@webrtc.org.

The reason for reverting is: Breaks Chromium because in Chromium we import
WebRTC with rtc_include_tests=false
(https://bugs.chromium.org/p/chromium/issues/detail?id=713179#c6).

Chromium uses webrtc/test/fuzzers and this CL adds test dependencies to
neteq_rtc_fuzzer..

Powered by Google App Engine
This is Rietveld 408576698