Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14283017:
Stop build on failed device status check, apk install and add CheckInstall step. (Closed)
|
Created:
7 years, 7 months ago by navabi Modified:
7 years, 7 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, klundberg+watch_chromium.org, ilevy+watch_chromium.org, frankf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionStop build on failed device status check, apk install and add CheckInstall step.
BUG=230970, 236035
R=ilevy@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200020
Patch Set 1 #
Total comments: 13
Patch Set 2 : Raise exception instead of exit and fail on all non-88 error codes. #Patch Set 3 : Added TODO to change apk used to install. #Patch Set 4 : Added HelloWorld apk to install instead of SdkController. #
Total comments: 1
Patch Set 5 : Don't use constant for check install for loop. #Patch Set 6 : Only try to install one time. #Patch Set 7 : DO NOT COMMIT: Print statement to test on try bot. #Patch Set 8 : Remove debug print (same as patch set 6). #Patch Set 9 : Rebased. #Patch Set 10 : Rebased. #Patch Set 11 : Rebased off origin. #
Messages
Total messages: 29 (0 generated)
https://codereview.chromium.org/14283017/diff/1/build/android/buildbot/bb_dev... File build/android/buildbot/bb_device_steps.py (right): https://codereview.chromium.org/14283017/diff/1/build/android/buildbot/bb_dev... build/android/buildbot/bb_device_steps.py:194: RunCmd(['build/android/adb_install_apk.py'] + args, halt_on_failure=True) This will make the build stop if an apk install fails. Before any call to InstallApk (most come from RunInstrumentationSuite), there is a new CheckInstall() step to make sure APKs can be installed on attached devices. Thus, that should catch most issues, but this will fail if something goes wrong between the CheckInstall() step and the step to run the instrumentation tests. Note: Unit tests do not install via InstallAPK. It installs in test_package_apk.StripAndCopyExecutable which calls ManagedInstall. Thus, CheckInstall is still needed as it is a step that is run regardless of what tests are run.
https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:91: if code == 1 and halt_on_failure: How about if "code != 88" https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:92: exit(1) How about an exception instead of sys.exit: raise subprocess.CalledProcessError() https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:194: RunCmd(['build/android/adb_install_apk.py'] + args, halt_on_failure=True) On 2013/04/27 00:25:46, navabi wrote: > This will make the build stop if an apk install fails. Before any call to > InstallApk (most come from RunInstrumentationSuite), there is a new > CheckInstall() step to make sure APKs can be installed on attached devices. > Thus, that should catch most issues, but this will fail if something goes wrong > between the CheckInstall() step and the step to run the instrumentation tests. > > Note: Unit tests do not install via InstallAPK. It installs in > test_package_apk.StripAndCopyExecutable which calls ManagedInstall. Thus, > CheckInstall is still needed as it is a step that is run regardless of what > tests are run. I'm confused why this isn't sufficient -- why do we need the check install?
https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:92: exit(1) On 2013/04/30 20:01:16, Isaac wrote: > How about an exception instead of sys.exit: > > raise subprocess.CalledProcessError() actually looks like that exception takes arguments. raise OSError() would work.
https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:91: if code == 1 and halt_on_failure: On 2013/04/30 20:01:16, Isaac wrote: > How about if "code != 88" Done (assuming you mean != 88 and != 0). https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:92: exit(1) On 2013/04/30 20:03:38, Isaac wrote: > On 2013/04/30 20:01:16, Isaac wrote: > > How about an exception instead of sys.exit: > > > > raise subprocess.CalledProcessError() > > actually looks like that exception takes arguments. > > raise OSError() would work. Done. https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:194: RunCmd(['build/android/adb_install_apk.py'] + args, halt_on_failure=True) On 2013/04/30 20:01:16, Isaac wrote: > On 2013/04/27 00:25:46, navabi wrote: > > This will make the build stop if an apk install fails. Before any call to > > InstallApk (most come from RunInstrumentationSuite), there is a new > > CheckInstall() step to make sure APKs can be installed on attached devices. > > Thus, that should catch most issues, but this will fail if something goes > wrong > > between the CheckInstall() step and the step to run the instrumentation tests. > > > > Note: Unit tests do not install via InstallAPK. It installs in > > test_package_apk.StripAndCopyExecutable which calls ManagedInstall. Thus, > > CheckInstall is still needed as it is a step that is run regardless of what > > tests are run. > > I'm confused why this isn't sufficient -- why do we need the check install? Because not all test suites install apk's with adb_install_apk.py. This is sufficient for RunInstrumentationSuite, since it calls InstallApk. The unit tests, for example, do not install the apk using InstallApk (or even adb_install_apk.py). They install with ManagedInstall in android_commands.py. Thus, CheckInstall is needed to cover all cases.
https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:194: RunCmd(['build/android/adb_install_apk.py'] + args, halt_on_failure=True) On 2013/04/30 20:01:16, Isaac wrote: > On 2013/04/27 00:25:46, navabi wrote: > > This will make the build stop if an apk install fails. Before any call to > > InstallApk (most come from RunInstrumentationSuite), there is a new > > CheckInstall() step to make sure APKs can be installed on attached devices. > > Thus, that should catch most issues, but this will fail if something goes > wrong > > between the CheckInstall() step and the step to run the instrumentation tests. > > > > Note: Unit tests do not install via InstallAPK. It installs in > > test_package_apk.StripAndCopyExecutable which calls ManagedInstall. Thus, > > CheckInstall is still needed as it is a step that is run regardless of what > > tests are run. > > I'm confused why this isn't sufficient -- why do we need the check install? For more information see the Note on the comment above. It explains the same thing.
https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:194: RunCmd(['build/android/adb_install_apk.py'] + args, halt_on_failure=True) On 2013/04/30 21:41:46, navabi wrote: > On 2013/04/30 20:01:16, Isaac wrote: > > On 2013/04/27 00:25:46, navabi wrote: > > > This will make the build stop if an apk install fails. Before any call to > > > InstallApk (most come from RunInstrumentationSuite), there is a new > > > CheckInstall() step to make sure APKs can be installed on attached devices. > > > Thus, that should catch most issues, but this will fail if something goes > > wrong > > > between the CheckInstall() step and the step to run the instrumentation > tests. > > > > > > Note: Unit tests do not install via InstallAPK. It installs in > > > test_package_apk.StripAndCopyExecutable which calls ManagedInstall. Thus, > > > CheckInstall is still needed as it is a step that is run regardless of what > > > tests are run. > > > > I'm confused why this isn't sufficient -- why do we need the check install? > > Because not all test suites install apk's with adb_install_apk.py. This is > sufficient for RunInstrumentationSuite, since it calls InstallApk. The unit > tests, for example, do not install the apk using InstallApk (or even > adb_install_apk.py). They install with ManagedInstall in android_commands.py. > Thus, CheckInstall is needed to cover all cases. That makes sense. Maybe in those cases we could change the test suite to early terminate if install falls (it may do so already). Frank, would be OK w/ us changing run_tests to return warning code 88 on non-fatal errors (i.e. test failures)? https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:288: RunCmd(['build/android/device_status_check.py'], halt_on_failure=True) devise_status_check returns error codes on things that should not halt the build. Script needs to be changed to return warning code. Warning code (88) should probably be put in pylib/constants.py.
https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:288: RunCmd(['build/android/device_status_check.py'], halt_on_failure=True) On 2013/04/30 21:53:17, Isaac wrote: > devise_status_check returns error codes on things that should not halt the > build. Script needs to be changed to return warning code. > > Warning code (88) should probably be put in pylib/constants.py. I don't think that is the case. The main function of device_status_check will either exit normally or return with code 1 if there are no devices attached. What other error codes may be returned by device_status_check with should not halt the build?
https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/14283017/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:288: RunCmd(['build/android/device_status_check.py'], halt_on_failure=True) On 2013/04/30 21:59:56, navabi wrote: > On 2013/04/30 21:53:17, Isaac wrote: > > devise_status_check returns error codes on things that should not halt the > > build. Script needs to be changed to return warning code. > > > > Warning code (88) should probably be put in pylib/constants.py. > > I don't think that is the case. The main function of device_status_check will > either exit normally or return with code 1 if there are no devices attached. > > What other error codes may be returned by device_status_check with should not > halt the build? Sorry, yes you're right. I forgot Siva changed this is a recent CL.
PTAL.
I still have an outstanding comment on patchset 1..
On 2013/05/02 23:12:07, Isaac wrote: > I still have an outstanding comment on patchset 1.. I don't think we should have halt on failure on all the different test suites. It gets messy as there are 6 different test suites that install in at least 4 different ways via scripts throughout the source tree (e.g. run_browser_tests.py runbuildbot_steps.py, etc.). That is the reason we have the CheckInstall step before running any of the test suites. The instrumentation tests halt on failure is there because it is called in this script. Also the bug is a P1, so even if we decide to put the halt-on-failures in, I think this one should go in as is, so that builds stop when CheckInstall fails.
On 2013/05/03 00:18:03, navabi wrote:
> On 2013/05/02 23:12:07, Isaac wrote:
> > I still have an outstanding comment on patchset 1..
>
> I don't think we should have halt on failure on all the different test suites.
> It gets messy as there are 6 different test suites that install in at least 4
> different ways via scripts throughout the source tree (e.g.
run_browser_tests.py
> runbuildbot_steps.py, etc.).
>
> That is the reason we have the CheckInstall step before running any of the
test
> suites. The instrumentation tests halt on failure is there because it is
called
> in this script.
>
> Also the bug is a P1, so even if we decide to put the halt-on-failures in, I
> think this one should go in as is, so that builds stop when CheckInstall
fails.
Added TODO to potentially use a different apk. We can add a build target, step
to build an empty APK. There are two options, empty apk from native code or
empty apk from java code. I'm not convinced this is worth it.
Empty apk from native code requires an empty shared library the way
apk_test.gypi currently works.
Empty apk from java require empty Java activity the way java_apk.gypi works.
I tried an empty Java activity, looking at multiple_proguards.apk which creates
a dummy apk, but that apk does not install on devices:
/build/android/tests/multiple_proguards/multiple_proguards.gyp:multiple_proguards_test_apk
Insatalling MultipleProguards.apk fails on [INSTALL_FAILED_INVALID_APK]). I
talked to cjhopman and the gyp rules to build the apk need to be investigated.
I prefer we land this as is, as the SdkController.apk installs properly and fast
(> .03 secs).
2864 KB/s (73873 bytes in 0.025s)
pkg: /data/local/tmp/SdkControllerApp.apk
Failure [INSTALL_FAILED_ALREADY_EXISTS]
> I prefer we land this as is, as the SdkController.apk installs properly and
fast
> (> .03 secs).
>
> 2864 KB/s (73873 bytes in 0.025s)
> pkg: /data/local/tmp/SdkControllerApp.apk
> Failure [INSTALL_FAILED_ALREADY_EXISTS]
Sorry. Here is the output from installing SdkControllerApp.apk fast;
navabi@navabi1:/usr/local/google/code/clankium-master2/src$
./build/android/adb_install_apk.py --apk third_party/android_tools/sdk/t\
ools/apps/SdkController/bin/SdkControllerApp.apk
----- Installed on 014696590800801A -----
3467 KB/s (73873 bytes in 0.020s)
pkg: /data/local/tmp/SdkControllerApp.apk
Success
What does SdkController do? I'm uneasy installing a random apk even if it's already in our tree? Why not look at the source of SdkController, remove everything non-essential, rebuild and check in an empty apk.
On 2013/05/07 01:46:08, Isaac wrote: > What does SdkController do? I'm uneasy installing a random apk even if it's > already in our tree? Why not look at the source of SdkController, remove > everything non-essential, rebuild and check in an empty apk. There is no need to look at the source of SdkController and work backwards to make an empty apk. Building and checking in an APK that does nothing is not the problem. The problem is adding a new gyp rule (using our existing gypi files) to build an empty APK for the CheckInstall step to install. I am against adding make targets to build the APK that is installed, as this will add an unnecessary dependence on the step to make the APK. I don't have a strong feeling against checking in an APK file in the tree. SDKController was nice because it was there in the SDK. It allows controlling an Android Emulator from a device (https://play.google.com/store/apps/details?id=com.esemobilesoftware.android.t...). I'll check in an APK that does nothing into build/android and install that.
On 2013/05/07 11:01:15, navabi wrote: > On 2013/05/07 01:46:08, Isaac wrote: > > What does SdkController do? I'm uneasy installing a random apk even if it's > > already in our tree? Why not look at the source of SdkController, remove > > everything non-essential, rebuild and check in an empty apk. > > There is no need to look at the source of SdkController and work backwards to > make an empty apk. Building and checking in an APK that does nothing is not the > problem. > > The problem is adding a new gyp rule (using our existing gypi files) to build an > empty APK for the CheckInstall step to install. I am against adding make targets > to build the APK that is installed, as this will add an unnecessary dependence > on the step to make the APK. > > I don't have a strong feeling against checking in an APK file in the tree. > SDKController was nice because it was there in the SDK. It allows controlling an > Android Emulator from a device > (https://play.google.com/store/apps/details?id=com.esemobilesoftware.android.t...). > I'll check in an APK that does nothing into build/android and install that. Thanks Isaac. PTAL. Checked in HelloWorld apk to install instead of SdkController (even faster to install too). ----- Installed on 014696590800801A ----- 2409 KB/s (37106 bytes in 0.015s) pkg: /data/local/tmp/CheckInstallApk-debug.apk Success
LGTM. Make sure your android manifest does not require jelly bean (preferably this will install on gingerbread in case we ever want to test that).
https://codereview.chromium.org/14283017/diff/22001/build/android/buildbot/bb... File build/android/buildbot/bb_device_steps.py (right): https://codereview.chromium.org/14283017/diff/22001/build/android/buildbot/bb... build/android/buildbot/bb_device_steps.py:173: for _ in range(NUM_CHECK_INSTALL): Actually, why try to install multiple times? If we want to do this loop, just put the number 3 inline here instead of making a static constant.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/14283017/29001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/14283017/42001
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is android_dbg, revision is HEAD
On 2013/05/09 06:18:26, I haz the power (commit-bot) wrote: > The commit queue went berserk retrying too often for a > seemingly flaky test. Builder is android_dbg, revision is HEAD The try bot does not seem to be adding the CheckInstall apk correctly. To test, I added a print in the code that prints the file info for the apk. On the try bot, st_size=0. Locally, the file updated, st_size=37106. Any ideas? On try bot: CheckInstallApk-debug.apk info: posix.stat_result(st_mode=33152, st_ino=5571001, st_dev=2053L, st_nlink=1, st_uid=1000, st_gid=1000, st_size=0, st_atime=1368152137, st_mtime=1368152137, st_ctime=1368152137) Locally: CheckInstallApk-debug.apk info: posix.stat_result(st_mode=33184, st_ino=26904654, st_dev=64513L, st_nlink=1, st_uid=138869, st_gid=5000, st_size=37106, st_atime=1368149787, st_mtime=1367948922, st_ctime=1367948922)
Any ideas on why I can't dcommit this change. I get a list of warnings about being unable to rm directories, and then it seems to hang: ... warning: unable to rmdir tools/swarm_client: Directory not empty warning: unable to rmdir v8: Directory not empty warning: unable to rmdir webkit/media/crypto/ppapi/cdm: Directory not empty Switched to branch 'git-cl-cherry-pick'
I think those are normal warnings that always show up. Try running git cl presubmit and making sure that completes. If it does, make sure you've synced and are rebased off of origin.
I have done all those things. Presumbit passes and completes. I have rebased off origin. The dcommit does not complete. It gives the following error: Switched to branch 'git-cl-cherry-pick' Unable to determine upstream SVN information from HEAD history. Perhaps the repository is empty. at /usr/lib/git-core/git-svn line 845. On Mon, May 13, 2013 at 2:02 PM, <ilevy@chromium.org> wrote: > I think those are normal warnings that always show up. Try running git > cl > presubmit and making sure that completes. > > If it does, make sure you've synced and are rebased off of origin. > > https://codereview.chromium.**org/14283017/<https://codereview.chromium.org/1... >
Have you initialized git-svn in your repo? If not, you have to do that before you can dcommit. On Mon, May 13, 2013 at 7:29 PM, Armand Navabi <navabi@google.com> wrote: > I have done all those things. Presumbit passes and completes. I have > rebased off origin. > The dcommit does not complete. It gives the following error: > > Switched to branch 'git-cl-cherry-pick' > > Unable to determine upstream SVN information from HEAD history. > Perhaps the repository is empty. at /usr/lib/git-core/git-svn line 845. > > > > On Mon, May 13, 2013 at 2:02 PM, <ilevy@chromium.org> wrote: > >> I think those are normal warnings that always show up. Try running git >> cl >> presubmit and making sure that completes. >> >> If it does, make sure you've synced and are rebased off of origin. >> >> https://codereview.chromium.**org/14283017/<https://codereview.chromium.org/1... >> > >
Message was sent while issue was closed.
Committed patchset #11 manually as r200020 (presubmit successful).
Message was sent while issue was closed.
On 2013/05/14 02:49:11, Isaac wrote: > Have you initialized git-svn in your repo? If not, you have to do that > before you can dcommit. > > > On Mon, May 13, 2013 at 7:29 PM, Armand Navabi <mailto:navabi@google.com> wrote: > > > I have done all those things. Presumbit passes and completes. I have > > rebased off origin. > > The dcommit does not complete. It gives the following error: > > > > Switched to branch 'git-cl-cherry-pick' > > > > Unable to determine upstream SVN information from HEAD history. > > Perhaps the repository is empty. at /usr/lib/git-core/git-svn line 845. > > > > > > > > On Mon, May 13, 2013 at 2:02 PM, <mailto:ilevy@chromium.org> wrote: > > > >> I think those are normal warnings that always show up. Try running git > >> cl > >> presubmit and making sure that completes. > >> > >> If it does, make sure you've synced and are rebased off of origin. > >> > >> > https://codereview.chromium.**org/14283017/%3Chttps://codereview.chromium.org...> > >> > > > > That was it. Thanks Isaac. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
