Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1980003002:
Separate building and enabling libevent. (Closed)
|
Created:
4 years, 7 months ago by phoglund Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc, tommi Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSeparate building and enabling libevent.
We're now ready https://codereview.webrtc.org/1984503002/ downstream,
so make sure we can enable libevent but still choose which libevent
implementation to use. This follows the common pattern where an enable_
flag controls whether we should use the feature at all, whereas build_
controls if we should use the dependency from our DEPS file or
something else.
NOTRY=True
Committed: https://crrev.com/ff274394fed4b54d9fbe977ab64d71ecef522880
Cr-Commit-Position: refs/heads/master@{#12772}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 4
Patch Set 4 : Make build_libevent also depend on platforms (doesn't build on win) #Patch Set 5 : Invert elses and source removals #Patch Set 6 : Better attempt at cleanup #
Total comments: 6
Patch Set 7 : Including platform impls even if libevent should not build, for base_unittests #Patch Set 8 : Yet another attempt #
Total comments: 3
Patch Set 9 : #Patch Set 10 : Added GN config #
Total comments: 6
Patch Set 11 : Better GN config #Patch Set 12 : Move gyp define #
Total comments: 2
Patch Set 13 : Moved build libevent out of if statement. #Patch Set 14 : Fix target defines #
Messages
Total messages: 36 (12 generated)
phoglund@webrtc.org changed reviewers: + kjellander@webrtc.org
I'd like either you or tommi@ to rewrite the conditions so they're not using sources! statements. I'm also missing GN configurations for this. https://codereview.webrtc.org/1980003002/diff/40001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/40001/webrtc/base/base.gyp#newc... webrtc/base/base.gyp:125: ['enable_libevent!=1', { I suggest starting with enable_libevent==1 and then put this in the else-clause, i.e. after }, { Also please rewrite the source listings without using sources! exclusions - it makes GYP really hard to read and maintain (or ask tommi to do so in a separate CL; IMO it should never have been allowed to land like that in https://codereview.webrtc.org/1984503002). https://codereview.webrtc.org/1980003002/diff/40001/webrtc/base/base.gyp#newc... webrtc/base/base.gyp:133: ['enable_libevent==1 or OS=="linux" or OS=="android" or OS=="win"', { See previous comment. You can put this into a sub-condition of the suggested if-else construction above (for readability).
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
I can rewrite the conditions to avoid using sources! statements. I'd like to do that after this cl has landed, so that we can deal with fewer corner cases.
PTAL https://codereview.webrtc.org/1980003002/diff/40001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/40001/webrtc/base/base.gyp#newc... webrtc/base/base.gyp:125: ['enable_libevent!=1', { On 2016/05/16 12:21:13, kjellander (webrtc) wrote: > I suggest starting with > enable_libevent==1 > > and then put this in the else-clause, i.e. after > }, { > > Also please rewrite the source listings without using sources! exclusions - it > makes GYP really hard to read and maintain (or ask tommi to do so in a separate > CL; IMO it should never have been allowed to land like that in > https://codereview.webrtc.org/1984503002). Yeah, I might as well do it here so I'll do it. https://codereview.webrtc.org/1980003002/diff/40001/webrtc/base/base.gyp#newc... webrtc/base/base.gyp:133: ['enable_libevent==1 or OS=="linux" or OS=="android" or OS=="win"', { On 2016/05/16 12:21:13, kjellander (webrtc) wrote: > See previous comment. You can put this into a sub-condition of the suggested > if-else construction above (for readability). Done. https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp#new... webrtc/base/base.gyp:130: ['OS!="linux" and OS!="android" and OS!="win"', { I thought this was cleaner than nesting 'conditions'. This one needs to be the else block for both conditions above.
https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp#new... webrtc/base/base.gyp:130: ['OS!="linux" and OS!="android" and OS!="win"', { On 2016/05/16 13:18:01, phoglund wrote: > I thought this was cleaner than nesting 'conditions'. This one needs to be the > else block for both conditions above. Also this should translate cleanly to GN.
Nice cleanup! https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp#new... webrtc/base/base.gyp:87: 'task_queue_posix.h', Move 'task_queue.h' and 'task_queue_posix.h' into the enable_libevent==1 condition since they're only used by those sources.
https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp#new... webrtc/base/base.gyp:87: 'task_queue_posix.h', On 2016/05/16 13:26:13, kjellander (webrtc) wrote: > Move 'task_queue.h' and 'task_queue_posix.h' into the enable_libevent==1 > condition since they're only used by those sources. the posix file is also needed for mac.
We should use this as an interview question, and watch the candidates tremble in horror as they are unable to come up with a solution. https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp#new... webrtc/base/base.gyp:87: 'task_queue_posix.h', On 2016/05/16 13:28:55, tommi-webrtc wrote: > On 2016/05/16 13:26:13, kjellander (webrtc) wrote: > > Move 'task_queue.h' and 'task_queue_posix.h' into the enable_libevent==1 > > condition since they're only used by those sources. > > the posix file is also needed for mac. Yeah, look at the new patch set. I think we should leave the .h files in there to save some duplication. https://codereview.webrtc.org/1980003002/diff/140001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/140001/webrtc/base/base.gyp#new... webrtc/base/base.gyp:124: 'task_queue_posix.cc', I don't like putting the posix file in two places but the alternative is much worse IMO: then we need to put a super complex condition for it as a separate condition ("if not linux and android and libevent not enabled: then add the file"). It's clearer what's going on now at least.
lg, but can you add GN config as well? https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/100001/webrtc/base/base.gyp#new... webrtc/base/base.gyp:87: 'task_queue_posix.h', On 2016/05/16 14:18:45, phoglund wrote: > On 2016/05/16 13:28:55, tommi-webrtc wrote: > > On 2016/05/16 13:26:13, kjellander (webrtc) wrote: > > > Move 'task_queue.h' and 'task_queue_posix.h' into the enable_libevent==1 > > > condition since they're only used by those sources. > > > > the posix file is also needed for mac. > > Yeah, look at the new patch set. I think we should leave the .h files in there > to save some duplication. Fair enough. https://codereview.webrtc.org/1980003002/diff/140001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/140001/webrtc/base/base.gyp#new... webrtc/base/base.gyp:124: 'task_queue_posix.cc', On 2016/05/16 14:18:45, phoglund wrote: > I don't like putting the posix file in two places but the alternative is much > worse IMO: then we need to put a super complex condition for it as a separate > condition ("if not linux and android and libevent not enabled: then add the > file"). It's clearer what's going on now at least. That's fine with me, simplicity is key. It's still way easier to read than the previous excluding version.
lgtm https://codereview.webrtc.org/1980003002/diff/140001/webrtc/base/base.gyp File webrtc/base/base.gyp (right): https://codereview.webrtc.org/1980003002/diff/140001/webrtc/base/base.gyp#new... webrtc/base/base.gyp:127: # If not libevent, fall back to the old task queues. s/old/other
PTAL
On 2016/05/16 14:53:19, phoglund wrote: > PTAL Hmm, the ios gn bot appears to be broken, it breaks for everyone.
On 2016/05/16 15:03:26, phoglund wrote: > On 2016/05/16 14:53:19, phoglund wrote: > > PTAL > > Hmm, the ios gn bot appears to be broken, it breaks for everyone. Yes, it's never been green: https://bugs.chromium.org/p/webrtc/issues/detail?id=5195. That's why it's not a part of the CQ trybots.
GN needs a bit more work due to some missing parts in the original CL. https://codereview.webrtc.org/1980003002/diff/180001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/1980003002/diff/180001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:178: if (!is_win && !is_mac && !is_ios && !is_nacl) { Can you please introduce and use rtc_build_libevent and rtc_enable_libevent variables in webrtc/build/webrtc.gni? https://codereview.webrtc.org/1980003002/diff/180001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:180: defines += [ "WEBRTC_BUILD_LIBEVENT" ] The define should got into webrtc/BUILD.gn's common_config target? reflecting the change in https://codereview.webrtc.org/1984503002/diff/280001/webrtc/build/common.gypi
PTAL https://codereview.webrtc.org/1980003002/diff/180001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/1980003002/diff/180001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:178: if (!is_win && !is_mac && !is_ios && !is_nacl) { On 2016/05/16 15:09:12, kjellander (webrtc) wrote: > Can you please introduce and use rtc_build_libevent and rtc_enable_libevent > variables in webrtc/build/webrtc.gni? Done. https://codereview.webrtc.org/1980003002/diff/180001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:180: defines += [ "WEBRTC_BUILD_LIBEVENT" ] On 2016/05/16 15:09:12, kjellander (webrtc) wrote: > The define should got into > webrtc/BUILD.gn's common_config target? > reflecting the change in > https://codereview.webrtc.org/1984503002/diff/280001/webrtc/build/common.gypi I think that feels icky. The defines there are very general, and I think it's wise to keep such a specific define close to where it's being used.
https://codereview.webrtc.org/1980003002/diff/180001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/1980003002/diff/180001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:180: defines += [ "WEBRTC_BUILD_LIBEVENT" ] On 2016/05/16 15:32:29, phoglund wrote: > On 2016/05/16 15:09:12, kjellander (webrtc) wrote: > > The define should got into > > webrtc/BUILD.gn's common_config target? > > reflecting the change in > > https://codereview.webrtc.org/1984503002/diff/280001/webrtc/build/common.gypi > > I think that feels icky. The defines there are very general, and I think it's > wise to keep such a specific define close to where it's being used. I agree. Can you move the other one into base.gyp instead then? The important thing is that GN and GYP are in sync per target (or we'll have a hard time when we're going to switch).
https://codereview.webrtc.org/1980003002/diff/180001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/1980003002/diff/180001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:180: defines += [ "WEBRTC_BUILD_LIBEVENT" ] On 2016/05/16 15:34:28, kjellander (webrtc) wrote: > On 2016/05/16 15:32:29, phoglund wrote: > > On 2016/05/16 15:09:12, kjellander (webrtc) wrote: > > > The define should got into > > > webrtc/BUILD.gn's common_config target? > > > reflecting the change in > > > > https://codereview.webrtc.org/1984503002/diff/280001/webrtc/build/common.gypi > > > > I think that feels icky. The defines there are very general, and I think it's > > wise to keep such a specific define close to where it's being used. > > I agree. Can you move the other one into base.gyp instead then? The important > thing is that GN and GYP are in sync per target (or we'll have a hard time when > we're going to switch). Certainly, that's a good idea.
lgtm with the GN comment addressed. https://codereview.webrtc.org/1980003002/diff/220001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/1980003002/diff/220001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:179: if (rtc_build_libevent) { Move the if (rtc_build_libevent) block out of the "if (rtc_enable_libevent)" block to make this similar to GYP. Even if it's unlikely one would be interested in building it without having it enabled, it is possible, and GYP+GN should be in sync.
The CQ bit was checked by phoglund@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/1980003002/#ps240001 (title: "Moved build libevent out of if statement.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980003002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980003002/240001
https://codereview.webrtc.org/1980003002/diff/220001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/1980003002/diff/220001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:179: if (rtc_build_libevent) { On 2016/05/17 07:38:26, kjellander (webrtc) wrote: > Move the if (rtc_build_libevent) block out of the "if (rtc_enable_libevent)" > block to make this similar to GYP. > > Even if it's unlikely one would be interested in building it without having it > enabled, it is possible, and GYP+GN should be in sync. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/12216)
The CQ bit was checked by phoglund@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/1980003002/#ps260001 (title: "Fix target defines")
The CQ bit was unchecked by phoglund@webrtc.org
Description was changed from ========== Separate building and enabling libevent. We're now ready https://codereview.webrtc.org/1984503002/ downstream, so make sure we can enable libevent but still choose which libevent implementation to use. This follows the common pattern where an enable_ flag controls whether we should use the feature at all, whereas build_ controls if we should use the dependency from our DEPS file or something else. ========== to ========== Separate building and enabling libevent. We're now ready https://codereview.webrtc.org/1984503002/ downstream, so make sure we can enable libevent but still choose which libevent implementation to use. This follows the common pattern where an enable_ flag controls whether we should use the feature at all, whereas build_ controls if we should use the dependency from our DEPS file or something else. NOTRY=True ==========
The CQ bit was checked by phoglund@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980003002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980003002/260001
Message was sent while issue was closed.
Description was changed from ========== Separate building and enabling libevent. We're now ready https://codereview.webrtc.org/1984503002/ downstream, so make sure we can enable libevent but still choose which libevent implementation to use. This follows the common pattern where an enable_ flag controls whether we should use the feature at all, whereas build_ controls if we should use the dependency from our DEPS file or something else. NOTRY=True ========== to ========== Separate building and enabling libevent. We're now ready https://codereview.webrtc.org/1984503002/ downstream, so make sure we can enable libevent but still choose which libevent implementation to use. This follows the common pattern where an enable_ flag controls whether we should use the feature at all, whereas build_ controls if we should use the dependency from our DEPS file or something else. NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Separate building and enabling libevent. We're now ready https://codereview.webrtc.org/1984503002/ downstream, so make sure we can enable libevent but still choose which libevent implementation to use. This follows the common pattern where an enable_ flag controls whether we should use the feature at all, whereas build_ controls if we should use the dependency from our DEPS file or something else. NOTRY=True ========== to ========== Separate building and enabling libevent. We're now ready https://codereview.webrtc.org/1984503002/ downstream, so make sure we can enable libevent but still choose which libevent implementation to use. This follows the common pattern where an enable_ flag controls whether we should use the feature at all, whereas build_ controls if we should use the dependency from our DEPS file or something else. NOTRY=True Committed: https://crrev.com/ff274394fed4b54d9fbe977ab64d71ecef522880 Cr-Commit-Position: refs/heads/master@{#12772} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/ff274394fed4b54d9fbe977ab64d71ecef522880 Cr-Commit-Position: refs/heads/master@{#12772} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
