Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2029323006:
Add missing dependencies on audio, video and call to the new GN files. (Closed)
|
Created:
4 years, 6 months ago by katrielc1 Modified:
4 years, 6 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, pbos-webrtc, stefan-webrtc, mflodman, phoglund Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd missing dependencies on audio, video and call to the new GN files.
This caused linker failures when depending on the new `api` target.
TBR=henrika@webrtc.org
NOTRY=True
Committed: https://crrev.com/14897d0b7d649b74b0fadb5d6886b17da142ece9
Cr-Commit-Position: refs/heads/master@{#13042}
Patch Set 1 #Patch Set 2 : Add kjellander@ to OWNERS for BUILD.gn #
Messages
Total messages: 27 (12 generated)
Description was changed from ========== Add missing dependencies on audio, video and call to the new GN files. This caused linker failures when depending on the new `api` target. BUG= ========== to ========== Add missing dependencies on audio, video and call to the new GN files. This caused linker failures when depending on the new `api` target. BUG= ==========
katrielc@chromium.org changed reviewers: + kjellander@webrtc.org
katrielc@chromium.org changed reviewers: + katrielc@chromium.org
Please add corresponding dependencies to the GYP files as well. And please don't run all trybots (including all tests) on compile-only patches, use a macro like described in https://groups.google.com/a/webrtc.org/forum/#!topic/all/YwBU4xcyiwQ instead, and set NOTRY=True on the CL. That saves significant resources and leads to shorter CQ times for CLs that actually needs to run the tests.
On 2016/06/03 12:15:22, kjellander_webrtc wrote: > Please add corresponding dependencies to the GYP files as well. I had a look for what the corresponding dependencies should be in GYP, but it's not obvious to me (never used GYP). video/webrtc_video.gypi doesn't define targets, and webrtc.gypi doesn't seem to have a video-only one. Should it just depend on webrtc_all in GYP? > And please don't run all trybots (including all tests) on compile-only patches Noted, sorry -- I've defined the alias. (Did it this time without thinking.)
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
lgtm when kjellander is fine with this
Sorry, you're right there is no corresponding GYP targets. This is actually one of the few places where there's no 1:1 mapping between GYP<->GN, and I've already told pbos@ in the past that this needs to be cleaned up. lgtm (I'll CQ 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/patch-status/2029323006/1
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/6076)
On 2016/06/03 18:18:53, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6076) Does this actually need approval from an OWNER, or is it covered by your migration kjellander? (If so, how do I fix the trybot?)
On 2016/06/03 18:22:55, katrielc wrote: > On 2016/06/03 18:18:53, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > presubmit on tryserver.webrtc (JOB_FAILED, > > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6076) > > Does this actually need approval from an OWNER, or is it covered by your > migration kjellander? (If so, how do I fix the trybot?) Gah, webrtc/api/OWNERS is missing the entry all other OWNERS files has: per-file BUILD.gn=kjellander@webrtc.org Can you add that change to this CL, then add TBR=henrika@webrtc.org to this CL's description, then I can take another look just to be sure we're good before you let it go in?
Description was changed from ========== Add missing dependencies on audio, video and call to the new GN files. This caused linker failures when depending on the new `api` target. BUG= ========== to ========== Add missing dependencies on audio, video and call to the new GN files. This caused linker failures when depending on the new `api` target. TBR=henrika@webrtc.org ==========
On 2016/06/03 18:43:10, kjellander_webrtc wrote: > On 2016/06/03 18:22:55, katrielc wrote: > > On 2016/06/03 18:18:53, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > presubmit on tryserver.webrtc (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6076) > > > > Does this actually need approval from an OWNER, or is it covered by your > > migration kjellander? (If so, how do I fix the trybot?) > > Gah, webrtc/api/OWNERS is missing the entry all other OWNERS files has: > per-file mailto:BUILD.gn=kjellander@webrtc.org > > Can you add that change to this CL, then add > mailto:TBR=henrika@webrtc.org > to this CL's description, then I can take another look just to be sure we're > good before you let it go in? Done.
Description was changed from ========== Add missing dependencies on audio, video and call to the new GN files. This caused linker failures when depending on the new `api` target. TBR=henrika@webrtc.org ========== to ========== Add missing dependencies on audio, video and call to the new GN files. This caused linker failures when depending on the new `api` target. TBR=henrika@webrtc.org NOTRY=True ==========
The CQ bit was checked by kjellander@webrtc.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2029323006/#ps20001 (title: "Add kjellander@ to OWNERS for BUILD.gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2029323006/20001
On 2016/06/03 20:12:48, kjellander_webrtc wrote: > lgtm I added NOTRY=True to skip trybots on second patch set.
Message was sent while issue was closed.
Description was changed from ========== Add missing dependencies on audio, video and call to the new GN files. This caused linker failures when depending on the new `api` target. TBR=henrika@webrtc.org NOTRY=True ========== to ========== Add missing dependencies on audio, video and call to the new GN files. This caused linker failures when depending on the new `api` target. TBR=henrika@webrtc.org NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add missing dependencies on audio, video and call to the new GN files. This caused linker failures when depending on the new `api` target. TBR=henrika@webrtc.org NOTRY=True ========== to ========== Add missing dependencies on audio, video and call to the new GN files. This caused linker failures when depending on the new `api` target. TBR=henrika@webrtc.org NOTRY=True Committed: https://crrev.com/14897d0b7d649b74b0fadb5d6886b17da142ece9 Cr-Commit-Position: refs/heads/master@{#13042} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/14897d0b7d649b74b0fadb5d6886b17da142ece9 Cr-Commit-Position: refs/heads/master@{#13042} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
