Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
173013003:
Compile libjingle for PNaCl (Closed)
|
Created:
6 years, 10 months ago by Sergey Ulanov Modified:
6 years, 10 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCompile libjingle for PNaCl
Added libjingle_nacl target that compile libjingle for PNaCl.
Removed dependency on jsoncpp.
BUG=276739
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252825
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 5
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : android #Patch Set 8 : typo #
Messages
Total messages: 26 (0 generated)
+tommi as reviewer as well since he made most of the recent changes to libjingle.gyp. :)
https://codereview.chromium.org/173013003/diff/70001/third_party/libjingle/li... File third_party/libjingle/libjingle.gyp (right): https://codereview.chromium.org/173013003/diff/70001/third_party/libjingle/li... third_party/libjingle/libjingle.gyp:256: 'sources!' : [ why do we have these files in the libjingle_srcs.gypi and remove here? https://codereview.chromium.org/173013003/diff/70001/third_party/libjingle/li... third_party/libjingle/libjingle.gyp:278: 'sources/': [ can we move all these sources! to libjingle_srcs.gypi so that they can be shared between nacl and non-nacl. https://codereview.chromium.org/173013003/diff/70001/third_party/libjingle/li... File third_party/libjingle/libjingle_nacl.gyp (right): https://codereview.chromium.org/173013003/diff/70001/third_party/libjingle/li... third_party/libjingle/libjingle_nacl.gyp:14: 'target_name': 'libjingle_nacl', can nacl just be treated as another platform aside to mac/linux etc? In that case we don't need a separated gyp for nacl as lot of parts are duplicated to libjingle.gyp in this file.
https://codereview.chromium.org/173013003/diff/70001/third_party/libjingle/li... File third_party/libjingle/libjingle.gyp (right): https://codereview.chromium.org/173013003/diff/70001/third_party/libjingle/li... third_party/libjingle/libjingle.gyp:256: 'sources!' : [ On 2014/02/20 19:13:30, Ronghua Wu wrote: > why do we have these files in the libjingle_srcs.gypi and remove here? I find it useful to have a single list of all files. Otherwise it becomes harder to manage when files are added/removed. In this particular case logging.cc are used in the NaCl target. NaCl also suffers from the issue with two constants.cc, but since it's different build system we should workaround it separately. E.g. when the problem is fixed for NaCl we will be able to remove libjingle_p2p_constants_nacl without any changes in this file or in libjingle_srcs.gyp. https://codereview.chromium.org/173013003/diff/70001/third_party/libjingle/li... third_party/libjingle/libjingle.gyp:278: 'sources/': [ On 2014/02/20 19:13:30, Ronghua Wu wrote: > can we move all these sources! to libjingle_srcs.gypi so that they can be shared > between nacl and non-nacl. Done. https://codereview.chromium.org/173013003/diff/70001/third_party/libjingle/li... File third_party/libjingle/libjingle_nacl.gyp (right): https://codereview.chromium.org/173013003/diff/70001/third_party/libjingle/li... third_party/libjingle/libjingle_nacl.gyp:14: 'target_name': 'libjingle_nacl', On 2014/02/20 19:13:30, Ronghua Wu wrote: > can nacl just be treated as another platform aside to mac/linux etc? In that > case we don't need a separated gyp for nacl as lot of parts are duplicated to > libjingle.gyp in this file. No, unfortunately it doesn't work this way. I think proper solution would be to support a nacl in the 'toolset' parameter (see crbug.com/120214), but currently GYP doesn't support it, and we need separate set of targets for NaCl. All other NaCl targets are compiled in the same way (e.g. see base_untrusted.gyp).
BTW, in the latest patchset also removed jsoncpp dependency and talk/base/json.cc - it wasn't used anywhere.
lgtm. Thanks for the heads up. Good job getting rid of the jsoncpp dependency! :)
https://codereview.chromium.org/173013003/diff/130001/third_party/libjingle/l... File third_party/libjingle/libjingle_nacl.gyp (right): https://codereview.chromium.org/173013003/diff/130001/third_party/libjingle/l... third_party/libjingle/libjingle_nacl.gyp:20: 'build_newlib': 0, assume this change is intended. https://codereview.chromium.org/173013003/diff/130001/third_party/libjingle/l... third_party/libjingle/libjingle_nacl.gyp:80: 'USE_WEBRTC_DEV_BRANCH', Can we move these to the include file as well? Maybe change libjingle_srcs.gypi to libjingle_common.gypi
https://codereview.chromium.org/173013003/diff/130001/third_party/libjingle/l... File third_party/libjingle/libjingle_nacl.gyp (right): https://codereview.chromium.org/173013003/diff/130001/third_party/libjingle/l... third_party/libjingle/libjingle_nacl.gyp:20: 'build_newlib': 0, On 2014/02/21 16:51:17, Ronghua Wu wrote: > assume this change is intended. Yes. Since we are going to use PNaCl build, but not NaCl, it doesn't make sense to build libjingle for NaCl at this point. https://codereview.chromium.org/173013003/diff/130001/third_party/libjingle/l... third_party/libjingle/libjingle_nacl.gyp:80: 'USE_WEBRTC_DEV_BRANCH', On 2014/02/21 16:51:17, Ronghua Wu wrote: > Can we move these to the include file as well? I did look into that. Problem is that libjingle.gyp has multiple targets and the defines list is in target_defaults. The gypi file is included for only one of them. Beside that the set of defines we need here is different from what's defined in libjingle.gyp. The current approach is the simplest and cleanest I can see. > > Maybe change libjingle_srcs.gypi to libjingle_common.gypi Done.
https://codereview.chromium.org/173013003/diff/130001/third_party/libjingle/l... File third_party/libjingle/libjingle_nacl.gyp (right): https://codereview.chromium.org/173013003/diff/130001/third_party/libjingle/l... third_party/libjingle/libjingle_nacl.gyp:80: 'USE_WEBRTC_DEV_BRANCH', On 2014/02/21 19:49:19, Sergey Ulanov wrote: > On 2014/02/21 16:51:17, Ronghua Wu wrote: > > Can we move these to the include file as well? > > I did look into that. Problem is that libjingle.gyp has multiple targets and the > defines list is in target_defaults. The gypi file is included for only one of > them. Beside that the set of defines we need here is different from what's > defined in libjingle.gyp. The current approach is the simplest and cleanest I > can see. > > > > > Maybe change libjingle_srcs.gypi to libjingle_common.gypi > > Done. I thought we can define the list of the source as a variable (e.g. libjingle_srcs) and used it in the source file for the 2 libjingle libs. Then you can include libjingle_common.gypi on top of libjingle/libjingle_nacl.gyp. And do something like this for the 2 libjingle libs. sources': [ <(libjingle_srcs) ] But if this is not possible for some reason, I'm fine with current approach.
On 2014/02/21 19:55:12, Ronghua Wu wrote: > https://codereview.chromium.org/173013003/diff/130001/third_party/libjingle/l... > File third_party/libjingle/libjingle_nacl.gyp (right): > > https://codereview.chromium.org/173013003/diff/130001/third_party/libjingle/l... > third_party/libjingle/libjingle_nacl.gyp:80: 'USE_WEBRTC_DEV_BRANCH', > On 2014/02/21 19:49:19, Sergey Ulanov wrote: > > On 2014/02/21 16:51:17, Ronghua Wu wrote: > > > Can we move these to the include file as well? > > > > I did look into that. Problem is that libjingle.gyp has multiple targets and > the > > defines list is in target_defaults. The gypi file is included for only one of > > them. Beside that the set of defines we need here is different from what's > > defined in libjingle.gyp. The current approach is the simplest and cleanest I > > can see. > > > > > > > > Maybe change libjingle_srcs.gypi to libjingle_common.gypi > > > > Done. > > I thought we can define the list of the source as a variable (e.g. > libjingle_srcs) and used it in the source file for the 2 libjingle libs. > > Then you can include libjingle_common.gypi on top of > libjingle/libjingle_nacl.gyp. And do something like this for the 2 libjingle > libs. > sources': [ <(libjingle_srcs) ] That's possible, but then the exclusion rules would have to be duplicated between the two gyp files (as in patchset 1). > > But if this is not possible for some reason, I'm fine with current approach.
lgtm
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/173013003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/173013003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/173013003/110002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/173013003/870001
Message was sent while issue was closed.
Change committed as 252825 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
