Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3001623003:
Decoupling rtc_base from Obj-C code (Closed)
DescriptionDecoupling rtc_base from Obj-C code
The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have
also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743
for more information).
To achieve this we have created 2 targets (rtc_base_objc and rtc_base_generic)
and rtc_base will act as a proxy between these targets (this way we can avoid a
circular dependency between rtc_base_generic and rtc_base_objc).
BUG=webrtc:7743
Review-Url: https://codereview.webrtc.org/3001623003
Cr-Commit-Position: refs/heads/master@{#19661}
Committed: https://chromium.googlesource.com/external/webrtc/+/a0a5b98937de0f0bf90e17011d2cbc8c632cb409
Patch Set 1 #Patch Set 2 : From source_set to static_library #Patch Set 3 : Trying to fix win issue with noop.cc workaround #
Total comments: 5
Patch Set 4 : s/objc_rtc_base/rtc_base_objc #Patch Set 5 : Generate rtc_base_objc only if is_ios || is_mac #Patch Set 6 : Adding comments to explain the noop.{cc,mm} trick #
Total comments: 7
Patch Set 7 : making rtc_base a source_set + addressing comments #Patch Set 8 : Removing noop.{cc,mm} #
Total comments: 3
Messages
Total messages: 25 (12 generated)
Description was changed from ========== Moving rtc_base Obj-C code in another GN target BUG=webrtc:7743 ========== to ========== Moving rtc_base Obj-C code in another GN target. This is a proposal to split rtc_base in two targets and keep different sources in different target. We have already created a CL (https://codereview.webrtc.org/2985453002) to achieve this but it has circular dependencies. In this CL we break the circular dependency by making rtc_base act like a proxy target which will depend on the objc_rtc_base target or the rtc_base_generic one. Here is the proposed dependency tree: rtc_base -- (is_mac || is_ios) -->objc_rtc_base --> rtc_base_generic -- (else) --> rtc_base_generic Note that objc_rtc_base and rtc_base_generic are only visible by rtc_base and that rtc_base is publicly depending on them. This seems to be the only way to break the circular dependency without pushing the objc_rtc_base dependency down to leaf targets (like we did in https://codereview.webrtc.org/2976363002). BUG=webrtc:7743 ==========
mbonadei@webrtc.org changed reviewers: + kjellander@webrtc.org
Description was changed from ========== Moving rtc_base Obj-C code in another GN target. This is a proposal to split rtc_base in two targets and keep different sources in different target. We have already created a CL (https://codereview.webrtc.org/2985453002) to achieve this but it has circular dependencies. In this CL we break the circular dependency by making rtc_base act like a proxy target which will depend on the objc_rtc_base target or the rtc_base_generic one. Here is the proposed dependency tree: rtc_base -- (is_mac || is_ios) -->objc_rtc_base --> rtc_base_generic -- (else) --> rtc_base_generic Note that objc_rtc_base and rtc_base_generic are only visible by rtc_base and that rtc_base is publicly depending on them. This seems to be the only way to break the circular dependency without pushing the objc_rtc_base dependency down to leaf targets (like we did in https://codereview.webrtc.org/2976363002). BUG=webrtc:7743 ========== to ========== Moving rtc_base Obj-C code in another GN target. This is a proposal to split rtc_base in two targets and keep different sources in different targets. We have already created a CL (https://codereview.webrtc.org/2985453002) to achieve this but it has circular dependencies. In this CL we break the circular dependency by making rtc_base act like a proxy target which will depend on the objc_rtc_base target or the rtc_base_generic one. Here is the proposed dependency tree: rtc_base -- (is_mac || is_ios) -->objc_rtc_base --> rtc_base_generic -- (else) --> rtc_base_generic Note that objc_rtc_base and rtc_base_generic are only visible by rtc_base and that rtc_base is publicly depending on them. This seems to be the only way to break the circular dependency without pushing the objc_rtc_base dependency down to leaf targets (like we did in https://codereview.webrtc.org/2976363002). BUG=webrtc:7743 ==========
Description was changed from ========== Moving rtc_base Obj-C code in another GN target. This is a proposal to split rtc_base in two targets and keep different sources in different targets. We have already created a CL (https://codereview.webrtc.org/2985453002) to achieve this but it has circular dependencies. In this CL we break the circular dependency by making rtc_base act like a proxy target which will depend on the objc_rtc_base target or the rtc_base_generic one. Here is the proposed dependency tree: rtc_base -- (is_mac || is_ios) -->objc_rtc_base --> rtc_base_generic -- (else) --> rtc_base_generic Note that objc_rtc_base and rtc_base_generic are only visible by rtc_base and that rtc_base is publicly depending on them. This seems to be the only way to break the circular dependency without pushing the objc_rtc_base dependency down to leaf targets (like we did in https://codereview.webrtc.org/2976363002). BUG=webrtc:7743 ========== to ========== Moving rtc_base Obj-C code in another GN target. This is a proposal to split rtc_base in two targets and keep different sources in different targets. We have already created a CL (https://codereview.webrtc.org/2985453002) to achieve this but it has circular dependencies. In this CL we break the circular dependency by making rtc_base act like a proxy target which will depend on the objc_rtc_base target or the rtc_base_generic one. Here is the proposed dependency tree: rtc_base -- (is_mac || is_ios) --> objc_rtc_base --> rtc_base_generic -- (else) --> rtc_base_generic Note that objc_rtc_base and rtc_base_generic are only visible by rtc_base and that rtc_base is publicly depending on them. This seems to be the only way to break the circular dependency without pushing the objc_rtc_base dependency down to leaf targets (like we did in https://codereview.webrtc.org/2976363002). BUG=webrtc:7743 ==========
Description was changed from ========== Moving rtc_base Obj-C code in another GN target. This is a proposal to split rtc_base in two targets and keep different sources in different targets. We have already created a CL (https://codereview.webrtc.org/2985453002) to achieve this but it has circular dependencies. In this CL we break the circular dependency by making rtc_base act like a proxy target which will depend on the objc_rtc_base target or the rtc_base_generic one. Here is the proposed dependency tree: rtc_base -- (is_mac || is_ios) --> objc_rtc_base --> rtc_base_generic -- (else) --> rtc_base_generic Note that objc_rtc_base and rtc_base_generic are only visible by rtc_base and that rtc_base is publicly depending on them. This seems to be the only way to break the circular dependency without pushing the objc_rtc_base dependency down to leaf targets (like we did in https://codereview.webrtc.org/2976363002). BUG=webrtc:7743 ========== to ========== Moving rtc_base Obj-C code in another GN target. This is a proposal to split rtc_base in two targets and keep different sources in different targets. We have already created a CL (https://codereview.webrtc.org/2985453002) to achieve this but it has circular dependencies. In this CL we break the circular dependency by making rtc_base act like a proxy target which will depend on the objc_rtc_base target or the rtc_base_generic one. Here is the proposed dependency tree: rtc_base -- (is_mac || is_ios) --> objc_rtc_base --> rtc_base_generic -- (else) --> rtc_base_generic Note that objc_rtc_base and rtc_base_generic are only visible by rtc_base and that rtc_base is publicly depending on them. This seems to be the only way to break the circular dependency without pushing the objc_rtc_base dependency down to leaf targets (like we did in https://codereview.webrtc.org/2976363002). BUG=webrtc:7743 ==========
https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:419: rtc_static_library("objc_rtc_base") { How about rtc_base_objc? https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:539: visibility = [ How about visibility = [ ":*" ] to make maintenance a little easier. Then move it up a few steps to make it consistent with most other targets.
https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:419: rtc_static_library("objc_rtc_base") { On 2017/08/14 14:00:36, kjellander_webrtc wrote: > How about rtc_base_objc? Seems good to me. I also prefer the _objc suffix (I will update all the other CLs in the next days). https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:539: visibility = [ On 2017/08/14 14:00:36, kjellander_webrtc wrote: > How about > visibility = [ ":*" ] > to make maintenance a little easier. > Then move it up a few steps to make it consistent with most other targets. I would like to keep it as private as possible because this is the real rtc_base and I would like to avoid that a target depends on this one skipping the proxy.
On 2017/08/14 15:07:35, mbonadei wrote: > https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn > File webrtc/rtc_base/BUILD.gn (right): > > https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn#... > webrtc/rtc_base/BUILD.gn:419: rtc_static_library("objc_rtc_base") { > On 2017/08/14 14:00:36, kjellander_webrtc wrote: > > How about rtc_base_objc? > > Seems good to me. I also prefer the _objc suffix (I will update all the other > CLs in the next days). > > https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn#... > webrtc/rtc_base/BUILD.gn:539: visibility = [ > On 2017/08/14 14:00:36, kjellander_webrtc wrote: > > How about > > visibility = [ ":*" ] > > to make maintenance a little easier. > > Then move it up a few steps to make it consistent with most other targets. > > I would like to keep it as private as possible because this is the real rtc_base > and I would like to avoid that a target depends on this one skipping the > proxy. This seems like a great solution to me. lgtm
lgtm https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3001623003/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:539: visibility = [ On 2017/08/14 15:07:34, mbonadei wrote: > On 2017/08/14 14:00:36, kjellander_webrtc wrote: > > How about > > visibility = [ ":*" ] > > to make maintenance a little easier. > > Then move it up a few steps to make it consistent with most other targets. > > I would like to keep it as private as possible because this is the real rtc_base > and I would like to avoid that a target depends on this one skipping the > proxy. Fair enough.
Description was changed from ========== Moving rtc_base Obj-C code in another GN target. This is a proposal to split rtc_base in two targets and keep different sources in different targets. We have already created a CL (https://codereview.webrtc.org/2985453002) to achieve this but it has circular dependencies. In this CL we break the circular dependency by making rtc_base act like a proxy target which will depend on the objc_rtc_base target or the rtc_base_generic one. Here is the proposed dependency tree: rtc_base -- (is_mac || is_ios) --> objc_rtc_base --> rtc_base_generic -- (else) --> rtc_base_generic Note that objc_rtc_base and rtc_base_generic are only visible by rtc_base and that rtc_base is publicly depending on them. This seems to be the only way to break the circular dependency without pushing the objc_rtc_base dependency down to leaf targets (like we did in https://codereview.webrtc.org/2976363002). BUG=webrtc:7743 ========== to ========== The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743 for more information). To achieve this we have created 2 targets (rtc_base_objc and rtc_base_generic) and rtc_base will act as a proxy between these targets (this way we can avoid a circular dependency between rtc_base_generic and rtc_base_objc). BUG=webrtc:7743 ==========
Description was changed from ========== The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743 for more information). To achieve this we have created 2 targets (rtc_base_objc and rtc_base_generic) and rtc_base will act as a proxy between these targets (this way we can avoid a circular dependency between rtc_base_generic and rtc_base_objc). BUG=webrtc:7743 ========== to ========== Decoupling rtc_base from Obj-C code The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743 for more information). To achieve this we have created 2 targets (rtc_base_objc and rtc_base_generic) and rtc_base will act as a proxy between these targets (this way we can avoid a circular dependency between rtc_base_generic and rtc_base_objc). BUG=webrtc:7743 ==========
mbonadei@webrtc.org changed reviewers: + kwiberg@webrtc.org
+kwiberg@ for OWNER lgtm.
https://codereview.webrtc.org/3001623003/diff/100001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3001623003/diff/100001/webrtc/rtc_base/BUILD.gn... webrtc/rtc_base/BUILD.gn:401: rtc_static_library("rtc_base") { If you made this a source set, would the need for no-op file go away? https://codereview.webrtc.org/3001623003/diff/100001/webrtc/rtc_base/BUILD.gn... webrtc/rtc_base/BUILD.gn:418: ] I'm confused. In the else clause, we *aren't* on macOS or iOS, so why add a no-op source file? And why have different types of no-op files for the two cases? https://codereview.webrtc.org/3001623003/diff/100001/webrtc/rtc_base/BUILD.gn... webrtc/rtc_base/BUILD.gn:421: ] This one could be lifted out of the if-else, right? (And if you did, could rtc_base_objc have a regular dep instead of a public dep on rtc_base_generic?)
PTAL. https://codereview.webrtc.org/3001623003/diff/100001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3001623003/diff/100001/webrtc/rtc_base/BUILD.gn... webrtc/rtc_base/BUILD.gn:401: rtc_static_library("rtc_base") { On 2017/08/31 20:46:14, kwiberg-webrtc wrote: > If you made this a source set, would the need for no-op file go away? In the next patch set I have switched to rtc_source_set. Please note that there are some differences (especially regarding how the linker works on them): source_set: https://chromium.googlesource.com/chromium/src/tools/gn/+/HEAD/docs/reference... static_library: https://chromium.googlesource.com/chromium/src/tools/gn/+/HEAD/docs/reference... If you confirm that a source_set is a good to have I am happy to avoid noop files (and I will review other CLs too). https://codereview.webrtc.org/3001623003/diff/100001/webrtc/rtc_base/BUILD.gn... webrtc/rtc_base/BUILD.gn:418: ] On 2017/08/31 20:46:14, kwiberg-webrtc wrote: > I'm confused. In the else clause, we *aren't* on macOS or iOS, so why add a > no-op source file? And why have different types of no-op files for the two > cases? Oh wait... Yes you are right! In similar CLs I avoided to add the noop file if we are not targeting iOS or macOS. This is probably a leftover that I copy/pasted and refactored to look symmetric. :/ Good catch! Thanks! In the next patch set I switched rtc_base from rtc_static_library to rtc_source_set so probably we can drop both noop.{mm,cc}. https://codereview.webrtc.org/3001623003/diff/100001/webrtc/rtc_base/BUILD.gn... webrtc/rtc_base/BUILD.gn:421: ] On 2017/08/31 20:46:14, kwiberg-webrtc wrote: > This one could be lifted out of the if-else, right? (And if you did, could > rtc_base_objc have a regular dep instead of a public dep on rtc_base_generic?) This is the right thing to do. I was too focused on breaking the circular dependency that I overlooked this. It is included in the next patch set. Thanks!
https://codereview.webrtc.org/3001623003/diff/140001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3001623003/diff/140001/webrtc/rtc_base/BUILD.gn... webrtc/rtc_base/BUILD.gn:444: "thread_darwin.mm", Good news: applefilesystem.mm has been removed in https://codereview.webrtc.org/2995413002! https://codereview.webrtc.org/3001623003/diff/140001/webrtc/rtc_base/BUILD.gn... webrtc/rtc_base/BUILD.gn:453: rtc_source_set("rtc_base_generic") { I also switched this to source_set (it is not clean to me if it makes sense or not, so please let me know).
lgtm https://codereview.webrtc.org/3001623003/diff/100001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3001623003/diff/100001/webrtc/rtc_base/BUILD.gn... webrtc/rtc_base/BUILD.gn:401: rtc_static_library("rtc_base") { On 2017/09/01 09:24:35, mbonadei wrote: > On 2017/08/31 20:46:14, kwiberg-webrtc wrote: > > If you made this a source set, would the need for no-op file go away? > > In the next patch set I have switched to rtc_source_set. Please note that > there are some differences (especially regarding how the linker works on > them): > > source_set: > https://chromium.googlesource.com/chromium/src/tools/gn/+/HEAD/docs/reference... > static_library: > https://chromium.googlesource.com/chromium/src/tools/gn/+/HEAD/docs/reference... > > If you confirm that a source_set is a good to have I am happy to avoid noop > files (and I will review other CLs too). The default Chromium advice is to use a source set unless you know you need a static library. And in this case in particular, almost all the "weight" of this target is in rtc_base_generic, which is already a static library, so I think a source set is a good fit. https://codereview.webrtc.org/3001623003/diff/140001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3001623003/diff/140001/webrtc/rtc_base/BUILD.gn... webrtc/rtc_base/BUILD.gn:453: rtc_source_set("rtc_base_generic") { On 2017/09/01 09:28:23, mbonadei wrote: > I also switched this to source_set (it is not clean to me if it makes sense or > not, so please let me know). I think the main effect will be to reduce link time. I don't have a sophisticated sense of the ramifications, however. Try it!
The CQ bit was checked by mbonadei@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/3001623003/#ps140001 (title: "Removing noop.{cc,mm}")
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": 140001, "attempt_start_ts": 1504523058200250,
"parent_rev": "0c88a504120738f1d7371bfbf748b17bab7be81d", "commit_rev":
"a0a5b98937de0f0bf90e17011d2cbc8c632cb409"}
Message was sent while issue was closed.
Description was changed from ========== Decoupling rtc_base from Obj-C code The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743 for more information). To achieve this we have created 2 targets (rtc_base_objc and rtc_base_generic) and rtc_base will act as a proxy between these targets (this way we can avoid a circular dependency between rtc_base_generic and rtc_base_objc). BUG=webrtc:7743 ========== to ========== Decoupling rtc_base from Obj-C code The goal of this CL is to separate Obj-C/Obj-C++ code from targets which have also C++ code (see https://bugs.chromium.org/p/webrtc/issues/detail?id=7743 for more information). To achieve this we have created 2 targets (rtc_base_objc and rtc_base_generic) and rtc_base will act as a proxy between these targets (this way we can avoid a circular dependency between rtc_base_generic and rtc_base_objc). BUG=webrtc:7743 Review-Url: https://codereview.webrtc.org/3001623003 Cr-Commit-Position: refs/heads/master@{#19661} Committed: https://chromium.googlesource.com/external/webrtc/+/a0a5b98937de0f0bf90e17011... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/a0a5b98937de0f0bf90e17011...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.webrtc.org/3008103002/ by mbonadei@webrtc.org. The reason for reverting is: It breaks a downstream project.. |
