Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 8 months ago by kjellander_webrtc Modified:
4 years, 8 months ago Reviewers:
torbjorng (webrtc), perkj_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCleanup webrtc/base/base.gyp
* Remove all source exclusions since they make the file very hard to
read and heavily increases the risk for mistakes.
* Don't compile the openssl* sources if use_openssl==0.
* Move platform specific sources into conditional includes to make it
easier to verify a 1:1 mapping with BUILD.gn (since GN doesn't support
automatic detection of platform specific sources based on filenames).
* Add missing sources for the GN build.
* Reorder some blocks to make GYP vs GN mapping match.
BUG=webrtc:4256
R=perkj@webrtc.org, torbjorng@webrtc.org
Committed: https://crrev.com/47f33cb28ffb0fa0f053ae0aa0086e11f85bf444
Cr-Commit-Position: refs/heads/master@{#12235}
Patch Set 1 : #
Total comments: 9
Patch Set 2 : Removed use_openssl condition for GYP and GN #
Created: 4 years, 8 months ago
Messages
Total messages: 20 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Cleanup webrtc/base/base.gyp * Remove all source exclusions since they make the file very hard to read and heavily increases the risk for mistakes. * Don't compile the openssl* sources if use_openssl==0. * Move platform specific sources into conditional includes to make it easier to verify a 1:1 mapping with BUILD.gn (since GN doesn't support automatic detection of platform specific sources based on filenames). * Add missing scoped_autorelease_pool.mm object for the GN build. WIP: doesn't pass GYP since some odd error.c BUG= ========== to ========== Cleanup webrtc/base/base.gyp * Remove all source exclusions since they make the file very hard to read and heavily increases the risk for mistakes. * Don't compile the openssl* sources if use_openssl==0. * Move platform specific sources into conditional includes to make it easier to verify a 1:1 mapping with BUILD.gn (since GN doesn't support automatic detection of platform specific sources based on filenames). * Add missing sources for the GN build. * Reorder some blocks to make GYP vs GN mapping match. BUG=webrtc:4256 ==========
kjellander@webrtc.org changed reviewers: + perkj@webrtc.org
https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (left): https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn#oldc... webrtc/base/BUILD.gn:260: "mathutils.h", intentionally removed? https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn#oldc... webrtc/base/BUILD.gn:377: "genericslot.h", why removed? https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn#newc... webrtc/base/BUILD.gn:389: "testbase64.h", intentionally added? https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/base.gyp#newc... webrtc/base/base.gyp:485: '<(DEPTH)/third_party/boringssl/boringssl.gyp:boringssl', ? Isn't boringssl and use_openssl separat things? You either use open_ssl or boringssl?
kjellander@webrtc.org changed reviewers: + torbjorng@webrtc.org
torbjorng: please see my question about the openssl* sources. https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (left): https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn#oldc... webrtc/base/BUILD.gn:260: "mathutils.h", On 2016/04/05 07:08:12, perkj_webrtc wrote: > intentionally removed? Yes, base.gyp only lists this for non-Chromium builds so the GN config was wrong here. https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn#oldc... webrtc/base/BUILD.gn:377: "genericslot.h", On 2016/04/05 07:08:12, perkj_webrtc wrote: > why removed? They don't exist (removed in https://webrtc-codereview.appspot.com/19649004) https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn#newc... webrtc/base/BUILD.gn:389: "testbase64.h", On 2016/04/05 07:08:12, perkj_webrtc wrote: > intentionally added? Yes, to match base.gyp https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/base.gyp#newc... webrtc/base/base.gyp:470: ['use_openssl==1', { TBH I don't recall the motivation behind this, but I'm sure there was a reason. Adding torbjorng to comment on this. https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/base.gyp#newc... webrtc/base/base.gyp:485: '<(DEPTH)/third_party/boringssl/boringssl.gyp:boringssl', On 2016/04/05 07:08:12, perkj_webrtc wrote: > ? Isn't boringssl and use_openssl separat things? You either use open_ssl or > boringssl? Yes, boringssl is a forked subset of openssl. But the build_ssl property is a WebRTC-only variable we use to optionally skip building BoringSSL (needed for some downstream projects). use_openssl is the Chromium variable used for deciding if to use OpenSSL (BoringSSL) or NSS, which Chromium still uses for iOS. In WebRTC use_openssl is always 1, since we use BoringSSL also for iOS.
On 2016/04/05 07:21:38, kjellander (webrtc) wrote: > torbjorng: please see my question about the openssl* sources. > > https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn > File webrtc/base/BUILD.gn (left): > > https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn#oldc... > webrtc/base/BUILD.gn:260: "mathutils.h", > On 2016/04/05 07:08:12, perkj_webrtc wrote: > > intentionally removed? > > Yes, base.gyp only lists this for non-Chromium builds so the GN config was wrong > here. > > https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn#oldc... > webrtc/base/BUILD.gn:377: "genericslot.h", > On 2016/04/05 07:08:12, perkj_webrtc wrote: > > why removed? > > They don't exist (removed in https://webrtc-codereview.appspot.com/19649004) > > https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn > File webrtc/base/BUILD.gn (right): > > https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/BUILD.gn#newc... > webrtc/base/BUILD.gn:389: "testbase64.h", > On 2016/04/05 07:08:12, perkj_webrtc wrote: > > intentionally added? > > Yes, to match base.gyp > > https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/base.gyp > File webrtc/base/base.gyp (right): > > https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/base.gyp#newc... > webrtc/base/base.gyp:470: ['use_openssl==1', { > TBH I don't recall the motivation behind this, but I'm sure there was a reason. > Adding torbjorng to comment on this. > > https://codereview.webrtc.org/1859803002/diff/40001/webrtc/base/base.gyp#newc... > webrtc/base/base.gyp:485: > '<(DEPTH)/third_party/boringssl/boringssl.gyp:boringssl', > On 2016/04/05 07:08:12, perkj_webrtc wrote: > > ? Isn't boringssl and use_openssl separat things? You either use open_ssl or > > boringssl? > > Yes, boringssl is a forked subset of openssl. > But the build_ssl property is a WebRTC-only variable we use to optionally skip > building BoringSSL (needed for some downstream projects). > > use_openssl is the Chromium variable used for deciding if to use OpenSSL > (BoringSSL) or NSS, which Chromium still uses for iOS. > In WebRTC use_openssl is always 1, since we use BoringSSL also for iOS. lgtm if Torbjörn is happy.
I think this change wrt openssl* is incorrect. A correct change would be to remove the use_openssl variable from the build files, as we don't support it being false. The openssl* files support both BoringSSL and (for now) legacy OpenSSL.
Patchset #2 (id:60001) has been deleted
On 2016/04/05 08:18:16, torbjorng (webrtc) wrote: > I think this change wrt openssl* is incorrect. > > A correct change would be to remove the use_openssl variable from the build > files, as we don't support it being false. > > The openssl* files support both BoringSSL and (for now) legacy OpenSSL. Thanks. I was confused by the BUILD.gn that had them in a condition. They were moved there in https://webrtc-codereview.appspot.com/31469004 and you forgot to update the BUILD.gn in your https://codereview.webrtc.org/1351503004. I have now restored the base.gyp and updated the BUILD.gn. PTAL torbjorn.
lgtm Feel free to clean out use_openssl from these files too: webrtc/build/isolate.gypi webrtc/media/media.gyp webrtc/supplement.gypi :-)
On 2016/04/05 10:41:58, torbjorng (webrtc) wrote: > lgtm > > Feel free to clean out use_openssl from these files too: > > webrtc/build/isolate.gypi This is just to copy Chromium's approach. Eventually we might need to keep it to have it properly passed for iOS (see below). That said, iOS doesn't use the isolate, but might do at some point in the future. > webrtc/media/media.gyp Doesn't have it actually. > webrtc/supplement.gypi We need it here to set it to 1 for iOS, since Chromium doesn't. > :-)
Description was changed from ========== Cleanup webrtc/base/base.gyp * Remove all source exclusions since they make the file very hard to read and heavily increases the risk for mistakes. * Don't compile the openssl* sources if use_openssl==0. * Move platform specific sources into conditional includes to make it easier to verify a 1:1 mapping with BUILD.gn (since GN doesn't support automatic detection of platform specific sources based on filenames). * Add missing sources for the GN build. * Reorder some blocks to make GYP vs GN mapping match. BUG=webrtc:4256 ========== to ========== Cleanup webrtc/base/base.gyp * Remove all source exclusions since they make the file very hard to read and heavily increases the risk for mistakes. * Don't compile the openssl* sources if use_openssl==0. * Move platform specific sources into conditional includes to make it easier to verify a 1:1 mapping with BUILD.gn (since GN doesn't support automatic detection of platform specific sources based on filenames). * Add missing sources for the GN build. * Reorder some blocks to make GYP vs GN mapping match. BUG=webrtc:4256 R=perkj@webrtc.org, torbjorng@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/47f33cb28ffb0fa0f053ae0aa... ==========
Description was changed from ========== Cleanup webrtc/base/base.gyp * Remove all source exclusions since they make the file very hard to read and heavily increases the risk for mistakes. * Don't compile the openssl* sources if use_openssl==0. * Move platform specific sources into conditional includes to make it easier to verify a 1:1 mapping with BUILD.gn (since GN doesn't support automatic detection of platform specific sources based on filenames). * Add missing sources for the GN build. * Reorder some blocks to make GYP vs GN mapping match. BUG=webrtc:4256 R=perkj@webrtc.org, torbjorng@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/47f33cb28ffb0fa0f053ae0aa... ========== to ========== Cleanup webrtc/base/base.gyp * Remove all source exclusions since they make the file very hard to read and heavily increases the risk for mistakes. * Don't compile the openssl* sources if use_openssl==0. * Move platform specific sources into conditional includes to make it easier to verify a 1:1 mapping with BUILD.gn (since GN doesn't support automatic detection of platform specific sources based on filenames). * Add missing sources for the GN build. * Reorder some blocks to make GYP vs GN mapping match. BUG=webrtc:4256 R=perkj@webrtc.org, torbjorng@webrtc.org Committed: https://crrev.com/47f33cb28ffb0fa0f053ae0aa0086e11f85bf444 Cr-Commit-Position: refs/heads/master@{#12235} ==========
Committed patchset #2 (id:80001) manually as 47f33cb28ffb0fa0f053ae0aa0086e11f85bf444 (presubmit successful).
Patchset 2 (id:??) landed as https://crrev.com/47f33cb28ffb0fa0f053ae0aa0086e11f85bf444 Cr-Commit-Position: refs/heads/master@{#12235}
Description was changed from ========== Cleanup webrtc/base/base.gyp * Remove all source exclusions since they make the file very hard to read and heavily increases the risk for mistakes. * Don't compile the openssl* sources if use_openssl==0. * Move platform specific sources into conditional includes to make it easier to verify a 1:1 mapping with BUILD.gn (since GN doesn't support automatic detection of platform specific sources based on filenames). * Add missing sources for the GN build. * Reorder some blocks to make GYP vs GN mapping match. BUG=webrtc:4256 R=perkj@webrtc.org, torbjorng@webrtc.org Committed: https://crrev.com/47f33cb28ffb0fa0f053ae0aa0086e11f85bf444 Cr-Commit-Position: refs/heads/master@{#12235} ========== to ========== Cleanup webrtc/base/base.gyp * Remove all source exclusions since they make the file very hard to read and heavily increases the risk for mistakes. * Don't compile the openssl* sources if use_openssl==0. * Move platform specific sources into conditional includes to make it easier to verify a 1:1 mapping with BUILD.gn (since GN doesn't support automatic detection of platform specific sources based on filenames). * Add missing sources for the GN build. * Reorder some blocks to make GYP vs GN mapping match. BUG=webrtc:4256 R=perkj@webrtc.org, torbjorng@webrtc.org Committed: https://crrev.com/47f33cb28ffb0fa0f053ae0aa0086e11f85bf444 Cr-Commit-Position: refs/heads/master@{#12235} ==========
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:80001) has been created in https://codereview.webrtc.org/1856323003/ by kjellander@webrtc.org. The reason for reverting is: For some odd reason this breaks chromium.webrtc.fyi bots: ../../third_party/webrtc_overrides/webrtc/base/win32socketinit.cc:13:2: error: "Only compile this on Windows" #error "Only compile this on Windows" ^ 1 error generated. https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/build... https://build.chromium.org/p/chromium.webrtc.fyi/builders/Linux%20Builder/bui.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
