Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767383005:
Add Windows, Mac, Android support to low bandwidth audio test (Closed)
|
Created:
3 years, 9 months ago by oprypin_webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, tlegrand-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd Windows, Mac, Android support to low bandwidth audio test
BUG=webrtc:7229
Review-Url: https://codereview.webrtc.org/2767383005
Cr-Commit-Position: refs/heads/master@{#17470}
Committed: https://chromium.googlesource.com/external/webrtc/+/6d305baa04da51ab4229d22e05b27722b10d7bd0
Patch Set 1 : Add Windows, Mac, Android support to low bandwidth audio test #Patch Set 2 : Remove unnecessary addition of .exe #
Total comments: 10
Patch Set 3 : Add configurable adb_path #
Total comments: 1
Patch Set 4 : Add configurable adb_path #Patch Set 5 : Fix Android build errors #
Messages
Total messages: 37 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Add Windows, Mac, Android support to low bandwidth audio test BUG=webrtc:7229 ========== to ========== Add Windows, Mac, Android support to low bandwidth audio test BUG=webrtc:7229 ==========
oprypin@webrtc.org changed reviewers: + kjellander@webrtc.org, stefan@webrtc.org
iOS builds are failing as expected due to a bug in the infrastructure when changing from rtc_executable to rtc_test. Will require a landmine. https://chromium.googlesource.com/external/webrtc/+/master/tools-webrtc/get_l...
https://codereview.webrtc.org/2767383005/diff/60001/tools-webrtc/download_too... File tools-webrtc/download_tools.py (right): https://codereview.webrtc.org/2767383005/diff/60001/tools-webrtc/download_too... tools-webrtc/download_tools.py:23: # Needed to properly resolve PATH and executable extensions on Windows. Skip the "and executable extensions" part in this script, since it's not used. https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:66: pesq_path = os.path.join(toolchain_dir, _GetPlatform(), 'pesq') What about .exe on Windows?
https://codereview.webrtc.org/2767383005/diff/60001/tools-webrtc/download_too... File tools-webrtc/download_tools.py (right): https://codereview.webrtc.org/2767383005/diff/60001/tools-webrtc/download_too... tools-webrtc/download_tools.py:23: # Needed to properly resolve PATH and executable extensions on Windows. On 2017/03/27 09:05:10, kjellander_webrtc wrote: > Skip the "and executable extensions" part in this script, since it's not used. I think it is actually still relevant to resolve the extension for download_from_google_storage.bat - I read somewhere that only .exe are resolved automatically but can't find that reference anymore. https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:66: pesq_path = os.path.join(toolchain_dir, _GetPlatform(), 'pesq') On 2017/03/27 09:05:10, kjellander_webrtc wrote: > What about .exe on Windows? Works fine (tested), .exe is resolved automatically.
https://codereview.webrtc.org/2767383005/diff/60001/tools-webrtc/download_too... File tools-webrtc/download_tools.py (right): https://codereview.webrtc.org/2767383005/diff/60001/tools-webrtc/download_too... tools-webrtc/download_tools.py:23: # Needed to properly resolve PATH and executable extensions on Windows. On 2017/03/27 09:19:07, oprypin_webrtc wrote: > On 2017/03/27 09:05:10, kjellander_webrtc wrote: > > Skip the "and executable extensions" part in this script, since it's not used. > > I think it is actually still relevant to resolve the extension for > download_from_google_storage.bat - I read somewhere that only .exe are resolved > automatically but can't find that reference anymore. Ah, I see now. I somehow confused this in my head with that you would append extensions, but you're talking about using the shell to auto-resolve extensions. I'm sure windows cmd.exe auto-resolves .bat, so yes let's keep this as is. https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:66: pesq_path = os.path.join(toolchain_dir, _GetPlatform(), 'pesq') On 2017/03/27 09:19:07, oprypin_webrtc wrote: > On 2017/03/27 09:05:10, kjellander_webrtc wrote: > > What about .exe on Windows? > > Works fine (tested), .exe is resolved automatically. Aha, even without shell=True? My mistake then. https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:76: adb_command = ['adb', 'pull', file_path, out_dir] adb is not normally in PATH unless you've first sourced build/android/envsetup.sh. We could make sure it is for the bots, but ideally you wouldn't need to remember having sourced build/android/envsetup.sh in order to run this script.
https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:76: adb_command = ['adb', 'pull', file_path, out_dir] On 2017/03/27 09:46:51, kjellander_webrtc wrote: > adb is not normally in PATH unless you've first sourced > build/android/envsetup.sh. We could make sure it is for the bots, but ideally > you wouldn't need to remember having sourced build/android/envsetup.sh in order > to run this script. I'm not even sure how we can improve this. I was looking at https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/8078c53c... - it doesn't seem to do anything about this.
https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:76: adb_command = ['adb', 'pull', file_path, out_dir] On 2017/03/27 09:59:41, oprypin_webrtc wrote: > On 2017/03/27 09:46:51, kjellander_webrtc wrote: > > adb is not normally in PATH unless you've first sourced > > build/android/envsetup.sh. We could make sure it is for the bots, but ideally > > you wouldn't need to remember having sourced build/android/envsetup.sh in > order > > to run this script. > > I'm not even sure how we can improve this. I was looking at > https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/8078c53c... > - it doesn't seem to do anything about this. Since the bots try hard to keep clean environments, but has ways to figure out the path to adb [1], let's introduce an --adb-path variable, like this script does [2]. Then it will just default to 'adb' if you don't pass the flag (only bots will do). [1]: https://cs.corp.google.com/chromium_build/scripts/slave/recipe_modules/chromi... [2]: https://cs.chromium.org/chromium/build/scripts/slave/android/authorize_adb_de...
https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2767383005/diff/60001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:76: adb_command = ['adb', 'pull', file_path, out_dir] On 2017/03/27 12:21:55, kjellander_webrtc wrote: > On 2017/03/27 09:59:41, oprypin_webrtc wrote: > > On 2017/03/27 09:46:51, kjellander_webrtc wrote: > > > adb is not normally in PATH unless you've first sourced > > > build/android/envsetup.sh. We could make sure it is for the bots, but > ideally > > > you wouldn't need to remember having sourced build/android/envsetup.sh in > > order > > > to run this script. > > > > I'm not even sure how we can improve this. I was looking at > > > https://chromium.googlesource.com/external/webrtc/trunk/webrtc.git/+/8078c53c... > > - it doesn't seem to do anything about this. > > Since the bots try hard to keep clean environments, but has ways to figure out > the path to adb [1], let's introduce an --adb-path variable, like this script > does [2]. Then it will just default to 'adb' if you don't pass the flag (only > bots will do). > > [1]: > https://cs.corp.google.com/chromium_build/scripts/slave/recipe_modules/chromi... > [2]: > https://cs.chromium.org/chromium/build/scripts/slave/android/authorize_adb_de... Done.
lgtm with a suggestion. https://codereview.webrtc.org/2767383005/diff/80001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.py (right): https://codereview.webrtc.org/2767383005/diff/80001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.py:71: def _GetFile(file_path, out_dir, android=False, adb_path='adb'): Let's have the default argument be None instead, since otherwise we'll have two locations for the default, which is a little confusing IMO.
Not sure what you want me to review. I didn't spend much time on the python script, but everything else lgtm
kjellander@webrtc.org changed reviewers: + solenberg@webrtc.org
We need an owner from https://chromium.googlesource.com/external/webrtc/+/master/webrtc/audio/OWNERS in addition to me and Stefan. Adding solenberg@
On 2017/03/28 16:21:54, kjellander_webrtc wrote: > We need an owner from > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/audio/OWNERS > in addition to me and Stefan. Adding solenberg@ lgtm
The CQ bit was checked by oprypin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2767383005/#ps100001 (title: "Add configurable adb_path")
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: android_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/22899)
On 2017/03/29 11:16:45, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_clang_dbg on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) > android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) > android_dbg on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/22899) Odd, we've seen this error when Mirko was doing work on GN templates for Android. Can you try a git cl try --bot=android_rel --clobber ? Then land another landmine to wipe Android (Linux) before submitting this, *sigh*
On 2017/03/29 11:34:17, kjellander_webrtc wrote: > On 2017/03/29 11:16:45, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > android_clang_dbg on master.tryserver.webrtc (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) > > android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) > > android_dbg on master.tryserver.webrtc (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/22899) > > Odd, we've seen this error when Mirko was doing work on GN templates for > Android. Can you try a git cl try --bot=android_rel --clobber ? > Then land another landmine to wipe Android (Linux) before submitting this, > *sigh* Did this slightly earlier, made no change: git cl try --bot=android_arm64_rel --clobber -m tryserver.webrtc
On 2017/03/29 11:36:17, oprypin_webrtc wrote: > On 2017/03/29 11:34:17, kjellander_webrtc wrote: > > On 2017/03/29 11:16:45, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > android_clang_dbg on master.tryserver.webrtc (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) > > > android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) > > > android_dbg on master.tryserver.webrtc (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/22899) > > > > Odd, we've seen this error when Mirko was doing work on GN templates for > > Android. Can you try a git cl try --bot=android_rel --clobber ? > > Then land another landmine to wipe Android (Linux) before submitting this, > > *sigh* > > Did this slightly earlier, made no change: > git cl try --bot=android_arm64_rel --clobber -m tryserver.webrtc Wow, that's annoying. So the GN change never compiled on Android? What if you change it back to rtc_executable, do the Android trybots pass? Just to isolate what's causing the problem here.
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
On 2017/03/29 11:43:27, kjellander (google.com) wrote: > On 2017/03/29 11:36:17, oprypin_webrtc wrote: > > On 2017/03/29 11:34:17, kjellander_webrtc wrote: > > > On 2017/03/29 11:16:45, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > android_clang_dbg on master.tryserver.webrtc (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...) > > > > android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) > > > > android_dbg on master.tryserver.webrtc (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/22899) > > > > > > Odd, we've seen this error when Mirko was doing work on GN templates for > > > Android. Can you try a git cl try --bot=android_rel --clobber ? > > > Then land another landmine to wipe Android (Linux) before submitting this, > > > *sigh* > > > > Did this slightly earlier, made no change: > > git cl try --bot=android_arm64_rel --clobber -m tryserver.webrtc > > Wow, that's annoying. So the GN change never compiled on Android? What if you > change it back to rtc_executable, do the Android trybots pass? Just to isolate > what's causing the problem here. OK, it was not easy to figure out, but the problem was caused by one rtc_test target depending on another rtc_test target. We routed dependencies in a different way here and reported a bug. http://crbug.com/706755
lgtm
The CQ bit was checked by oprypin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2767383005/#ps140001 (title: "Fix Android build errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490871112798920,
"parent_rev": "30cbd0ba8fb99e5323ad48a17644c358a9069cfd", "commit_rev":
"6d305baa04da51ab4229d22e05b27722b10d7bd0"}
Message was sent while issue was closed.
Description was changed from ========== Add Windows, Mac, Android support to low bandwidth audio test BUG=webrtc:7229 ========== to ========== Add Windows, Mac, Android support to low bandwidth audio test BUG=webrtc:7229 Review-Url: https://codereview.webrtc.org/2767383005 Cr-Commit-Position: refs/heads/master@{#17470} Committed: https://chromium.googlesource.com/external/webrtc/+/6d305baa04da51ab4229d22e0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/6d305baa04da51ab4229d22e0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
