Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionEnable 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 #
Messages
Total messages: 39 (15 generated)
kjellander@webrtc.org changed reviewers: + perkj@webrtc.org
Description was changed from ========== 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 don't attempt to fix that. BUG=webrtc:6828 ========== to ========== 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 don't attempt to fix that. BUG=webrtc:6828 NOTRY=True ==========
The CQ bit was checked by kjellander@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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
perkj@webrtc.org changed reviewers: + tommi@webrtc.org
Tommi, please? should webrtc_common be moved to rtc_base_approved if this deps already exis?. Or should whatever it is that exist in rtc_base_approved that depends on webrtc_common be moved out?
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#newc... webrtc/base/BUILD.gn:279: deps += [ "..:webrtc_common" ] base_approved shouldn't have any dependencies like this. a dependency should be the other way.
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#newc... webrtc/base/BUILD.gn:279: deps += [ "..:webrtc_common" ] On 2017/03/08 13:36:45, tommi (webrtc) wrote: > base_approved shouldn't have any dependencies like this. a dependency should be > the other way. Well, what I'm doing here is just adding what is actually used in the sources. webrtc_common is a tiny target only containing a few headers, but I'm open to merging it into rtc_base_approved (but not in this CL). If I remove this and the check_includes = false line at line 88 I get these errors: ERROR at //webrtc/base/checks.h:14:11: Include not allowed. #include "webrtc/typedefs.h" ^---------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc:webrtc_common which should somehow be reachable. ___________________ ERROR at //webrtc/base/format_macros.h:27:11: Include not allowed. #include "webrtc/typedefs.h" ^---------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc:webrtc_common which should somehow be reachable. ___________________ ERROR at //webrtc/base/location.h:16:11: Include not allowed. #include "webrtc/system_wrappers/include/stringize_macros.h" ^------------------------------------------------ It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc/system_wrappers:system_wrappers which should somehow be reachable. ___________________ ERROR at //webrtc/base/md5digest.h:15:11: Include not allowed. #include "webrtc/base/messagedigest.h" ^-------------------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc/base:rtc_base which should somehow be reachable. ___________________ ERROR at //webrtc/base/onetimeevent.h:15:11: Include not allowed. #include "webrtc/typedefs.h" ^---------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc:webrtc_common which should somehow be reachable. ___________________ ERROR at //webrtc/base/pathutils.cc:19:11: Include not allowed. #include "webrtc/base/fileutils.h" ^---------------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc/base:rtc_base which should somehow be reachable. ___________________ ERROR at //webrtc/base/random.h:16:11: Include not allowed. #include "webrtc/typedefs.h" ^---------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc:webrtc_common which should somehow be reachable. ___________________ ERROR at //webrtc/base/rate_limiter.cc:12:11: Include not allowed. #include "webrtc/system_wrappers/include/clock.h" ^------------------------------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc/system_wrappers:system_wrappers which should somehow be reachable. ___________________ ERROR at //webrtc/base/rate_statistics.h:17:11: Include not allowed. #include "webrtc/typedefs.h" ^---------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc:webrtc_common which should somehow be reachable.
Please have another look
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#newc... webrtc/base/BUILD.gn:86: # :rtc_base_approved -> :rtc_base -> :rtc_base_approved where is the dependency from base_approved to base? https://codereview.webrtc.org/2717083002/diff/80001/webrtc/base/BUILD.gn#newc... webrtc/base/BUILD.gn:279: deps += [ "..:webrtc_common" ] On 2017/03/08 13:56:04, kjellander_webrtc wrote: > On 2017/03/08 13:36:45, tommi (webrtc) wrote: > > base_approved shouldn't have any dependencies like this. a dependency should > be > > the other way. > > Well, what I'm doing here is just adding what is actually used in the sources. > webrtc_common is a tiny target only containing a few headers, but I'm open to > merging it into rtc_base_approved (but not in this CL). If I remove this and the > check_includes = false line at line 88 I get these errors: > > ERROR at //webrtc/base/checks.h:14:11: Include not allowed. > #include "webrtc/typedefs.h" > ^---------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc:webrtc_common > which should somehow be reachable. > ___________________ > ERROR at //webrtc/base/format_macros.h:27:11: Include not allowed. > #include "webrtc/typedefs.h" > ^---------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc:webrtc_common > which should somehow be reachable. > ___________________ > ERROR at //webrtc/base/location.h:16:11: Include not allowed. > #include "webrtc/system_wrappers/include/stringize_macros.h" > ^------------------------------------------------ > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc/system_wrappers:system_wrappers > which should somehow be reachable. > ___________________ > ERROR at //webrtc/base/md5digest.h:15:11: Include not allowed. > #include "webrtc/base/messagedigest.h" > ^-------------------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc/base:rtc_base > which should somehow be reachable. > ___________________ > ERROR at //webrtc/base/onetimeevent.h:15:11: Include not allowed. > #include "webrtc/typedefs.h" > ^---------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc:webrtc_common > which should somehow be reachable. > ___________________ > ERROR at //webrtc/base/pathutils.cc:19:11: Include not allowed. > #include "webrtc/base/fileutils.h" > ^---------------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc/base:rtc_base > which should somehow be reachable. > ___________________ > ERROR at //webrtc/base/random.h:16:11: Include not allowed. > #include "webrtc/typedefs.h" > ^---------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc:webrtc_common > which should somehow be reachable. > ___________________ > ERROR at //webrtc/base/rate_limiter.cc:12:11: Include not allowed. > #include "webrtc/system_wrappers/include/clock.h" > ^------------------------------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc/system_wrappers:system_wrappers > which should somehow be reachable. > ___________________ > ERROR at //webrtc/base/rate_statistics.h:17:11: Include not allowed. > #include "webrtc/typedefs.h" > ^---------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc:webrtc_common > which should somehow be reachable. Ack. Is there information about this in the bug? (or is this another bug?)
Description was changed from ========== 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 don't attempt to fix that. BUG=webrtc:6828 NOTRY=True ========== to ========== 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 don't attempt to fix that. BUG=webrtc:6828, webrtc:3806 NOTRY=True ==========
Answered tommi's questions and added webrtc:3806. Per: Please review as this is blocking downstream work (and tommi is OOO). 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#newc... webrtc/base/BUILD.gn:86: # :rtc_base_approved -> :rtc_base -> :rtc_base_approved On 2017/03/31 08:38:29, tommi (webrtc) wrote: > where is the dependency from base_approved to base? ERROR at //webrtc/base/md5digest.h:15:11: Include not allowed. #include "webrtc/base/messagedigest.h" ^-------------------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc/base:rtc_base which should somehow be reachable. ___________________ ERROR at //webrtc/base/pathutils.cc:19:11: Include not allowed. #include "webrtc/base/fileutils.h" ^---------------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc/base:rtc_base which should somehow be reachable. The first one can easily be fixed by moving messagedigest.h to rtc_base_approved. But the second one requires more work (it was sneaked in here: https://codereview.webrtc.org/2533213005/diff/120001/webrtc/base/BUILD.gn). Also there are two violations causing a cycle with system_wrappers as well: ERROR at //webrtc/base/location.h:16:11: Include not allowed. #include "webrtc/system_wrappers/include/stringize_macros.h" ^------------------------------------------------ It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc/system_wrappers:system_wrappers which should somehow be reachable. ___________________ ERROR at //webrtc/base/rate_limiter.cc:12:11: Include not allowed. #include "webrtc/system_wrappers/include/clock.h" ^------------------------------------- It is not in any dependency of //webrtc/base:rtc_base_approved The include file is in the target(s): //webrtc/system_wrappers:system_wrappers which should somehow be reachable. https://codereview.webrtc.org/2717083002/diff/80001/webrtc/base/BUILD.gn#newc... webrtc/base/BUILD.gn:279: deps += [ "..:webrtc_common" ] On 2017/03/31 08:38:29, tommi (webrtc) wrote: > On 2017/03/08 13:56:04, kjellander_webrtc wrote: > > On 2017/03/08 13:36:45, tommi (webrtc) wrote: > > > base_approved shouldn't have any dependencies like this. a dependency should > > be > > > the other way. > > > > Well, what I'm doing here is just adding what is actually used in the sources. > > webrtc_common is a tiny target only containing a few headers, but I'm open to > > merging it into rtc_base_approved (but not in this CL). If I remove this and > the > > check_includes = false line at line 88 I get these errors: > > > > ERROR at //webrtc/base/checks.h:14:11: Include not allowed. > > #include "webrtc/typedefs.h" > > ^---------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc:webrtc_common > > which should somehow be reachable. > > ___________________ > > ERROR at //webrtc/base/format_macros.h:27:11: Include not allowed. > > #include "webrtc/typedefs.h" > > ^---------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc:webrtc_common > > which should somehow be reachable. > > ___________________ > > ERROR at //webrtc/base/location.h:16:11: Include not allowed. > > #include "webrtc/system_wrappers/include/stringize_macros.h" > > ^------------------------------------------------ > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc/system_wrappers:system_wrappers > > which should somehow be reachable. > > ___________________ > > ERROR at //webrtc/base/md5digest.h:15:11: Include not allowed. > > #include "webrtc/base/messagedigest.h" > > ^-------------------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc/base:rtc_base > > which should somehow be reachable. > > ___________________ > > ERROR at //webrtc/base/onetimeevent.h:15:11: Include not allowed. > > #include "webrtc/typedefs.h" > > ^---------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc:webrtc_common > > which should somehow be reachable. > > ___________________ > > ERROR at //webrtc/base/pathutils.cc:19:11: Include not allowed. > > #include "webrtc/base/fileutils.h" > > ^---------------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc/base:rtc_base > > which should somehow be reachable. > > ___________________ > > ERROR at //webrtc/base/random.h:16:11: Include not allowed. > > #include "webrtc/typedefs.h" > > ^---------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc:webrtc_common > > which should somehow be reachable. > > ___________________ > > ERROR at //webrtc/base/rate_limiter.cc:12:11: Include not allowed. > > #include "webrtc/system_wrappers/include/clock.h" > > ^------------------------------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc/system_wrappers:system_wrappers > > which should somehow be reachable. > > ___________________ > > ERROR at //webrtc/base/rate_statistics.h:17:11: Include not allowed. > > #include "webrtc/typedefs.h" > > ^---------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc:webrtc_common > > which should somehow be reachable. > > Ack. Is there information about this in the bug? (or is this another bug?) The referenced bug only deals with the enabling of GN check. Cleaning this up should be done separately (I'm not aware of any specific bug for webrtc_common and I'm not one is needed unless you're suggesting we merge those headers into rtc_base_approved).
To unblock your work: lgtm But please address the comments below and discuss rtc_base_approved with tommi when he is back. https://codereview.webrtc.org/2717083002/diff/100001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2717083002/diff/100001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:84: # TODO(kjellander): Remove (bugs.webrtc.org/6828) As I have understood it , rtc_base_approved should depend on no other webrtc targets. Can you file a separate bug on rtc_base_approved that says it should not depend on other libs. you can also refer to 3806. and all problems you got when compiling without the deps you had in this Decide with tommi who can/will work on it going forward. https://codereview.webrtc.org/2717083002/diff/100001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:283: deps += [ "..:webrtc_common" ] And add the todo and bug number here as well. Should not depend on webrtc_common. https://codereview.webrtc.org/2717083002/diff/100001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:810: "../system_wrappers:system_wrappers", system_wrappers ? Is that needed? Same todo and bug number here then.
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#newc... webrtc/base/BUILD.gn:86: # :rtc_base_approved -> :rtc_base -> :rtc_base_approved On 2017/04/04 04:23:09, kjellander_webrtc wrote: > On 2017/03/31 08:38:29, tommi (webrtc) wrote: > > where is the dependency from base_approved to base? > > ERROR at //webrtc/base/md5digest.h:15:11: Include not allowed. > #include "webrtc/base/messagedigest.h" > ^-------------------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc/base:rtc_base > which should somehow be reachable. md5digest.h should not be in rtc_base_approved. It's used only in some audio tests, so I'd rather not hack rtc_base_approved for this. > ___________________ > ERROR at //webrtc/base/pathutils.cc:19:11: Include not allowed. > #include "webrtc/base/fileutils.h" > ^---------------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc/base:rtc_base > which should somehow be reachable. I think this include can simply be removed. > > The first one can easily be fixed by moving messagedigest.h to > rtc_base_approved. But the second one requires more work (it was sneaked in > here: > https://codereview.webrtc.org/2533213005/diff/120001/webrtc/base/BUILD.gn). > > Also there are two violations causing a cycle with system_wrappers as well: > > ERROR at //webrtc/base/location.h:16:11: Include not allowed. > #include "webrtc/system_wrappers/include/stringize_macros.h" > ^------------------------------------------------ > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc/system_wrappers:system_wrappers > which should somehow be reachable. > ___________________ > ERROR at //webrtc/base/rate_limiter.cc:12:11: Include not allowed. > #include "webrtc/system_wrappers/include/clock.h" > ^------------------------------------- > It is not in any dependency of > //webrtc/base:rtc_base_approved > The include file is in the target(s): > //webrtc/system_wrappers:system_wrappers > which should somehow be reachable. Can you take a look at those places and see if they really make sense?
kjellander@webrtc.org changed reviewers: + nisse@webrtc.org
Adding nisse@ for more input. It seems easiest if we just merge system_wrappers into rtc_base_approved. But is that going to introduce a lot of new problems for us? I remember getting issues with PNaCl compilation when targets depends on our system_wrappers in Chrome. 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#newc... webrtc/base/BUILD.gn:86: # :rtc_base_approved -> :rtc_base -> :rtc_base_approved On 2017/04/04 09:53:14, tommi ooo (webrtc) wrote: > On 2017/04/04 04:23:09, kjellander_webrtc wrote: > > On 2017/03/31 08:38:29, tommi (webrtc) wrote: > > > where is the dependency from base_approved to base? > > > > ERROR at //webrtc/base/md5digest.h:15:11: Include not allowed. > > #include "webrtc/base/messagedigest.h" > > ^-------------------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc/base:rtc_base > > which should somehow be reachable. > > md5digest.h should not be in rtc_base_approved. It's used only in some audio > tests, so I'd rather not hack rtc_base_approved for this. You're right that it's only used for audio tests. What's your advice on what to do with md5digest.* then? Move to webrtc/test instead? > > ___________________ > > ERROR at //webrtc/base/pathutils.cc:19:11: Include not allowed. > > #include "webrtc/base/fileutils.h" > > ^---------------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc/base:rtc_base > > which should somehow be reachable. > > I think this include can simply be removed. Correct, that was easy! > > The first one can easily be fixed by moving messagedigest.h to > > rtc_base_approved. But the second one requires more work (it was sneaked in > > here: > > https://codereview.webrtc.org/2533213005/diff/120001/webrtc/base/BUILD.gn). > > > > Also there are two violations causing a cycle with system_wrappers as well: > > > > ERROR at //webrtc/base/location.h:16:11: Include not allowed. > > #include "webrtc/system_wrappers/include/stringize_macros.h" > > ^------------------------------------------------ > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc/system_wrappers:system_wrappers > > which should somehow be reachable. > > ___________________ https://cs.chromium.org/chromium/src/third_party/webrtc/system_wrappers/inclu... is simple to move into webrtc/base instead. Should I just incorporate it in rtc_base_approved? It seems borrowed straight of from Chromium's base. > > ERROR at //webrtc/base/rate_limiter.cc:12:11: Include not allowed. > > #include "webrtc/system_wrappers/include/clock.h" > > ^------------------------------------- > > It is not in any dependency of > > //webrtc/base:rtc_base_approved > > The include file is in the target(s): > > //webrtc/system_wrappers:system_wrappers > > which should somehow be reachable. To solve this one, I think we'd have to move clock.h, ntp_time.h and rw_lock_wrapper.h into rtc_base_approved. > Can you take a look at those places and see if they really make sense? See above. https://codereview.webrtc.org/2717083002/diff/100001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2717083002/diff/100001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:84: # TODO(kjellander): Remove (bugs.webrtc.org/6828) On 2017/04/04 09:30:59, perkj_webrtc wrote: > As I have understood it , rtc_base_approved should depend on no other webrtc > targets. > Can you file a separate bug on rtc_base_approved that says it should not depend > on other libs. you can also refer to 3806. > and all problems you got when compiling without the deps you had in this Decide > with tommi who can/will work on it going forward. I can file a bug, but let's wait and see what tommi/nisse responds to in the previous comment threads first.
On 2017/04/10 12:54:02, kjellander_webrtc wrote: > Adding nisse@ for more input. > > It seems easiest if we just merge system_wrappers into rtc_base_approved. I think practically all of system_wrappers is deprecated. So I think it's undesirable with a whole-sale move. > > md5digest.h should not be in rtc_base_approved. It's used only in some audio > > tests, so I'd rather not hack rtc_base_approved for this. > > You're right that it's only used for audio tests. What's your advice on what to > do with md5digest.* then? Move to webrtc/test instead? To keep things simple, I've moved test-only files to the base:rtc_base_test_utils target, without moving files around in the directory tree. > > > ___________________ > > > ERROR at //webrtc/base/pathutils.cc:19:11: Include not allowed. > > > #include "webrtc/base/fileutils.h" > > > ^---------------------- > > > It is not in any dependency of > > > //webrtc/base:rtc_base_approved > > > The include file is in the target(s): > > > //webrtc/base:rtc_base > > > which should somehow be reachable. > > > > I think this include can simply be removed. > > Correct, that was easy! Good! Eventually, I'd like to delete pathutils and most or all of fileutils... > > > The first one can easily be fixed by moving messagedigest.h to > > > rtc_base_approved. But the second one requires more work (it was sneaked in > > > here: > > > https://codereview.webrtc.org/2533213005/diff/120001/webrtc/base/BUILD.gn). > > > > > > Also there are two violations causing a cycle with system_wrappers as well: > > > > > > ERROR at //webrtc/base/location.h:16:11: Include not allowed. > > > #include "webrtc/system_wrappers/include/stringize_macros.h" > > > ^------------------------------------------------ > > > It is not in any dependency of > > > //webrtc/base:rtc_base_approved > > > The include file is in the target(s): > > > //webrtc/system_wrappers:system_wrappers > > > which should somehow be reachable. > > > ___________________ > https://cs.chromium.org/chromium/src/third_party/webrtc/system_wrappers/inclu... > is simple to move into webrtc/base instead. Should I just incorporate it in > rtc_base_approved? > It seems borrowed straight of from Chromium's base. Makes sense to move it to the rtc_base_approved target. Not sure which file it should be in. I think it would be nice to collect compiler-related hacks in a single header file. Currently, some of that is in webrtc/typedefs.h. But moving the stringize_macros.h as is is fine with me too. > > > ERROR at //webrtc/base/rate_limiter.cc:12:11: Include not allowed. > > > #include "webrtc/system_wrappers/include/clock.h" > > > ^------------------------------------- > > > It is not in any dependency of > > > //webrtc/base:rtc_base_approved > > > The include file is in the target(s): > > > //webrtc/system_wrappers:system_wrappers > > > which should somehow be reachable. > > To solve this one, I think we'd have to move clock.h, ntp_time.h and > rw_lock_wrapper.h into rtc_base_approved. clock.h is deprecated. So if it's easy, consider converting rate_limiter.cc to use base/timeutils.h instead. (And then I think rate_limiter.cc, ratelimiter.cc and ratetracker.cc overlap quite a bit, but I haven't looked into what's needed to eliminate one or two of then. I think terelius@ have looked into that a bit more. I guess ntp_time.h should be moved out of system_wrappers, but perhaps not to rtc_base_approved. The dependency of clock.h on ntp_time.h was added in cl https://codereview.webrtc.org/2393723004, before that, the dependency was in the opposite direction. Regarding rw_lock_wrapper.h, I think some kind of rw-lock belongs in rtc_base_approved. We already have sharedexclusivelock.h, I don't know how that differs.
New patch set up that addresses 3 of the 4 violations. I'm responding to review comments here (and will reply to nisse's message in the next post). https://codereview.webrtc.org/2717083002/diff/100001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2717083002/diff/100001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:84: # TODO(kjellander): Remove (bugs.webrtc.org/6828) On 2017/04/10 12:54:02, kjellander_webrtc wrote: > On 2017/04/04 09:30:59, perkj_webrtc wrote: > > As I have understood it , rtc_base_approved should depend on no other webrtc > > targets. > > Can you file a separate bug on rtc_base_approved that says it should not > depend > > on other libs. you can also refer to 3806. > > and all problems you got when compiling without the deps you had in this > Decide > > with tommi who can/will work on it going forward. > > I can file a bug, but let's wait and see what tommi/nisse responds to in the > previous comment threads first. I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=7480 for the remaining work that is not covered in this CL (since my main goal here is to cover webrtc/base in the root .gn file). I updated the TODO with the new bug number instead. https://codereview.webrtc.org/2717083002/diff/100001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:283: deps += [ "..:webrtc_common" ] On 2017/04/04 09:30:59, perkj_webrtc wrote: > And add the todo and bug number here as well. Should not depend on > webrtc_common. I don't see the problem with having dependencies on webrtc_common, almost every project depends on it and it's a really small target: https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=1abcfb59... https://codereview.webrtc.org/2717083002/diff/100001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:810: "../system_wrappers:system_wrappers", On 2017/04/04 09:30:59, perkj_webrtc wrote: > system_wrappers ? Is that needed? Same todo and bug number here then. Yes it depends on system_wrappers. I'm not sure what bug you want me to add, https://bugs.chromium.org/p/webrtc/issues/detail?id=3380 ?
Description was changed from ========== 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 don't attempt to fix that. BUG=webrtc:6828, webrtc:3806 NOTRY=True ========== to ========== 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 don't attempt to fix that. BUG=webrtc:6828, webrtc:3806, webrtc:7480 NOTRY=True ==========
Description was changed from ========== 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 don't attempt to fix that. BUG=webrtc:6828, webrtc:3806, webrtc:7480 NOTRY=True ========== to ========== 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 don't attempt to fix that. 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 BUG=webrtc:6828, webrtc:3806, webrtc:7480 NOTRY=True ==========
On 2017/04/11 07:29:47, nisse-webrtc wrote: > On 2017/04/10 12:54:02, kjellander_webrtc wrote: > > Adding nisse@ for more input. > > > > It seems easiest if we just merge system_wrappers into rtc_base_approved. > > I think practically all of system_wrappers is deprecated. So I think it's > undesirable with a whole-sale move. > > > > md5digest.h should not be in rtc_base_approved. It's used only in some > audio > > > tests, so I'd rather not hack rtc_base_approved for this. > > > > You're right that it's only used for audio tests. What's your advice on what > to > > do with md5digest.* then? Move to webrtc/test instead? > > To keep things simple, I've moved test-only files to the > base:rtc_base_test_utils target, without moving files around in the directory > tree. Thanks, I moved it to rtc_base_test_utils. Seems to work! > > > > ___________________ > > > > ERROR at //webrtc/base/pathutils.cc:19:11: Include not allowed. > > > > #include "webrtc/base/fileutils.h" > > > > ^---------------------- > > > > It is not in any dependency of > > > > //webrtc/base:rtc_base_approved > > > > The include file is in the target(s): > > > > //webrtc/base:rtc_base > > > > which should somehow be reachable. > > > > > > I think this include can simply be removed. > > > > Correct, that was easy! > > Good! Eventually, I'd like to delete pathutils and most or all of fileutils... > > > > > The first one can easily be fixed by moving messagedigest.h to > > > > rtc_base_approved. But the second one requires more work (it was sneaked > in > > > > here: > > > > > https://codereview.webrtc.org/2533213005/diff/120001/webrtc/base/BUILD.gn). > > > > > > > > Also there are two violations causing a cycle with system_wrappers as > well: > > > > > > > > ERROR at //webrtc/base/location.h:16:11: Include not allowed. > > > > #include "webrtc/system_wrappers/include/stringize_macros.h" > > > > ^------------------------------------------------ > > > > It is not in any dependency of > > > > //webrtc/base:rtc_base_approved > > > > The include file is in the target(s): > > > > //webrtc/system_wrappers:system_wrappers > > > > which should somehow be reachable. > > > > ___________________ > > > https://cs.chromium.org/chromium/src/third_party/webrtc/system_wrappers/inclu... > > is simple to move into webrtc/base instead. Should I just incorporate it in > > rtc_base_approved? > > It seems borrowed straight of from Chromium's base. > > Makes sense to move it to the rtc_base_approved target. Not sure which file it > should be in. I think it would be nice to collect compiler-related hacks in a > single header file. Currently, some of that is in webrtc/typedefs.h. But moving > the stringize_macros.h as is is fine with me too. I simply moved it into rtc_base_approved. > > > > ERROR at //webrtc/base/rate_limiter.cc:12:11: Include not allowed. > > > > #include "webrtc/system_wrappers/include/clock.h" > > > > ^------------------------------------- > > > > It is not in any dependency of > > > > //webrtc/base:rtc_base_approved > > > > The include file is in the target(s): > > > > //webrtc/system_wrappers:system_wrappers > > > > which should somehow be reachable. > > > > To solve this one, I think we'd have to move clock.h, ntp_time.h and > > rw_lock_wrapper.h into rtc_base_approved. > > clock.h is deprecated. So if it's easy, consider converting rate_limiter.cc to > use base/timeutils.h instead. (And then I think rate_limiter.cc, ratelimiter.cc > and ratetracker.cc overlap quite a bit, but I haven't looked into what's needed > to eliminate one or two of then. I think terelius@ have looked into that a bit > more. > > I guess ntp_time.h should be moved out of system_wrappers, but perhaps not to > rtc_base_approved. > > The dependency of clock.h on ntp_time.h was added in cl > https://codereview.webrtc.org/2393723004, before that, the dependency was in the > opposite direction. > > Regarding rw_lock_wrapper.h, I think some kind of rw-lock belongs in > rtc_base_approved. We already have sharedexclusivelock.h, I don't know how that > differs. This is too much work for this CL (the whole point was to get GN check enabled for webrtc/base). I'm leaving that for https://bugs.chromium.org/p/webrtc/issues/detail?id=7480
lgtm
Description was changed from ========== 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 don't attempt to fix that. 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 BUG=webrtc:6828, webrtc:3806, webrtc:7480 NOTRY=True ========== to ========== 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 don'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 ==========
tommi: can you have a final look?
Description was changed from ========== 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 don'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 ========== to ========== 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 ==========
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#new... webrtc/base/BUILD.gn:294: deps += [ "..:webrtc_common" ] This is needed by the following files that include webrtc/typedefs.h: //webrtc/base/checks.h:14:11 //webrtc/base/format_macros.h:27:11 //webrtc/base/onetimeevent.h:15:11 //webrtc/base/random.h:16:11 //webrtc/base/rate_limiter.cc:12:11 //webrtc/base/rate_statistics.h:17:11
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#new... webrtc/base/BUILD.gn:294: deps += [ "..:webrtc_common" ] do we still need this even with checks disabled? https://codereview.webrtc.org/2717083002/diff/200001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:294: deps += [ "..:webrtc_common" ] On 2017/04/19 10:58:47, kjellander_webrtc wrote: > This is needed by the following files that include webrtc/typedefs.h: > //webrtc/base/checks.h:14:11 > //webrtc/base/format_macros.h:27:11 > //webrtc/base/onetimeevent.h:15:11 > //webrtc/base/random.h:16:11 > //webrtc/base/rate_limiter.cc:12:11 > //webrtc/base/rate_statistics.h:17:11 Could we instead move typedefs.h to rtc_base_approved?
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#new... webrtc/base/BUILD.gn:294: deps += [ "..:webrtc_common" ] On 2017/04/19 13:45:01, tommi (wëbrtc) wrote: > On 2017/04/19 10:58:47, kjellander_webrtc wrote: > > This is needed by the following files that include webrtc/typedefs.h: > > //webrtc/base/checks.h:14:11 > > //webrtc/base/format_macros.h:27:11 > > //webrtc/base/onetimeevent.h:15:11 > > //webrtc/base/random.h:16:11 > > //webrtc/base/rate_limiter.cc:12:11 > > //webrtc/base/rate_statistics.h:17:11 > > Could we instead move typedefs.h to rtc_base_approved? We could, but that's a massive amount of work considering how many places it's included from, I'd rather not do it here. https://codereview.webrtc.org/2717083002/diff/200001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:294: deps += [ "..:webrtc_common" ] On 2017/04/19 13:45:01, tommi (wëbrtc) wrote: > do we still need this even with checks disabled? No we can just remove it. I just wanted to add what would be otherwise hidden in this CL even if not all the work could be done. Removed in PS#12.
On 2017/04/19 13:57:51, kjellander_webrtc wrote: > 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#new... > webrtc/base/BUILD.gn:294: deps += [ "..:webrtc_common" ] > On 2017/04/19 13:45:01, tommi (wëbrtc) wrote: > > On 2017/04/19 10:58:47, kjellander_webrtc wrote: > > > This is needed by the following files that include webrtc/typedefs.h: > > > //webrtc/base/checks.h:14:11 > > > //webrtc/base/format_macros.h:27:11 > > > //webrtc/base/onetimeevent.h:15:11 > > > //webrtc/base/random.h:16:11 > > > //webrtc/base/rate_limiter.cc:12:11 > > > //webrtc/base/rate_statistics.h:17:11 > > > > Could we instead move typedefs.h to rtc_base_approved? > > We could, but that's a massive amount of work considering how many places it's > included from, I'd rather not do it here. > > https://codereview.webrtc.org/2717083002/diff/200001/webrtc/base/BUILD.gn#new... > webrtc/base/BUILD.gn:294: deps += [ "..:webrtc_common" ] > On 2017/04/19 13:45:01, tommi (wëbrtc) wrote: > > do we still need this even with checks disabled? > > No we can just remove it. I just wanted to add what would be otherwise hidden in > this CL even if not all the work could be done. > Removed in PS#12. Great! thanks and lgtm.
The CQ bit was checked by kjellander@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org, nisse@webrtc.org Link to the patchset: https://codereview.webrtc.org/2717083002/#ps240001 (title: "Rebased")
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": 240001, "attempt_start_ts": 1492616098277470,
"parent_rev": "800daef50a7ae1204a5e2ec77569366d9e7b641a", "commit_rev":
"ed754e71ae8866db641677073274e86fe704eeac"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/ed754e71ae8866db641677073... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/external/webrtc/+/ed754e71ae8866db641677073...
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.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
