Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1847013002:
Set defines for Chromium build. (Closed)
|
Created:
4 years, 8 months ago by kjellander_webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSet defines for Chromium build.
Copy the defines from the target_defaults section of Chromium's
src/third_party/libjingle.gyp into our webrtc/build/common.gypi
in order to ensure the same defines are used for the Chromium build
when removing the source listings in src/third_party/libjingle.gyp.
With this CL landed, it should be possible to replace them with
dependencies on:
* webrtc/api/api.gyp:libjingle_peerconnections
* webrtc/media/media.gyp:rtc_media
* webrtc/pc/pc.gyp:rtc_pc
* webrtc/pp2/p2p.gyp:rtc_p2p
* webrtc/libjingle/xmpp/xmpp.gyp:rtc_xmpp
Not ported (Windows specific):
* Precompiled headers (build/win_precompile.gypi):
since it only seems to offer a compile speedup. Will be landed
for all of WebRTC in separate CL.
BUG=webrtc:4256
NOTRY=True
R=perkj@webrtc.org, tommi@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/9266cc066845c5d5be6551c583dcc831a2a47b67
Patch Set 1 #Patch Set 2 : Simplied how the defines are handled #
Total comments: 6
Patch Set 3 : Added TODOs #
Messages
Total messages: 20 (8 generated)
Description was changed from ========== Adapt webrtc/media to Chromium's GYP build Make the webrtc/media/media.gyp be built similar to how the source is currently built using Chromium's src/third_party/libjingle/libjingle.gyp. This is a prerequisite for being able to switch over to using WebRTC's GYP files for all source code. Not ported: Windows: * The _CRT_SECURE_NO_WARNINGS define: should only affect warnings. * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. This depends on https://codereview.chromium.org/1772403003/ being rolled into WebRTC. BUG=webrtc:4256 NOTRY=True ========== to ========== Adapt webrtc/media to Chromium's GYP build Update webrtc/media/media.gyp to be similar to how the source is currently built using Chromium's src/third_party/libjingle/libjingle.gyp. This is a prerequisite for being able to switch over to using WebRTC's GYP files for all source code. Make the 'EXPAT_RELATIVE_PATH' define be set only for the code that use it instead of globally. Not ported (Windows specific): * The _CRT_SECURE_NO_WARNINGS define: should only affect warnings. * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. This depends on https://codereview.chromium.org/1772403003/ being rolled into WebRTC. BUG=webrtc:4256 NOTRY=True ==========
Description was changed from ========== Adapt webrtc/media to Chromium's GYP build Update webrtc/media/media.gyp to be similar to how the source is currently built using Chromium's src/third_party/libjingle/libjingle.gyp. This is a prerequisite for being able to switch over to using WebRTC's GYP files for all source code. Make the 'EXPAT_RELATIVE_PATH' define be set only for the code that use it instead of globally. Not ported (Windows specific): * The _CRT_SECURE_NO_WARNINGS define: should only affect warnings. * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. This depends on https://codereview.chromium.org/1772403003/ being rolled into WebRTC. BUG=webrtc:4256 NOTRY=True ========== to ========== Update webrtc/media to Chromium's GYP build + move EXPAT_RELATIVE_PATH Update webrtc/media/media.gyp to be similar to how the source is currently built using Chromium's src/third_party/libjingle/libjingle.gyp. This is a prerequisite for being able to switch over to using WebRTC's GYP files for all source code. Make the 'EXPAT_RELATIVE_PATH' define be set only for the code that use it instead of globally. Not ported (Windows specific): * The _CRT_SECURE_NO_WARNINGS define: should only affect warnings. * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. This depends on https://codereview.chromium.org/1772403003/ being rolled into WebRTC. BUG=webrtc:4256 NOTRY=True ==========
kjellander@webrtc.org changed reviewers: + perkj@webrtc.org
Description was changed from ========== Update webrtc/media to Chromium's GYP build + move EXPAT_RELATIVE_PATH Update webrtc/media/media.gyp to be similar to how the source is currently built using Chromium's src/third_party/libjingle/libjingle.gyp. This is a prerequisite for being able to switch over to using WebRTC's GYP files for all source code. Make the 'EXPAT_RELATIVE_PATH' define be set only for the code that use it instead of globally. Not ported (Windows specific): * The _CRT_SECURE_NO_WARNINGS define: should only affect warnings. * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. This depends on https://codereview.chromium.org/1772403003/ being rolled into WebRTC. BUG=webrtc:4256 NOTRY=True ========== to ========== Update webrtc/media to Chromium's GYP build + move EXPAT_RELATIVE_PATH Update webrtc/media/media.gyp to be similar to how the source is currently built using Chromium's src/third_party/libjingle/libjingle.gyp. This is a prerequisite for being able to switch over to using WebRTC's GYP files for all source code. Set defines for some OSes globally: CHROMEOS, BSD, OPENBSD Make the 'EXPAT_RELATIVE_PATH' define be set only for the code that use it instead of globally. Not ported (Windows specific): * The _CRT_SECURE_NO_WARNINGS define: should only affect warnings. * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. This depends on https://codereview.chromium.org/1772403003/ being rolled into WebRTC. BUG=webrtc:4256 NOTRY=True ==========
Actually, hold off reviewing this as it's better I cover pc.gyp in this CL as well (and move the common defines out into a .gypi file).
Description was changed from ========== Update webrtc/media to Chromium's GYP build + move EXPAT_RELATIVE_PATH Update webrtc/media/media.gyp to be similar to how the source is currently built using Chromium's src/third_party/libjingle/libjingle.gyp. This is a prerequisite for being able to switch over to using WebRTC's GYP files for all source code. Set defines for some OSes globally: CHROMEOS, BSD, OPENBSD Make the 'EXPAT_RELATIVE_PATH' define be set only for the code that use it instead of globally. Not ported (Windows specific): * The _CRT_SECURE_NO_WARNINGS define: should only affect warnings. * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. This depends on https://codereview.chromium.org/1772403003/ being rolled into WebRTC. BUG=webrtc:4256 NOTRY=True ========== to ========== Set defines for Chromium build. Copy the defines from the target_defaults section of Chromium's src/third_party/libjingle.gyp into our webrtc/build/common.gypi in order to ensure the same defines are used for the Chromium build when removing the source listings in src/third_party/libjingle.gyp. With this CL landed, it should be possible to replace them with dependencies on: * webrtc/api/api.gyp:libjingle_peerconnections * webrtc/media/media.gyp:rtc_media * webrtc/pc/pc.gyp:rtc_pc * webrtc/pp2/p2p.gyp:rtc_p2p * webrtc/libjingle/xmpp/xmpp.gyp:rtc_xmpp Not ported (Windows specific): * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. BUG=webrtc:4256 NOTRY=True ==========
kjellander@webrtc.org changed reviewers: + tommi@webrtc.org
OK the new patch set is much simpler and safer I believe. I haven't yet looked at the NaCl stuff going on in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... but hey, one step at a time :)
On 2016/03/31 17:34:54, kjellander (webrtc) wrote: > OK the new patch set is much simpler and safer I believe. > > I haven't yet looked at the NaCl stuff going on in > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > but hey, one step at a time :) Notice: I repurposed the old CL a bit, so I'm not sure which e-mail header you'll get for the notification e-mails... just FYI.
https://codereview.webrtc.org/1847013002/diff/20001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1847013002/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:496: 'FEATURE_ENABLE_VOICEMAIL', I don't think you need FEATURE_ENABLE_VOICEMAIL. Only used in webrtc/p2p/p2p.gyp and xmpp.gyp and there it is currently conditioned on if 'build_with_chromium=0... Are you sure all these actually still have a meaning? https://codereview.webrtc.org/1847013002/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:528: '_CRT_SECURE_NO_WARNINGS', # Suppres warnings about _vsnprinf git grep "_vsnprinf" returns nothing. Not needed any more?
https://codereview.webrtc.org/1847013002/diff/20001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1847013002/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:496: 'FEATURE_ENABLE_VOICEMAIL', On 2016/04/01 09:33:47, perkj_webrtc wrote: > I don't think you need FEATURE_ENABLE_VOICEMAIL. > Only used in webrtc/p2p/p2p.gyp and xmpp.gyp and there it is currently > conditioned on if 'build_with_chromium=0... > > Are you sure all these actually still have a meaning? Initially I did a lot of grepping to see which defines are used, but then I realized setting them on a per-project basis is doable, but will make reviewing take longer and thus removing src/third_party/libjingle take even longer. In this CL I try to just make the code behave the same for Chromium - then we can do further cleanup later. https://codereview.webrtc.org/1847013002/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:528: '_CRT_SECURE_NO_WARNINGS', # Suppres warnings about _vsnprinf On 2016/04/01 09:33:47, perkj_webrtc wrote: > git grep "_vsnprinf" returns nothing. Not needed any more? I don't know, I thought it may be used in system libraries that are in turn used by our code. Considering how picky Chromium devs are about compiler warnings I didn't dare removing it.
lgtm Provided we (you) try to clean it up once Chrome has moved to use our gyp files and it is easier to test what is needed.
lgtm https://codereview.webrtc.org/1847013002/diff/20001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1847013002/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:496: 'FEATURE_ENABLE_VOICEMAIL', On 2016/04/01 10:34:36, kjellander (webrtc) wrote: > On 2016/04/01 09:33:47, perkj_webrtc wrote: > > I don't think you need FEATURE_ENABLE_VOICEMAIL. > > Only used in webrtc/p2p/p2p.gyp and xmpp.gyp and there it is currently > > conditioned on if 'build_with_chromium=0... > > > > Are you sure all these actually still have a meaning? > > Initially I did a lot of grepping to see which defines are used, but then I > realized setting them on a per-project basis is doable, but will make reviewing > take longer and thus removing src/third_party/libjingle take even longer. In > this CL I try to just make the code behave the same for Chromium - then we can > do further cleanup later. wanna put down a todo so that it won't be forgotten about?
https://codereview.webrtc.org/1847013002/diff/20001/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1847013002/diff/20001/webrtc/build/common.gypi#... webrtc/build/common.gypi:496: 'FEATURE_ENABLE_VOICEMAIL', On 2016/04/01 13:23:09, tommi-webrtc wrote: > On 2016/04/01 10:34:36, kjellander (webrtc) wrote: > > On 2016/04/01 09:33:47, perkj_webrtc wrote: > > > I don't think you need FEATURE_ENABLE_VOICEMAIL. > > > Only used in webrtc/p2p/p2p.gyp and xmpp.gyp and there it is currently > > > conditioned on if 'build_with_chromium=0... > > > > > > Are you sure all these actually still have a meaning? > > > > Initially I did a lot of grepping to see which defines are used, but then I > > realized setting them on a per-project basis is doable, but will make > reviewing > > take longer and thus removing src/third_party/libjingle take even longer. In > > this CL I try to just make the code behave the same for Chromium - then we can > > do further cleanup later. > > wanna put down a todo so that it won't be forgotten about? Yepp, added TODOs in PS#3
Message was sent while issue was closed.
Description was changed from ========== Set defines for Chromium build. Copy the defines from the target_defaults section of Chromium's src/third_party/libjingle.gyp into our webrtc/build/common.gypi in order to ensure the same defines are used for the Chromium build when removing the source listings in src/third_party/libjingle.gyp. With this CL landed, it should be possible to replace them with dependencies on: * webrtc/api/api.gyp:libjingle_peerconnections * webrtc/media/media.gyp:rtc_media * webrtc/pc/pc.gyp:rtc_pc * webrtc/pp2/p2p.gyp:rtc_p2p * webrtc/libjingle/xmpp/xmpp.gyp:rtc_xmpp Not ported (Windows specific): * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. BUG=webrtc:4256 NOTRY=True ========== to ========== Set defines for Chromium build. Copy the defines from the target_defaults section of Chromium's src/third_party/libjingle.gyp into our webrtc/build/common.gypi in order to ensure the same defines are used for the Chromium build when removing the source listings in src/third_party/libjingle.gyp. With this CL landed, it should be possible to replace them with dependencies on: * webrtc/api/api.gyp:libjingle_peerconnections * webrtc/media/media.gyp:rtc_media * webrtc/pc/pc.gyp:rtc_pc * webrtc/pp2/p2p.gyp:rtc_p2p * webrtc/libjingle/xmpp/xmpp.gyp:rtc_xmpp Not ported (Windows specific): * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. BUG=webrtc:4256 NOTRY=True R=perkj@webrtc.org, tommi@webrtc.org Committed: https://crrev.com/9266cc066845c5d5be6551c583dcc831a2a47b67 Cr-Commit-Position: refs/heads/master@{#12212} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9266cc066845c5d5be6551c583dcc831a2a47b67 Cr-Commit-Position: refs/heads/master@{#12212}
Message was sent while issue was closed.
Description was changed from ========== Set defines for Chromium build. Copy the defines from the target_defaults section of Chromium's src/third_party/libjingle.gyp into our webrtc/build/common.gypi in order to ensure the same defines are used for the Chromium build when removing the source listings in src/third_party/libjingle.gyp. With this CL landed, it should be possible to replace them with dependencies on: * webrtc/api/api.gyp:libjingle_peerconnections * webrtc/media/media.gyp:rtc_media * webrtc/pc/pc.gyp:rtc_pc * webrtc/pp2/p2p.gyp:rtc_p2p * webrtc/libjingle/xmpp/xmpp.gyp:rtc_xmpp Not ported (Windows specific): * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. BUG=webrtc:4256 NOTRY=True R=perkj@webrtc.org, tommi@webrtc.org Committed: https://crrev.com/9266cc066845c5d5be6551c583dcc831a2a47b67 Cr-Commit-Position: refs/heads/master@{#12212} ========== to ========== Set defines for Chromium build. Copy the defines from the target_defaults section of Chromium's src/third_party/libjingle.gyp into our webrtc/build/common.gypi in order to ensure the same defines are used for the Chromium build when removing the source listings in src/third_party/libjingle.gyp. With this CL landed, it should be possible to replace them with dependencies on: * webrtc/api/api.gyp:libjingle_peerconnections * webrtc/media/media.gyp:rtc_media * webrtc/pc/pc.gyp:rtc_pc * webrtc/pp2/p2p.gyp:rtc_p2p * webrtc/libjingle/xmpp/xmpp.gyp:rtc_xmpp Not ported (Windows specific): * Precompiled headers (build/win_precompile.gypi): since it only seems to offer a compile speedup. Will be landed for all of WebRTC in separate CL. BUG=webrtc:4256 NOTRY=True R=perkj@webrtc.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/9266cc066845c5d5be6551c58... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 9266cc066845c5d5be6551c583dcc831a2a47b67 (presubmit successful).
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.webrtc.org/1861603002/ by kjellander@webrtc.org. The reason for reverting is: This breaks remoting_unittests on Windows in Chromium: [5116:2536:0404/012329:5457156:ERROR:webrtcsession.cc(1388)] ConnectDataChannel called when data_channel_ is NULL. [5116:2536:0404/012329:5457187:ERROR:opensslidentity.cc(154)] Generating certificate: error:0c000071:ASN.1 encoding routines:OPENSSL_internal:ERROR_GETTING_TIME [5116:2536:0404/012329:5457218:ERROR:opensslidentity.cc(154)] Generating certificate: error:0c000071:ASN.1 encoding routines:OPENSSL_internal:ERROR_GETTING_TIME [5116:2536:0404/012329:5457218:WARNING:dtlsidentitystore.cc(221)] Failed to generate DTLS identity. [. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
