Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by tkchin_webrtc Modified:
4 years, 2 months ago Reviewers:
kthelgason, kjellander_webrtc, Chuck CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), henrika_webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, sdk-team_agora.io, peah-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd iOS static library GN build script.
NOTRY=True
BUG=webrtc:6372
Committed: https://crrev.com/5fa51e29479ed584a8879363240863152294abd1
Cr-Commit-Position: refs/heads/master@{#14532}
Patch Set 1 #Patch Set 2 : Remove commented line #
Total comments: 2
Patch Set 3 : Don't build vp9 #Patch Set 4 : Update comment #
Total comments: 4
Patch Set 5 : CR comments #Patch Set 6 : Revert copyright change. #
Created: 4 years, 2 months ago
Messages
Total messages: 24 (10 generated)
Description was changed from ========== Add iOS static library GN build script. BUG=6372 ========== to ========== Add iOS static library GN build script. BUG=6372 ==========
tkchin@webrtc.org changed reviewers: + haysc@webrtc.org, kjellander@google.com
Description was changed from ========== Add iOS static library GN build script. BUG=6372 ========== to ========== Add iOS static library GN build script. BUG=webrtc:6372 ==========
tkchin@webrtc.org changed reviewers: + kjellander@webrtc.org - kjellander@google.com
First stab to get things working again for static libraries.
https://codereview.webrtc.org/2391123002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCVideoTrack.mm (right): https://codereview.webrtc.org/2391123002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCVideoTrack.mm:72: if (adapter.videoRenderer == renderer) { Why this change? Seems unrelated to the CL?
https://codereview.webrtc.org/2391123002/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCVideoTrack.mm (right): https://codereview.webrtc.org/2391123002/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCVideoTrack.mm:72: if (adapter.videoRenderer == renderer) { On 2016/10/04 20:44:46, Chuck wrote: > Why this change? Seems unrelated to the CL? I think somehow in GN we get assertions in release builds compared to GYP before. So now this assert is triggering when you try to add more than one renderer, which is incorrect.
Can we just update the existing script instead of creating a new one? Then the bots won't need to be updated (and I imagine there are many references to it in other places as well). In that case, we also need to add at least the -r flag if we want to keep supporting embedding of the revision, like in the old script: https://cs.chromium.org/chromium/src/third_party/webrtc/build/ios/build_ios_l... The bots are currently passing -r <hash> to the existing script, but it's easy to just remove that if it's not adding any value.
kthelgason@webrtc.org changed reviewers: + kthelgason@webrtc.org
I also agree with kjellander@ that we should update the old script rather than creating a new one. https://codereview.webrtc.org/2391123002/diff/60001/webrtc/build/ios/build_io... File webrtc/build/ios/build_ios_libs_gn.sh (right): https://codereview.webrtc.org/2391123002/diff/60001/webrtc/build/ios/build_io... webrtc/build/ios/build_ios_libs_gn.sh:44: build_static_webrtc "x86" "i386" I'm not really convinced we need this. https://codereview.webrtc.org/2391123002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCVideoTrack.mm (right): https://codereview.webrtc.org/2391123002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCVideoTrack.mm:72: if (adapter.videoRenderer == renderer) { This seems strange. Is this a bug that you're fixing? Seems unrelated to this CL.
Per first comment I would rather do the script update over a series of CLs to get something working right now instead of replacing the previous script with one that doesn't have everything anyway. https://codereview.webrtc.org/2391123002/diff/60001/webrtc/build/ios/build_io... File webrtc/build/ios/build_ios_libs_gn.sh (right): https://codereview.webrtc.org/2391123002/diff/60001/webrtc/build/ios/build_io... webrtc/build/ios/build_ios_libs_gn.sh:44: build_static_webrtc "x86" "i386" On 2016/10/05 07:46:24, kthelgason wrote: > I'm not really convinced we need this. Required to build correctly in other repository. https://codereview.webrtc.org/2391123002/diff/60001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCVideoTrack.mm (right): https://codereview.webrtc.org/2391123002/diff/60001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCVideoTrack.mm:72: if (adapter.videoRenderer == renderer) { On 2016/10/05 07:46:24, kthelgason wrote: > This seems strange. Is this a bug that you're fixing? Seems unrelated to this > CL. See other comment.
lgtm
On 2016/10/05 16:42:27, tkchin_webrtc wrote: > Per first comment I would rather do the script update over a series of CLs to > get something working right now instead of replacing the previous script with > one that doesn't have everything anyway. Even just with this CL, this script would be useful for many, and by providing a gitiles link to the previous version in the CL description anyone interested in the broken GYP version can look at that. Having something broken checked in is worse than something basic but working IMHO. > > https://codereview.webrtc.org/2391123002/diff/60001/webrtc/build/ios/build_io... > File webrtc/build/ios/build_ios_libs_gn.sh (right): > > https://codereview.webrtc.org/2391123002/diff/60001/webrtc/build/ios/build_io... > webrtc/build/ios/build_ios_libs_gn.sh:44: build_static_webrtc "x86" "i386" > On 2016/10/05 07:46:24, kthelgason wrote: > > I'm not really convinced we need this. > > Required to build correctly in other repository. > > https://codereview.webrtc.org/2391123002/diff/60001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/RTCVideoTrack.mm (right): > > https://codereview.webrtc.org/2391123002/diff/60001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCVideoTrack.mm:72: if (adapter.videoRenderer > == renderer) { > On 2016/10/05 07:46:24, kthelgason wrote: > > This seems strange. Is this a bug that you're fixing? Seems unrelated to this > > CL. > > See other comment.
Chatted offline, removed old script.
On 2016/10/05 18:16:25, tkchin_webrtc wrote: > Chatted offline, removed old script. in that case, lgtm.
lgtm
Description was changed from ========== Add iOS static library GN build script. BUG=webrtc:6372 ========== to ========== Add iOS static library GN build script. NOTRY=True BUG=webrtc:6372 ==========
The CQ bit was checked by tkchin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from haysc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2391123002/#ps100001 (title: "Revert copyright change.")
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 iOS static library GN build script. NOTRY=True BUG=webrtc:6372 ========== to ========== Add iOS static library GN build script. NOTRY=True BUG=webrtc:6372 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add iOS static library GN build script. NOTRY=True BUG=webrtc:6372 ========== to ========== Add iOS static library GN build script. NOTRY=True BUG=webrtc:6372 Committed: https://crrev.com/5fa51e29479ed584a8879363240863152294abd1 Cr-Commit-Position: refs/heads/master@{#14532} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5fa51e29479ed584a8879363240863152294abd1 Cr-Commit-Position: refs/heads/master@{#14532} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
