Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 3 months ago by kthelgason Modified:
4 years, 3 months ago Reviewers:
kjellander_webrtc, tkchin_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
Description[GN] Add rtc_sdk_framework_objc target to GN
The build artifacts don't look completely identical to the ones generated
by the GYP targets, but manual review shows the same symbols are exported.
On iOS, the version generated by the GN follows convention, including
a "Headers" directory, and the .modulemap file. I think this is preferred
over the gyp version.
BUG=webrtc:6320
NOTRY=True
TESTED=Run AppRTCDemo on iOS + Mac and verified with nm that they export the same symbols.
Committed: https://crrev.com/ebc34e78dbff8a34b9b0ec5c54a846e61934955a
Cr-Commit-Position: refs/heads/master@{#14228}
Patch Set 1 #
Total comments: 6
Patch Set 2 : code review #
Total comments: 1
Patch Set 3 : Extract common headers #
Total comments: 1
Patch Set 4 : Add missing header for mac #
Created: 4 years, 3 months ago
Messages
Total messages: 21 (9 generated)
kthelgason@webrtc.org changed reviewers: + kjellander@webrtc.org
That was fast! Generally looks good. Do you know if this is covered: https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/sdk.gyp?q=sdk.gyp... Can you perform some testing on the GYP vs GN versions of the output and describe in the CL description how you've done and motivate the correctness of this new target? https://codereview.webrtc.org/2340633003/diff/1/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2340633003/diff/1/webrtc/sdk/BUILD.gn#newcode235 webrtc/sdk/BUILD.gn:235: "objc/Framework/Headers/WebRTC/RTCFileLogger.h", This one should not be present in Chromium builds: https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/sdk.gyp?rcl=0&l=334 So move it into: if (!build_with_chromium) { sources += [ "objc/Framework/Headers/WebRTC/RTCFileLogger.h" ] } https://codereview.webrtc.org/2340633003/diff/1/webrtc/sdk/BUILD.gn#newcode307 webrtc/sdk/BUILD.gn:307: "objc/Framework/Headers/WebRTC/RTCFileLogger.h", See previous comment about RTCFileLogger.h. https://codereview.webrtc.org/2340633003/diff/1/webrtc/sdk/BUILD.gn#newcode317 webrtc/sdk/BUILD.gn:317: "objc/Framework/Headers/WebRTC/RTCNSGLVideoView.h", This one is excluded for iOS in GYP: https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/sdk.gyp?rcl=0&l=329 so I don't think it belongs here. https://codereview.webrtc.org/2340633003/diff/1/webrtc/sdk/BUILD.gn#newcode345 webrtc/sdk/BUILD.gn:345: "objc/Framework/Headers/WebRTC/RTCFileLogger.h", Same here. https://codereview.webrtc.org/2340633003/diff/1/webrtc/sdk/BUILD.gn#newcode355 webrtc/sdk/BUILD.gn:355: "objc/Framework/Headers/WebRTC/RTCNSGLVideoView.h", Same here.
On 2016/09/14 07:28:48, kjellander_webrtc wrote: > That was fast! Generally looks good. > Do you know if this is covered: > https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/sdk.gyp?q=sdk.gyp... I'm pretty sure those are covered, yes. No code signing is the default, the Info.plist file is specified, and the modulemap is generated as part of the template (as far as I can tell). > Can you perform some testing on the GYP vs GN versions of the output and > describe in the CL description how you've done and motivate the correctness of > this new target? The targets don't look completely identical, the "Versions" directory structure is not created on mac, which is fine AFAIK. I ran nm on both binaries and diff'ed their exported symbols to make sure nothing was missing, and it looked good to me. On iOS, however, the version generated by the GN follows more recent conventions for iOS, including a "Headers" directory, and the .modulemap file. I think this is preferred over the gyp version. In any case, I built all the targets that depend on the "rtc_sdk_framework_objc" target, and tested AppRTCDemo on both mac and ios, just as a sanity check, and things seemed to work. I realise that statement might not infer a lot of confidence though!
Can you incorporate some of the comments/reasoning of your previous reply into the CL description? https://codereview.webrtc.org/2340633003/diff/1/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2340633003/diff/1/webrtc/sdk/BUILD.gn#newcode383 webrtc/sdk/BUILD.gn:383: "GLKit.framework", Is this not supposed to be there on iOS? https://codereview.webrtc.org/2340633003/diff/20001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2340633003/diff/20001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:300: "objc/Framework/Headers/WebRTC/RTCAVFoundationVideoSource.h", Can you sort these lists alphabetically, extract them into a variable and use that, since it looks very much the same for sources and public_headers? I guess make a common_headers variable, which does not include objc/Framework/Headers/WebRTC/RTCNSGLVideoView.h for iOS, then add that header for Mac only. e.g. Outer scope: common_objc_headers = [......] Mac: sources = common_objc_headers + "objc/Framework/Headers/WebRTC/RTCNSGLVideoView.h" iOS: sources = common_objc_headers public_headers = common_objc_headers
Description was changed from ========== [GN] Add rtc_sdk_framework_objc target to GN BUG=webrtc:6320 ========== to ========== [GN] Add rtc_sdk_framework_objc target to GN While the artifacts don't look completely identical to the ones generated by the GYP targets, I've tested them with AppRTCDemo, and verified with nm that they export the same symbols. BUG=webrtc:6320 ==========
Updated. PTAL
Just one file missing. I think you can add a little more from your excellent message regarding what you've done to the CL description. Then we're good to go! https://codereview.webrtc.org/2340633003/diff/40001/webrtc/sdk/BUILD.gn File webrtc/sdk/BUILD.gn (right): https://codereview.webrtc.org/2340633003/diff/40001/webrtc/sdk/BUILD.gn#newco... webrtc/sdk/BUILD.gn:259: sources = common_objc_headers This one should have + "objc/Framework/Headers/WebRTC/RTCNSGLVideoView.h"
Description was changed from ========== [GN] Add rtc_sdk_framework_objc target to GN While the artifacts don't look completely identical to the ones generated by the GYP targets, I've tested them with AppRTCDemo, and verified with nm that they export the same symbols. BUG=webrtc:6320 ========== to ========== [GN] Add rtc_sdk_framework_objc target to GN While the artifacts don't look completely identical to the ones generated by the GYP targets, I've tested them with AppRTCDemo, and verified with nm that they export the same symbols. On iOS, the version generated by the GN follows convention, including a "Headers" directory, and the .modulemap file. I think this is preferred over the gyp version. BUG=webrtc:6320 ==========
On 2016/09/14 13:24:10, kjellander_webrtc wrote: > https://codereview.webrtc.org/2340633003/diff/40001/webrtc/sdk/BUILD.gn#newco... > webrtc/sdk/BUILD.gn:259: sources = common_objc_headers > This one should have > + "objc/Framework/Headers/WebRTC/RTCNSGLVideoView.h" Whoops! Added, and updated description. Feel free to rewrite it yourself if you feel something is missing.
Description was changed from ========== [GN] Add rtc_sdk_framework_objc target to GN While the artifacts don't look completely identical to the ones generated by the GYP targets, I've tested them with AppRTCDemo, and verified with nm that they export the same symbols. On iOS, the version generated by the GN follows convention, including a "Headers" directory, and the .modulemap file. I think this is preferred over the gyp version. BUG=webrtc:6320 ========== to ========== [GN] Add rtc_sdk_framework_objc target to GN The build artifacts don't look completely identical to the ones generated by the GYP targets, but manual review shows the same symbols are exported. On iOS, the version generated by the GN follows convention, including a "Headers" directory, and the .modulemap file. I think this is preferred over the gyp version. BUG=webrtc:6320 TESTED=Run AppRTCDemo on iOS + Mac and verified with nm that they export the same symbols. ==========
kjellander@webrtc.org changed reviewers: + tkchin@webrtc.org
Description was changed from ========== [GN] Add rtc_sdk_framework_objc target to GN The build artifacts don't look completely identical to the ones generated by the GYP targets, but manual review shows the same symbols are exported. On iOS, the version generated by the GN follows convention, including a "Headers" directory, and the .modulemap file. I think this is preferred over the gyp version. BUG=webrtc:6320 TESTED=Run AppRTCDemo on iOS + Mac and verified with nm that they export the same symbols. ========== to ========== [GN] Add rtc_sdk_framework_objc target to GN The build artifacts don't look completely identical to the ones generated by the GYP targets, but manual review shows the same symbols are exported. On iOS, the version generated by the GN follows convention, including a "Headers" directory, and the .modulemap file. I think this is preferred over the gyp version. BUG=webrtc:6320 NOTRY=True TESTED=Run AppRTCDemo on iOS + Mac and verified with nm that they export the same symbols. ==========
lgtm but wait for Zeke's review. I added NOTRY=True to the CL description since this is a build-only change that don't require any tests to run. I triggered a few trybots to be sure we don't break anything unexpected.
On 2016/09/14 14:13:24, kjellander_webrtc wrote: > lgtm but wait for Zeke's review. > > I added NOTRY=True to the CL description since this is a build-only change that > don't require any tests to run. I triggered a few trybots to be sure we don't > break anything unexpected. lgtm
The CQ bit was checked by kthelgason@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 ========== [GN] Add rtc_sdk_framework_objc target to GN The build artifacts don't look completely identical to the ones generated by the GYP targets, but manual review shows the same symbols are exported. On iOS, the version generated by the GN follows convention, including a "Headers" directory, and the .modulemap file. I think this is preferred over the gyp version. BUG=webrtc:6320 NOTRY=True TESTED=Run AppRTCDemo on iOS + Mac and verified with nm that they export the same symbols. ========== to ========== [GN] Add rtc_sdk_framework_objc target to GN The build artifacts don't look completely identical to the ones generated by the GYP targets, but manual review shows the same symbols are exported. On iOS, the version generated by the GN follows convention, including a "Headers" directory, and the .modulemap file. I think this is preferred over the gyp version. BUG=webrtc:6320 NOTRY=True TESTED=Run AppRTCDemo on iOS + Mac and verified with nm that they export the same symbols. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [GN] Add rtc_sdk_framework_objc target to GN The build artifacts don't look completely identical to the ones generated by the GYP targets, but manual review shows the same symbols are exported. On iOS, the version generated by the GN follows convention, including a "Headers" directory, and the .modulemap file. I think this is preferred over the gyp version. BUG=webrtc:6320 NOTRY=True TESTED=Run AppRTCDemo on iOS + Mac and verified with nm that they export the same symbols. ========== to ========== [GN] Add rtc_sdk_framework_objc target to GN The build artifacts don't look completely identical to the ones generated by the GYP targets, but manual review shows the same symbols are exported. On iOS, the version generated by the GN follows convention, including a "Headers" directory, and the .modulemap file. I think this is preferred over the gyp version. BUG=webrtc:6320 NOTRY=True TESTED=Run AppRTCDemo on iOS + Mac and verified with nm that they export the same symbols. Committed: https://crrev.com/ebc34e78dbff8a34b9b0ec5c54a846e61934955a Cr-Commit-Position: refs/heads/master@{#14228} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ebc34e78dbff8a34b9b0ec5c54a846e61934955a Cr-Commit-Position: refs/heads/master@{#14228} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
