Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412003007:
Remove system_wrappers dep from field_trial_default and metrics_default. (Closed)
|
Created:
5 years, 1 month ago by Sergey Ulanov Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, Andrew MacDonald, peah-webrtc, mflodman, henrika_webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove system_wrappers dep from field_trial_default and metrics_default.
field_trial_default and metrics_default don't use system_wrappers and
don't need to depend on it. The dependency on field_trial_default was
added in libjingle in crrev.com/356135 and that broke compilation of
libjingle for NaCl with GN because system_wrappers currently doesn't
compile for NaCl.
TBR=niklas.enbom@webrtc.org
Committed: https://crrev.com/5ab193fd8d38bcbbe2778cb177096ed0fad868fc
Cr-Commit-Position: refs/heads/master@{#10495}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : #
Messages
Total messages: 29 (11 generated)
Description was changed from ========== Remove system_wrappers dep from field_trial_default and metrics_default. feld_trial_default and metrics_default don't use system_wrappers and don't need to depende on it. This was breaking compiation of libjingle for NaCl with GN because system_wrappers currently doesn't compile for NaCl. ========== to ========== Remove system_wrappers dep from field_trial_default and metrics_default. field_trial_default and metrics_default don't use system_wrappers and don't need to depende on it. The dependency on field_trial_default was added in libjingle in crrev.com/356135 and that broke compilation of libjingle for NaCl with GN because system_wrappers currently doesn't compile for NaCl. ==========
sergeyu@chromium.org changed reviewers: + guoweis@webrtc.org, kjellander@webrtc.org
could you also clean up system_wrappers.gyp? https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.gn File webrtc/system_wrappers/BUILD.gn (right): https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.... webrtc/system_wrappers/BUILD.gn:198: ":system_wrappers_default", IS this right? Shouldn't this be system_wrappers?
https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.gn File webrtc/system_wrappers/BUILD.gn (right): https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.... webrtc/system_wrappers/BUILD.gn:198: ":system_wrappers_default", On 2015/10/27 23:08:07, guoweis wrote: > IS this right? Shouldn't this be system_wrappers? Yes, thanks for catching it
Like guoweis said, please update system_wrappers.gyp also. https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.gn File webrtc/system_wrappers/BUILD.gn (left): https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.... webrtc/system_wrappers/BUILD.gn:27: "interface/field_trial.h", Not that it actually matters to the build, but wouldn't it be nicer to move this file into the field_trial_default target if you're removing the dependency on system_wrappers? (since it's included in field_trial_default.cc)
Description was changed from ========== Remove system_wrappers dep from field_trial_default and metrics_default. field_trial_default and metrics_default don't use system_wrappers and don't need to depende on it. The dependency on field_trial_default was added in libjingle in crrev.com/356135 and that broke compilation of libjingle for NaCl with GN because system_wrappers currently doesn't compile for NaCl. ========== to ========== Remove system_wrappers dep from field_trial_default and metrics_default. field_trial_default and metrics_default don't use system_wrappers and don't need to depend on it. The dependency on field_trial_default was added in libjingle in crrev.com/356135 and that broke compilation of libjingle for NaCl with GN because system_wrappers currently doesn't compile for NaCl. ==========
On 2015/10/28 13:48:43, kjellander (webrtc) wrote: > Like guoweis said, please update system_wrappers.gyp also. > > https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.gn > File webrtc/system_wrappers/BUILD.gn (left): > > https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.... > webrtc/system_wrappers/BUILD.gn:27: "interface/field_trial.h", > Not that it actually matters to the build, but wouldn't it be nicer to move this > file into the field_trial_default target if you're removing the dependency on > system_wrappers? > (since it's included in field_trial_default.cc) To be more specific, in system_wrappers.gyp, metrics_default still depends on system_wrappers and should be changed to match this change.
Updated GYP as well now. https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.gn File webrtc/system_wrappers/BUILD.gn (left): https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.... webrtc/system_wrappers/BUILD.gn:27: "interface/field_trial.h", On 2015/10/28 13:48:43, kjellander (webrtc) wrote: > Not that it actually matters to the build, but wouldn't it be nicer to move this > file into the field_trial_default target if you're removing the dependency on > system_wrappers? > (since it's included in field_trial_default.cc) I think this file should stay in system_wrappers as it defines functions to be used in other parts of webrtc. As I discussed with Guo-wei we can avoid compile-time dependencies on external functions like this and replace these with runtime dependency injection. So field_trial_default and metrics_default can be removed in the future..
On 2015/10/30 22:57:15, Sergey Ulanov wrote: > Updated GYP as well now. > > https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.gn > File webrtc/system_wrappers/BUILD.gn (left): > > https://codereview.webrtc.org/1412003007/diff/1/webrtc/system_wrappers/BUILD.... > webrtc/system_wrappers/BUILD.gn:27: "interface/field_trial.h", > On 2015/10/28 13:48:43, kjellander (webrtc) wrote: > > Not that it actually matters to the build, but wouldn't it be nicer to move > this > > file into the field_trial_default target if you're removing the dependency on > > system_wrappers? > > (since it's included in field_trial_default.cc) > > I think this file should stay in system_wrappers as it defines functions to be > used in other parts of webrtc. > > As I discussed with Guo-wei we can avoid compile-time dependencies on external > functions like this and replace these with runtime dependency injection. So > field_trial_default and metrics_default can be removed in the future.. lgtm. FYI, I created a webrtc bug 5146 to track moving to runtime part.
kjellander@google.com changed reviewers: + kjellander@google.com
lgtm
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412003007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412003007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1524)
On 2015/11/02 05:24:35, kjellander (DO NOT USE THIS) wrote: > lgtm Can you please rubberstamp from the right account?
The CQ bit was checked by kjellander@webrtc.org
lgtm Sorry about that.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412003007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412003007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1528)
sergeyu@chromium.org changed reviewers: + niklas.enbom@webrtc.org+
+niklas.enbom@webrtc.org for system_wrappers.gyp
Description was changed from ========== Remove system_wrappers dep from field_trial_default and metrics_default. field_trial_default and metrics_default don't use system_wrappers and don't need to depend on it. The dependency on field_trial_default was added in libjingle in crrev.com/356135 and that broke compilation of libjingle for NaCl with GN because system_wrappers currently doesn't compile for NaCl. ========== to ========== Remove system_wrappers dep from field_trial_default and metrics_default. field_trial_default and metrics_default don't use system_wrappers and don't need to depend on it. The dependency on field_trial_default was added in libjingle in crrev.com/356135 and that broke compilation of libjingle for NaCl with GN because system_wrappers currently doesn't compile for NaCl. TBR=niklas.enbom@webrtc.org ==========
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412003007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412003007/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5ab193fd8d38bcbbe2778cb177096ed0fad868fc Cr-Commit-Position: refs/heads/master@{#10495} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
