|
|
DescriptionAdd field_trial_default dependency to libjingle_peerconnection
This is needed for webrtc::field_trial::FindFullName in peerconnection.cc
NOTRY=True
Committed: https://crrev.com/a7a01df2aebe7108afad208ccd0341c2f0bc7b3b
Cr-Commit-Position: refs/heads/master@{#13836}
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 3
Patch Set 3 #Messages
Total messages: 34 (13 generated)
arlolra@gmail.com changed reviewers: + pthatcher@webrtc.org
pthatcher@chromium.org changed reviewers: + kjellander@chromium.org, pthatcher@chromium.org
I'm fine with this if kjellander is.
Description was changed from ========== For webrtc::field_trial::FindFullName in peerconnection.cc ========== to ========== For webrtc::field_trial::FindFullName in peerconnection.cc NOTRY=True ==========
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
This can land with NOTRY=True if the trybots I fired are happy. https://codereview.webrtc.org/2120673004/diff/1/webrtc/api/api.gyp File webrtc/api/api.gyp (right): https://codereview.webrtc.org/2120673004/diff/1/webrtc/api/api.gyp#newcode103 webrtc/api/api.gyp:103: '<(webrtc_root)/system_wrappers/system_wrappers.gyp:field_trial_default', I verified FindFullName is invoked in peerconnection.cc.
kjellander@webrtc.org changed reviewers: - kjellander@chromium.org
lgtm
Description was changed from ========== For webrtc::field_trial::FindFullName in peerconnection.cc NOTRY=True ========== to ========== Add field_trial_default dependency to libjingle_peerconnection This is needed for webrtc::field_trial::FindFullName in peerconnection.cc NOTRY=True ==========
Description was changed from ========== Add field_trial_default dependency to libjingle_peerconnection This is needed for webrtc::field_trial::FindFullName in peerconnection.cc NOTRY=True ========== to ========== Add field_trial_default dependency to libjingle_peerconnection This is needed for webrtc::field_trial::FindFullName in peerconnection.cc ==========
Actually, this does not lgtm yet, you should do the same for the GN target as well: https://cs.chromium.org/chromium/src/third_party/webrtc/api/BUILD.gn?rcl=0&l=201
Thanks! Hopefully I didn't overstep with the changes there.
On 2016/08/17 15:50:10, arlolra wrote: > Thanks! Hopefully I didn't overstep with the changes there. Please revert some of them. See comments.
On 2016/08/17 16:49:24, kjellander_webrtc wrote: > On 2016/08/17 15:50:10, arlolra wrote: > > Thanks! Hopefully I didn't overstep with the changes there. > > Please revert some of them. See comments. Which comments? Can you elaborate. "../system_wrappers:field_trial_default", is redundant with "../call", so should I omit that? And is putting the more specific targets as in api.gyp wrong in BUILD.gn? I would think these should match up.
Sorry! I forgot to post my comment drafts in the last round. https://codereview.webrtc.org/2120673004/diff/20001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (left): https://codereview.webrtc.org/2120673004/diff/20001/webrtc/api/BUILD.gn#oldco... webrtc/api/BUILD.gn:114: "../call", You're removing the dependency on ../call here. Please keep it around (it's one of the few targets that don't have a proper 1:1 mapping between GYP+GN. https://codereview.webrtc.org/2120673004/diff/20001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2120673004/diff/20001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:114: "../media:rtc_media", This change does nothing, the default group in media includes only rtc_media, so I'd prefer not touching this line. https://codereview.webrtc.org/2120673004/diff/20001/webrtc/api/BUILD.gn#newco... webrtc/api/BUILD.gn:115: "../pc:rtc_pc", Same for this.
On 2016/08/18 10:22:40, kjellander_webrtc wrote: > Sorry! I forgot to post my comment drafts in the last round. No problem. I reverted the unnecessary changes. Thanks.
Description was changed from ========== Add field_trial_default dependency to libjingle_peerconnection This is needed for webrtc::field_trial::FindFullName in peerconnection.cc ========== to ========== Add field_trial_default dependency to libjingle_peerconnection This is needed for webrtc::field_trial::FindFullName in peerconnection.cc NOTRY=True ==========
On 2016/08/20 14:51:13, arlolra wrote: > On 2016/08/18 10:22:40, kjellander_webrtc wrote: > > Sorry! I forgot to post my comment drafts in the last round. > > No problem. I reverted the unnecessary changes. Thanks. Thanks. I'll land this for you.
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
A disapproval has been posted by following reviewers: kjellander@webrtc.org. Please make sure to get positive review before another CQ attempt.
On 2016/08/22 06:34:49, commit-bot: I haz the power wrote: > A disapproval has been posted by following reviewers: mailto:kjellander@webrtc.org. > Please make sure to get positive review before another CQ attempt. lgtm
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/...
Message was sent while issue was closed.
Description was changed from ========== Add field_trial_default dependency to libjingle_peerconnection This is needed for webrtc::field_trial::FindFullName in peerconnection.cc NOTRY=True ========== to ========== Add field_trial_default dependency to libjingle_peerconnection This is needed for webrtc::field_trial::FindFullName in peerconnection.cc NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add field_trial_default dependency to libjingle_peerconnection This is needed for webrtc::field_trial::FindFullName in peerconnection.cc NOTRY=True ========== to ========== Add field_trial_default dependency to libjingle_peerconnection This is needed for webrtc::field_trial::FindFullName in peerconnection.cc NOTRY=True Committed: https://crrev.com/a7a01df2aebe7108afad208ccd0341c2f0bc7b3b Cr-Commit-Position: refs/heads/master@{#13836} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a7a01df2aebe7108afad208ccd0341c2f0bc7b3b Cr-Commit-Position: refs/heads/master@{#13836}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/2263063002/ by sakal@webrtc.org. The reason for reverting is: Breaks chromium..
Message was sent while issue was closed.
On 2016/08/22 07:25:50, sakal wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.webrtc.org/2263063002/ by mailto:sakal@webrtc.org. > > The reason for reverting is: Breaks chromium.. Well that's sad. I assume the reason is that, "../call", "../system_wrappers:field_trial_default", is redundant in BUILD.gn Can we try again w/ just "../call" this time? Or have I lost too much capital?
Message was sent while issue was closed.
On 2016/08/22 07:25:50, sakal wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.webrtc.org/2263063002/ by mailto:sakal@webrtc.org. > > The reason for reverting is: Breaks chromium.. sakal: please provide a link to a breakage in the revert message. Here's what broke: https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Builder%2... FAILED: lib_content_browsertests__library.so lib_content_browsertests__library.so.TOC lib.unstripped/lib_content_browsertests__library.so python "/mnt/data/b/c/b/Android_Builder__dbg_/src/build/toolchain/gcc_solink_wrapper.py" --readelf="../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-readelf" --nm="../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-nm" --strip=../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-strip --sofile="./lib.unstripped/lib_content_browsertests__library.so" --tocfile="./lib_content_browsertests__library.so.TOC" --output="./lib_content_browsertests__library.so" -- ../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -shared -Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -fuse-ld=gold -Wl,--icf=all -Wl,--build-id=sha1 -Wl,--no-undefined -Wl,--exclude-libs=libgcc.a -Wl,--exclude-libs=libc++_static.a -Wl,--exclude-libs=libvpx_assembly_arm.a -Wl,--warn-shared-textrel -Wl,-O1 -Wl,--as-needed -nostdlib -Wl,--warn-shared-textrel --sysroot=../../third_party/android_tools/ndk/platforms/android-16/arch-arm -Wl,--version-script=/mnt/data/b/c/b/Android_Builder__dbg_/src/build/android/android_no_jni_exports.lst -Wl,-wrap,calloc -Wl,-wrap,free -Wl,-wrap,malloc -Wl,-wrap,memalign -Wl,-wrap,posix_memalign -Wl,-wrap,pvalloc -Wl,-wrap,realloc -Wl,-wrap,valloc -L../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libs/armeabi-v7a -o "./lib.unstripped/lib_content_browsertests__library.so" -Wl,-soname="lib_content_browsertests__library.so" @"./lib_content_browsertests__library.so.rsp" /mnt/data/b/c/b/Android_Builder__dbg_/src/third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld.gold: error: obj/third_party/libjingle/libjingle.a(field_trial.o): multiple definition of 'webrtc::field_trial::FindFullName(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)' /mnt/data/b/c/b/Android_Builder__dbg_/src/third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld.gold: obj/third_party/webrtc/system_wrappers/field_trial_default/field_trial_default.o: previous definition here https://cs.chromium.org/chromium/src/third_party/libjingle/BUILD.gn?rcl=0&l=70 Ideally we would just wait for further cleanup in https://bugs.chromium.org/p/webrtc/issues/detail?id=4256 to happen before attempting to reland this. Or if you can figure out another way to avoid breaking Chromium?
Message was sent while issue was closed.
Isn't the solution just to remove, ``` } else { # Otherwise, we just add the field_trial which redirects to base. sources = [ "../webrtc_overrides/field_trial.cc", ] } ``` here https://cs.chromium.org/chromium/src/third_party/libjingle/BUILD.gn?rcl=0&l=70 since the symbol is already defined with this patch? That seems to work for the `is_nacl` case.
Message was sent while issue was closed.
On 2016/08/22 15:21:28, arlolra wrote: > Isn't the solution just to remove, > > ``` > } else { > # Otherwise, we just add the field_trial which redirects to base. > sources = [ > "../webrtc_overrides/field_trial.cc", > ] > } > ``` > > here > https://cs.chromium.org/chromium/src/third_party/libjingle/BUILD.gn?rcl=0&l=70 > > since the symbol is already defined with this patch? > > That seems to work for the `is_nacl` case. I wasn't fully aware of how the field trial stuff worked before. The idea is that you provide either your own implementation (to allow hooking up your own statistics) or you link with one of the default implementations, which is not linked by default for any of our production code (to provide the flexibility to downstream users). This is also being reworked a little recently in https://codereview.webrtc.org/2403463002/. I'm afraid this is not well documented, which is also something that could be improved. |