Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2001913002:
Split TaskQueue out into a separate build target. (Closed)
|
Created:
4 years, 7 months ago by tommi Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc, kjellander_webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSplit TaskQueue out into a separate build target.
This is needed as there are targets such as newlib_pnacl/remoting_client_plugin_newlib.pexe that depend on rtc_base_approved but don't need TaskQueue. We could implement support for TaskQueue in nacl using ppapi types, but it looks like there isn't a need for it. Libevent isn't supported for nacl either, so I'm introducing a layer on top of rtc_base_approved for TaskQueue. It's conceivable that this target will morph into a target that holds other threading primitives such as platform_thread and possibly socket related operations, which is also an area that we currently #ifdef out for nacl in a few places.
Functional change: Removes the "is_nacl" check.
R=phoglund@webrtc.org
Committed: https://crrev.com/09c60da70439c2dfd50c0aa377594d371d8be9b0
Cr-Commit-Position: refs/heads/master@{#12878}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Address comments #Patch Set 3 : More cleanup #Patch Set 4 : Remove comment #Patch Set 5 : Fix GN build #
Messages
Total messages: 36 (14 generated)
Patrik - I simplified the GN files to only have the previously-global flags for libevent, in the base/BUILD.gn file. The global flags for gyp continue to exist for compatibility with downstream repos.
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001913002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001913002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_dbg/builds/11000)
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001913002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001913002/20001
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
Description was changed from ========== Split TaskQueue out into a separate build target. This is needed as there are targets such as newlib_pnacl/remoting_client_plugin_newlib.pexe that depend on rtc_base_approved but don't need TaskQueue. We could implement support for TaskQueue in nacl using ppapi types, but it looks like there isn't a need for it. Libevent isn't supported for nacl either, so I'm introducing a layer on top of rtc_base_approved for TaskQueue. It's conceivable that this target will morph into a target that holds other threading primitives such as platform_thread and possibly socket related operations, which is also an area that we currently #ifdef out for nacl in a few places. R=phoglund@webrtc.org ========== to ========== Split TaskQueue out into a separate build target. This is needed as there are targets such as newlib_pnacl/remoting_client_plugin_newlib.pexe that depend on rtc_base_approved but don't need TaskQueue. We could implement support for TaskQueue in nacl using ppapi types, but it looks like there isn't a need for it. Libevent isn't supported for nacl either, so I'm introducing a layer on top of rtc_base_approved for TaskQueue. It's conceivable that this target will morph into a target that holds other threading primitives such as platform_thread and possibly socket related operations, which is also an area that we currently #ifdef out for nacl in a few places. Functional change: Removes the "is_nacl" check. R=phoglund@webrtc.org ==========
https://codereview.webrtc.org/2001913002/diff/20001/webrtc/build/webrtc.gni File webrtc/build/webrtc.gni (left): https://codereview.webrtc.org/2001913002/diff/20001/webrtc/build/webrtc.gni#o... webrtc/build/webrtc.gni:62: if (is_win || is_mac || is_ios || is_nacl) { this 'is_nacl' check is now gone.
I understand why you want to make the simplifications, but we're putting a high premium on keeping the gyp and gn structure the same. I don't object to the new task queue layer but please keep the variables in gn. https://codereview.webrtc.org/2001913002/diff/20001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2001913002/diff/20001/webrtc/base/BUILD.gn#newc... webrtc/base/BUILD.gn:202: defines += [ "WEBRTC_BUILD_LIBEVENT" ] Please move this back where it was; we need GN and GYP to look the same. Also this is brittle since the define needs to be on all targets that include task_queue.h (or we hit the #error in there). https://codereview.webrtc.org/2001913002/diff/20001/webrtc/base/BUILD.gn#newc... webrtc/base/BUILD.gn:203: deps += [ "//base/third_party/libevent" ] This no longer respects rtc_build_libevent which means you'll break downstream. https://codereview.webrtc.org/2001913002/diff/20001/webrtc/build/webrtc.gni File webrtc/build/webrtc.gni (left): https://codereview.webrtc.org/2001913002/diff/20001/webrtc/build/webrtc.gni#o... webrtc/build/webrtc.gni:62: if (is_win || is_mac || is_ios || is_nacl) { On 2016/05/21 20:10:04, tommi-webrtc wrote: > this 'is_nacl' check is now gone. Please put this back, we need gn and gyp to be as similar as possible.
Address comments
More cleanup
Remove comment
https://codereview.webrtc.org/2001913002/diff/20001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2001913002/diff/20001/webrtc/base/BUILD.gn#newc... webrtc/base/BUILD.gn:202: defines += [ "WEBRTC_BUILD_LIBEVENT" ] On 2016/05/23 08:23:32, phoglund wrote: > Please move this back where it was; we need GN and GYP to look the same. Also > this is brittle since the define needs to be on all targets that include > task_queue.h (or we hit the #error in there). Done. https://codereview.webrtc.org/2001913002/diff/20001/webrtc/build/webrtc.gni File webrtc/build/webrtc.gni (left): https://codereview.webrtc.org/2001913002/diff/20001/webrtc/build/webrtc.gni#o... webrtc/build/webrtc.gni:62: if (is_win || is_mac || is_ios || is_nacl) { On 2016/05/23 08:23:32, phoglund wrote: > On 2016/05/21 20:10:04, tommi-webrtc wrote: > > this 'is_nacl' check is now gone. > > Please put this back, we need gn and gyp to be as similar as possible. Done. I restored this but left the 'is_nacl' check out.
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001913002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001913002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/1...) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...)
Fix GN build
The CQ bit was checked by tommi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001913002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001913002/100001
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
ping phoglund
lgtm
The CQ bit was checked by tommi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001913002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001913002/100001
Message was sent while issue was closed.
Description was changed from ========== Split TaskQueue out into a separate build target. This is needed as there are targets such as newlib_pnacl/remoting_client_plugin_newlib.pexe that depend on rtc_base_approved but don't need TaskQueue. We could implement support for TaskQueue in nacl using ppapi types, but it looks like there isn't a need for it. Libevent isn't supported for nacl either, so I'm introducing a layer on top of rtc_base_approved for TaskQueue. It's conceivable that this target will morph into a target that holds other threading primitives such as platform_thread and possibly socket related operations, which is also an area that we currently #ifdef out for nacl in a few places. Functional change: Removes the "is_nacl" check. R=phoglund@webrtc.org ========== to ========== Split TaskQueue out into a separate build target. This is needed as there are targets such as newlib_pnacl/remoting_client_plugin_newlib.pexe that depend on rtc_base_approved but don't need TaskQueue. We could implement support for TaskQueue in nacl using ppapi types, but it looks like there isn't a need for it. Libevent isn't supported for nacl either, so I'm introducing a layer on top of rtc_base_approved for TaskQueue. It's conceivable that this target will morph into a target that holds other threading primitives such as platform_thread and possibly socket related operations, which is also an area that we currently #ifdef out for nacl in a few places. Functional change: Removes the "is_nacl" check. R=phoglund@webrtc.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Split TaskQueue out into a separate build target. This is needed as there are targets such as newlib_pnacl/remoting_client_plugin_newlib.pexe that depend on rtc_base_approved but don't need TaskQueue. We could implement support for TaskQueue in nacl using ppapi types, but it looks like there isn't a need for it. Libevent isn't supported for nacl either, so I'm introducing a layer on top of rtc_base_approved for TaskQueue. It's conceivable that this target will morph into a target that holds other threading primitives such as platform_thread and possibly socket related operations, which is also an area that we currently #ifdef out for nacl in a few places. Functional change: Removes the "is_nacl" check. R=phoglund@webrtc.org ========== to ========== Split TaskQueue out into a separate build target. This is needed as there are targets such as newlib_pnacl/remoting_client_plugin_newlib.pexe that depend on rtc_base_approved but don't need TaskQueue. We could implement support for TaskQueue in nacl using ppapi types, but it looks like there isn't a need for it. Libevent isn't supported for nacl either, so I'm introducing a layer on top of rtc_base_approved for TaskQueue. It's conceivable that this target will morph into a target that holds other threading primitives such as platform_thread and possibly socket related operations, which is also an area that we currently #ifdef out for nacl in a few places. Functional change: Removes the "is_nacl" check. R=phoglund@webrtc.org Committed: https://crrev.com/09c60da70439c2dfd50c0aa377594d371d8be9b0 Cr-Commit-Position: refs/heads/master@{#12878} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/09c60da70439c2dfd50c0aa377594d371d8be9b0 Cr-Commit-Position: refs/heads/master@{#12878} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
