Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2187563005:
GN: Add target for modules_tests. (Closed)
|
Created:
4 years, 4 months ago by ehmaldonado_webrtc Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionGN: Add target for modules_tests.
Additional changes I needed to make it work:
- Modified a header in RTPFile.cc. Every other file is
using "webrtc/engine_configurations.h" instead.
- Disabled flag 4373 for msvs because it was disabled
in build/common.gypi.
BUG=webrtc:6038
TBR=kwiberg@webrtc.org
NOTRY=True
Committed: https://crrev.com/f98dc105bae202b51e16c88308997d47fc842a5a
Cr-Commit-Position: refs/heads/master@{#13628}
Patch Set 1 #Patch Set 2 : Disable some warnings in msvs. #Patch Set 3 : Moved disabled warnings to modules/BUILD.gn. #
Total comments: 1
Patch Set 4 : Makes videoprocessor_defines a variable. #Patch Set 5 : Forgot to add video_coding.gni to the CL. #
Total comments: 4
Patch Set 6 : Adressed comments. #
Total comments: 2
Messages
Total messages: 29 (12 generated)
ehmaldonado@webrtc.org changed reviewers: + phoglund@webrtc.org
Description was changed from ========== GN: Add target for modules_tests. Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. I needed this to make it work. BUG=webrtc:6038 ========== to ========== GN: Add target for modules_tests. Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. I needed this to make it work. Modified build/webrtc.gni to also disable some flags that were disabled in build/common.gypi. BUG=webrtc:6038 ==========
Description was changed from ========== GN: Add target for modules_tests. Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. I needed this to make it work. Modified build/webrtc.gni to also disable some flags that were disabled in build/common.gypi. BUG=webrtc:6038 ========== to ========== GN: Add target for modules_tests. Additional changes I needed to make it work: - Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. - Disabled flag 4373 for msvs because it was disabled in build/common.gypi. BUG=webrtc:6038 ==========
Sorry, reviewed this earlier but forgot to send drafts :/ https://codereview.webrtc.org/2187563005/diff/40001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/2187563005/diff/40001/webrtc/modules/BUILD.gn#n... webrtc/modules/BUILD.gn:41: defines = audio_coding_defines I see you have inlined videoprocessor_defines to just set H264, but make a variable anyway so gyp and gn looks the same.
On 2016/07/28 12:13:36, phoglund wrote: > Sorry, reviewed this earlier but forgot to send drafts :/ > > https://codereview.webrtc.org/2187563005/diff/40001/webrtc/modules/BUILD.gn > File webrtc/modules/BUILD.gn (right): > > https://codereview.webrtc.org/2187563005/diff/40001/webrtc/modules/BUILD.gn#n... > webrtc/modules/BUILD.gn:41: defines = audio_coding_defines > I see you have inlined videoprocessor_defines to just set H264, but make a > variable anyway so gyp and gn looks the same. I created a video_coding.gni, not sure if it's a good idea. PTAL
I think the .gni works, but it's fine to just declare the variable in the file it's used from, for now at least. Your call. lgtm
On 2016/07/29 10:57:02, phoglund wrote: > I think the .gni works, but it's fine to just declare the variable in the file > it's used from, for now at least. Your call. > > lgtm I'll leave it like it is. I think it'll easier to extend it later if necessary.
The CQ bit was checked by ehmaldonado@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7053)
ehmaldonado@webrtc.org changed reviewers: + kjellander@webrtc.org
Description was changed from ========== GN: Add target for modules_tests. Additional changes I needed to make it work: - Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. - Disabled flag 4373 for msvs because it was disabled in build/common.gypi. BUG=webrtc:6038 ========== to ========== GN: Add target for modules_tests. Additional changes I needed to make it work: - Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. - Disabled flag 4373 for msvs because it was disabled in build/common.gypi. BUG=webrtc:6038 TBR=kwiberg@webrtc.org ==========
I added TBR=kwiberg@webrtc.org for the .cc change. Please address my comments first though. To save some time since this is a compile-only change, you can set NOTRY=True on the CL and then just fire off compile trybots manually. I usually use an alias like this for that: alias trycompile='git cl try --bot=win_x64_gn_rel --bot=win_x64_gn_dbg --bot=win_x64_clang_rel --bot=win_x64_clang_dbg --bot=win_compile_x64_rel --bot=win_compile_rel --bot=win_compile_dbg --bot=win_compile_x64_dbg --bot=win_clang_rel --bot=mac_gn_rel --bot=mac_gn_dbg --bot=win_clang_dbg --bot=linux_gn_rel --bot=mac_compile_dbg --bot=mac_compile_rel --bot=linux_compile_rel --bot=linux_gn_dbg --bot=ios_rel --bot=linux_compile_dbg --bot=ios_arm64_dbg --bot=ios_arm64_rel --bot=ios_dbg --bot=ios64_sim_dbg --bot=ios32_sim_dbg --bot=android_gn_rel --bot=android_gn_dbg --bot=android_compile_x86_dbg --bot=android_compile_x64_dbg --bot=android_compile_dbg --bot=android_compile_arm64_rel --bot=android_compile_rel --bot=android_compile_arm64_dbg --bot=android_clang_dbg --bot=ios64_gn_rel --bot=ios64_gn_dbg -m tryserver.webrtc' https://codereview.webrtc.org/2187563005/diff/80001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/2187563005/diff/80001/webrtc/modules/BUILD.gn#n... webrtc/modules/BUILD.gn:84: if (is_ios) { Make this if (is_android || is_ios) { instead. I think there are some errors in other tests as well regarding this (we only run a few tests on iOS so that's probably why it's not been detected). Also the GN trybots don't run the Android tests yet, since we have limited machines with Android devices. I was planning on going through the Android tests once we got everything else in place. https://codereview.webrtc.org/2187563005/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/video_coding.gni (right): https://codereview.webrtc.org/2187563005/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_coding.gni:11: videoprocessor_defines = [] We don't need separate .gni file here (yet). It should be done only if we see the need of including it from more than one location. For now I suggest moving this into the BUILD.gn instead.
PTAL https://codereview.webrtc.org/2187563005/diff/80001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/2187563005/diff/80001/webrtc/modules/BUILD.gn#n... webrtc/modules/BUILD.gn:84: if (is_ios) { On 2016/08/03 15:22:49, kjellander_webrtc wrote: > Make this > if (is_android || is_ios) { > instead. I think there are some errors in other tests as well regarding this (we > only run a few tests on iOS so that's probably why it's not been detected). Also > the GN trybots don't run the Android tests yet, since we have limited machines > with Android devices. I was planning on going through the Android tests once we > got everything else in place. Done. https://codereview.webrtc.org/2187563005/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/video_coding.gni (right): https://codereview.webrtc.org/2187563005/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_coding.gni:11: videoprocessor_defines = [] On 2016/08/03 15:22:49, kjellander_webrtc wrote: > We don't need separate .gni file here (yet). > It should be done only if we see the need of including it from more than one > location. For now I suggest moving this into the BUILD.gn instead. Done. https://codereview.webrtc.org/2187563005/diff/100001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/2187563005/diff/100001/webrtc/modules/BUILD.gn#... webrtc/modules/BUILD.gn:42: videoprocessor_defines = [] Is this the right place to put this? I'd say so, since is not used anywhere else.
lgtm Please run the trybots I mentioned in previous comment, and set NOTRY=True for the description before landing (assuming all trybots goes green)
(answering the question) https://codereview.webrtc.org/2187563005/diff/100001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/2187563005/diff/100001/webrtc/modules/BUILD.gn#... webrtc/modules/BUILD.gn:42: videoprocessor_defines = [] On 2016/08/03 15:40:44, ehmaldonado_webrtc wrote: > Is this the right place to put this? > I'd say so, since is not used anywhere else. Yeah, close to where it's used is best IMO
Description was changed from ========== GN: Add target for modules_tests. Additional changes I needed to make it work: - Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. - Disabled flag 4373 for msvs because it was disabled in build/common.gypi. BUG=webrtc:6038 TBR=kwiberg@webrtc.org ========== to ========== GN: Add target for modules_tests. Additional changes I needed to make it work: - Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. - Disabled flag 4373 for msvs because it was disabled in build/common.gypi. BUG=webrtc:6038 TBR=kwiberg@webrtc.org NOTRY=True ==========
The CQ bit was checked by ehmaldonado@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@webrtc.org Link to the patchset: https://codereview.webrtc.org/2187563005/#ps100001 (title: "Adressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== GN: Add target for modules_tests. Additional changes I needed to make it work: - Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. - Disabled flag 4373 for msvs because it was disabled in build/common.gypi. BUG=webrtc:6038 TBR=kwiberg@webrtc.org NOTRY=True ========== to ========== GN: Add target for modules_tests. Additional changes I needed to make it work: - Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. - Disabled flag 4373 for msvs because it was disabled in build/common.gypi. BUG=webrtc:6038 TBR=kwiberg@webrtc.org NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== GN: Add target for modules_tests. Additional changes I needed to make it work: - Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. - Disabled flag 4373 for msvs because it was disabled in build/common.gypi. BUG=webrtc:6038 TBR=kwiberg@webrtc.org NOTRY=True ========== to ========== GN: Add target for modules_tests. Additional changes I needed to make it work: - Modified a header in RTPFile.cc. Every other file is using "webrtc/engine_configurations.h" instead. - Disabled flag 4373 for msvs because it was disabled in build/common.gypi. BUG=webrtc:6038 TBR=kwiberg@webrtc.org NOTRY=True Committed: https://crrev.com/f98dc105bae202b51e16c88308997d47fc842a5a Cr-Commit-Position: refs/heads/master@{#13628} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f98dc105bae202b51e16c88308997d47fc842a5a Cr-Commit-Position: refs/heads/master@{#13628} |
