|
|
Created:
4 years, 10 months ago by hjon_webrtc Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUpdate build_ios_libs.sh script to build new Objective-C API and gather header files.
BUG=
Committed: https://crrev.com/9bf5cde91a9b1673dcfe22a41ca7df9c000f869b
Cr-Commit-Position: refs/heads/master@{#11694}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changes based on feedback #
Total comments: 1
Patch Set 3 : Changes based on feedback #
Total comments: 8
Patch Set 4 : Changes based on feedback #Patch Set 5 : Update against master #
Total comments: 4
Patch Set 6 : Changes based on feedback #
Total comments: 2
Patch Set 7 : Use bool #
Total comments: 2
Messages
Total messages: 28 (9 generated)
Description was changed from ========== Update build_ios_libs.sh script to gather header files and change merge_ios_libs script to put merged libraries in lib directory. BUG= ========== to ========== Update build_ios_libs.sh script to build new Objective-C API and gather header files. BUG= ==========
hjon@webrtc.org changed reviewers: + tkchin@webrtc.org
https://codereview.webrtc.org/1673503002/diff/1/talk/build/build_ios_libs.sh File talk/build/build_ios_libs.sh (right): https://codereview.webrtc.org/1673503002/diff/1/talk/build/build_ios_libs.sh#... talk/build/build_ios_libs.sh:98: copy_headers ${HEADERS} ${LIBRARY_BASE_DIR}/include Can we do this in a .py instead? And place both the list and the script in the same file? To make things easier, suggest putting directories in the list, then making the py script filter the headers in the directory (accept all headers except +Private ones). That way we won't have to always update the headers file. Should support exporting individual headers too (there are some isolated ObjC headers we might want to export). https://codereview.webrtc.org/1673503002/diff/1/talk/build/merge_ios_libs.gyp File talk/build/merge_ios_libs.gyp (left): https://codereview.webrtc.org/1673503002/diff/1/talk/build/merge_ios_libs.gyp... talk/build/merge_ios_libs.gyp:38: '../libjingle.gyp:libjingle_peerconnection_objc', Don't do this yet (we don't want to break anyone yet). You can add a new target webrtc_api_objc_no_op if you want though.
Changed script to use a list of directories to find header files, as well as provide a way to export individual header files and exclude individual header files from being exported. This may not be the best or most idiomatic way to do this in Python. https://codereview.webrtc.org/1673503002/diff/1/talk/build/build_ios_libs.sh File talk/build/build_ios_libs.sh (right): https://codereview.webrtc.org/1673503002/diff/1/talk/build/build_ios_libs.sh#... talk/build/build_ios_libs.sh:98: copy_headers ${HEADERS} ${LIBRARY_BASE_DIR}/include On 2016/02/05 15:57:59, tkchin_webrtc wrote: > Can we do this in a .py instead? And place both the list and the script in the > same file? > To make things easier, suggest putting directories in the list, then making the > py script filter the headers in the directory (accept all headers except > +Private ones). That way we won't have to always update the headers file. Should > support exporting individual headers too (there are some isolated ObjC headers > we might want to export). Done. https://codereview.webrtc.org/1673503002/diff/1/talk/build/merge_ios_libs.gyp File talk/build/merge_ios_libs.gyp (left): https://codereview.webrtc.org/1673503002/diff/1/talk/build/merge_ios_libs.gyp... talk/build/merge_ios_libs.gyp:38: '../libjingle.gyp:libjingle_peerconnection_objc', On 2016/02/05 15:57:59, tkchin_webrtc wrote: > Don't do this yet (we don't want to break anyone yet). > > You can add a new target webrtc_api_objc_no_op if you want though. Done. https://codereview.webrtc.org/1673503002/diff/20001/talk/build/export_headers File talk/build/export_headers (right): https://codereview.webrtc.org/1673503002/diff/20001/talk/build/export_headers... talk/build/export_headers:34: exclude_headers = ['webrtc/api/objc/RTCNSGLVideoView.h'] Should avfoundationvideocapturer.h be excluded?
Added the flag we talked about IRL for building the old or new API. Let me know if the naming or style should be adjusted.
https://codereview.webrtc.org/1673503002/diff/40001/talk/build/export_headers File talk/build/export_headers (right): https://codereview.webrtc.org/1673503002/diff/40001/talk/build/export_headers... talk/build/export_headers:20: old_header_dirs = ['talk/app/webrtc/objc/public'] these should be constants, so all caps https://codereview.webrtc.org/1673503002/diff/40001/talk/build/export_headers... talk/build/export_headers:21: new_header_dirs = ['webrtc/api/objc/', 'webrtc/base/objc/'] nit: suggest legacy_header_dirs header_dirs so no old/new. here and below https://codereview.webrtc.org/1673503002/diff/40001/talk/build/export_headers... talk/build/export_headers:23: old_include_headers = [] nit: header_includes header_excludes https://codereview.webrtc.org/1673503002/diff/40001/talk/build/merge_ios_libs... File talk/build/merge_ios_libs.gyp (right): https://codereview.webrtc.org/1673503002/diff/40001/talk/build/merge_ios_libs... talk/build/merge_ios_libs.gyp:37: '<(webrtc_root)/system_wrappers/system_wrappers.gyp:field_trial_default', might be nice to push the field trial default dependency in to objc_app.gypi
https://codereview.webrtc.org/1673503002/diff/40001/talk/build/export_headers File talk/build/export_headers (right): https://codereview.webrtc.org/1673503002/diff/40001/talk/build/export_headers... talk/build/export_headers:20: old_header_dirs = ['talk/app/webrtc/objc/public'] On 2016/02/17 22:15:58, tkchin_webrtc wrote: > these should be constants, so all caps Done. https://codereview.webrtc.org/1673503002/diff/40001/talk/build/export_headers... talk/build/export_headers:21: new_header_dirs = ['webrtc/api/objc/', 'webrtc/base/objc/'] On 2016/02/17 22:15:58, tkchin_webrtc wrote: > nit: suggest > legacy_header_dirs > header_dirs > > so no old/new. here and below Done. https://codereview.webrtc.org/1673503002/diff/40001/talk/build/export_headers... talk/build/export_headers:23: old_include_headers = [] On 2016/02/17 22:15:58, tkchin_webrtc wrote: > nit: header_includes > header_excludes Done. https://codereview.webrtc.org/1673503002/diff/40001/talk/build/merge_ios_libs... File talk/build/merge_ios_libs.gyp (right): https://codereview.webrtc.org/1673503002/diff/40001/talk/build/merge_ios_libs... talk/build/merge_ios_libs.gyp:37: '<(webrtc_root)/system_wrappers/system_wrappers.gyp:field_trial_default', On 2016/02/17 22:15:58, tkchin_webrtc wrote: > might be nice to push the field trial default dependency in to objc_app.gypi Done.
lgtm % nits https://codereview.webrtc.org/1673503002/diff/80001/talk/build/build_ios_libs.sh File talk/build/build_ios_libs.sh (right): https://codereview.webrtc.org/1673503002/diff/80001/talk/build/build_ios_libs... talk/build/build_ios_libs.sh:31: BUILD_NEW_API=0 nit: to be consistent, let's call this USE_LEGACY_API and set it to 1 https://codereview.webrtc.org/1673503002/diff/80001/talk/build/merge_ios_libs... File talk/build/merge_ios_libs.gyp (right): https://codereview.webrtc.org/1673503002/diff/80001/talk/build/merge_ios_libs... talk/build/merge_ios_libs.gyp:39: 'sources': ['<(DEPTH)/webrtc/build/no_op.cc',], any reason why this was changed to <DEPTH instead of webrtc_root?
ptal for confirmation. https://codereview.webrtc.org/1673503002/diff/80001/talk/build/build_ios_libs.sh File talk/build/build_ios_libs.sh (right): https://codereview.webrtc.org/1673503002/diff/80001/talk/build/build_ios_libs... talk/build/build_ios_libs.sh:31: BUILD_NEW_API=0 On 2016/02/18 19:32:57, tkchin_webrtc wrote: > nit: to be consistent, let's call this USE_LEGACY_API and set it to 1 Done. https://codereview.webrtc.org/1673503002/diff/80001/talk/build/merge_ios_libs... File talk/build/merge_ios_libs.gyp (right): https://codereview.webrtc.org/1673503002/diff/80001/talk/build/merge_ios_libs... talk/build/merge_ios_libs.gyp:39: 'sources': ['<(DEPTH)/webrtc/build/no_op.cc',], On 2016/02/18 19:32:57, tkchin_webrtc wrote: > any reason why this was changed to <DEPTH instead of webrtc_root? Looks like it was originally <DEPTH, but was changed in master and I missed that when I updated against master. I'll update it. Also below.
On 2016/02/18 21:24:00, hjon_webrtc wrote: > ptal for confirmation. > > https://codereview.webrtc.org/1673503002/diff/80001/talk/build/build_ios_libs.sh > File talk/build/build_ios_libs.sh (right): > > https://codereview.webrtc.org/1673503002/diff/80001/talk/build/build_ios_libs... > talk/build/build_ios_libs.sh:31: BUILD_NEW_API=0 > On 2016/02/18 19:32:57, tkchin_webrtc wrote: > > nit: to be consistent, let's call this USE_LEGACY_API and set it to 1 > > Done. > > https://codereview.webrtc.org/1673503002/diff/80001/talk/build/merge_ios_libs... > File talk/build/merge_ios_libs.gyp (right): > > https://codereview.webrtc.org/1673503002/diff/80001/talk/build/merge_ios_libs... > talk/build/merge_ios_libs.gyp:39: 'sources': > ['<(DEPTH)/webrtc/build/no_op.cc',], > On 2016/02/18 19:32:57, tkchin_webrtc wrote: > > any reason why this was changed to <DEPTH instead of webrtc_root? > > Looks like it was originally <DEPTH, but was changed in master and I missed that > when I updated against master. I'll update it. Also below. Still lgtm.
https://codereview.webrtc.org/1673503002/diff/100001/talk/build/export_headers File talk/build/export_headers (right): https://codereview.webrtc.org/1673503002/diff/100001/talk/build/export_header... talk/build/export_headers:82: use_legacy_headers = args[1] nit: check the string value and turn it into a bool here
https://codereview.webrtc.org/1673503002/diff/100001/talk/build/export_headers File talk/build/export_headers (right): https://codereview.webrtc.org/1673503002/diff/100001/talk/build/export_header... talk/build/export_headers:82: use_legacy_headers = args[1] On 2016/02/18 23:49:39, tkchin_webrtc wrote: > nit: check the string value and turn it into a bool here Done. https://codereview.webrtc.org/1673503002/diff/120001/talk/build/export_headers File talk/build/export_headers (right): https://codereview.webrtc.org/1673503002/diff/120001/talk/build/export_header... talk/build/export_headers:82: use_legacy_headers = False if int(args[1]) == 0 else True Is this style acceptable or would you prefer a local variable to be added?
https://codereview.webrtc.org/1673503002/diff/120001/talk/build/export_headers File talk/build/export_headers (right): https://codereview.webrtc.org/1673503002/diff/120001/talk/build/export_header... talk/build/export_headers:82: use_legacy_headers = False if int(args[1]) == 0 else True On 2016/02/19 16:21:31, hjon_webrtc wrote: > Is this style acceptable or would you prefer a local variable to be added? If the linter doesn't complain this is fine.
The CQ bit was checked by hjon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1673503002/#ps120001 (title: "Use bool")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673503002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673503002/120001
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/3560)
hjon@webrtc.org changed reviewers: + jiayl@webrtc.org
@jiayl Could you also take a look?
lgtm
The CQ bit was checked by hjon@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673503002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673503002/120001
Message was sent while issue was closed.
Description was changed from ========== Update build_ios_libs.sh script to build new Objective-C API and gather header files. BUG= ========== to ========== Update build_ios_libs.sh script to build new Objective-C API and gather header files. BUG= ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Update build_ios_libs.sh script to build new Objective-C API and gather header files. BUG= ========== to ========== Update build_ios_libs.sh script to build new Objective-C API and gather header files. BUG= Committed: https://crrev.com/9bf5cde91a9b1673dcfe22a41ca7df9c000f869b Cr-Commit-Position: refs/heads/master@{#11694} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9bf5cde91a9b1673dcfe22a41ca7df9c000f869b Cr-Commit-Position: refs/heads/master@{#11694} |