Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
179333002:
Update android device provisioning script to set some reliability settings. (Closed)
|
Created:
6 years, 10 months ago by tonyg Modified:
6 years, 9 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpdate android device provisioning script to set some reliability settings.
BUG=309662
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253585
Patch Set 1 #
Total comments: 1
Patch Set 2 : bulach comments #
Total comments: 6
Patch Set 3 : Patch for landing #
Messages
Total messages: 14 (0 generated)
+klundberg since it may affect the recipes work. thanks tony! just some suggestions based on my understanding from the directories segregation we had, no idea if it's still relevant.. https://codereview.chromium.org/179333002/diff/1/build/android/pylib/bot_devi... File build/android/pylib/bot_device_settings.py (right): https://codereview.chromium.org/179333002/diff/1/build/android/pylib/bot_devi... build/android/pylib/bot_device_settings.py:22: 'airplane_mode_on': 1, iirc, there are some functional tests / bots that require network.. perhaps instead of a function, how about make it into a class DeviceConfigurator, then take options in its __init__ and/or expose individual settings so the caller can decide, depending on which bot it's running, what configurations it wants? also, there's a build/android/buildbot directory... the idea way back when was that "infra-team" owns buildbot, "test team" owns build/android(/pylib). I don't know if it's still relevant, but perhaps: 1) make this more generic, i.e., rename s/bot// and make it a component 2) put a wrapper in build/android/buildbot that uses this component also, I'm sure you're aware, but we do have downstream: c**nk/build/full_deploy.py which uses system_settings.py in that same directory.. I hope we'll be able to drop that soon? :)
> build/android/pylib/bot_device_settings.py:22: 'airplane_mode_on': 1, > iirc, there are some functional tests / bots that require network.. Good point. For the purpose of this patch, I decided to set it only for the perfbots and leave the functional bots non-deterministic in this regard. Someone can set it one way or the other in a subsequent patch. > perhaps instead of a function, how about make it into a class > DeviceConfigurator, then take options in its __init__ and/or expose individual > settings so the caller can decide, depending on which bot it's running, what > configurations it wants? Good idea. I reworked it as a file that contains dictionaries that one may want to use. Given that only 1 method is left, I don't think a class is warranted. > also, there's a build/android/buildbot directory... > the idea way back when was that "infra-team" owns buildbot, "test team" owns > build/android(/pylib). The reason I want the settings themselves in pylib is so that we can share with Telemetry so that developers can get their devices into a known perf testing state if desired. > I don't know if it's still relevant, but perhaps: > 1) make this more generic, i.e., rename s/bot// and make it a component Good point. Done. > 2) put a wrapper in build/android/buildbot that uses this component See above for why I want pylib. > also, I'm sure you're aware, but we do have downstream: > c**nk/build/full_deploy.py > which uses system_settings.py in that same directory.. Yes. That was the starting point for this change. However it was too ugly for my to consciously upstream. So I refactored it as I went along. > I hope we'll be able to drop that soon? :) We can definitely drop the old one after this lands.
lgtm, nice! quite like the new structure, thanks tony! ultra small nits, ship it! https://codereview.chromium.org/179333002/diff/20001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/179333002/diff/20001/build/android/provision_... build/android/provision_devices.py:72: local_props = ['ro.monkey=1', ultra nit: perhaps: local_props = [ ..., ..., ] https://codereview.chromium.org/179333002/diff/20001/build/android/provision_... build/android/provision_devices.py:95: if 'Perf' in os.environ.get('BUILDBOT_BUILDERNAME', ''): nit: s/Perf/ and use a .lower() so this would work downstream too ;) (and yes, we should eventually converge those names, but anyways..). https://codereview.chromium.org/179333002/diff/20001/build/android/pylib/devi... File build/android/pylib/device_settings.py (right): https://codereview.chromium.org/179333002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:28: print '\n', table, (80 - len(table)) * '-' log.debug (and 30 below) ?
https://codereview.chromium.org/179333002/diff/20001/build/android/provision_... File build/android/provision_devices.py (right): https://codereview.chromium.org/179333002/diff/20001/build/android/provision_... build/android/provision_devices.py:72: local_props = ['ro.monkey=1', On 2014/02/26 15:23:59, bulach wrote: > ultra nit: perhaps: > local_props = [ > ..., > ..., > ] Done. https://codereview.chromium.org/179333002/diff/20001/build/android/provision_... build/android/provision_devices.py:95: if 'Perf' in os.environ.get('BUILDBOT_BUILDERNAME', ''): On 2014/02/26 15:23:59, bulach wrote: > nit: s/Perf/ and use a .lower() > so this would work downstream too ;) > (and yes, we should eventually converge those names, but anyways..). Done. https://codereview.chromium.org/179333002/diff/20001/build/android/pylib/devi... File build/android/pylib/device_settings.py (right): https://codereview.chromium.org/179333002/diff/20001/build/android/pylib/devi... build/android/pylib/device_settings.py:28: print '\n', table, (80 - len(table)) * '-' On 2014/02/26 15:23:59, bulach wrote: > log.debug (and 30 below) ? Done.
The CQ bit was checked by tonyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tonyg@chromium.org/179333002/40001
lgtm. Thanks Tony!
Message was sent while issue was closed.
Change committed as 253585
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/181043008/ by tonyg@chromium.org. The reason for reverting is: Caused http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg....
Let's upgrade the devices on this bot as they are still on ICS. Added dprake. Can we skip JB and go straight to N4's on K? On Wed, Feb 26, 2014 at 5:38 PM, <tonyg@chromium.org> wrote: > A revert of this CL has been created in > https://codereview.chromium.org/181043008/ by tonyg@chromium.org. > > The reason for reverting is: Caused > > http://build.chromium.org/p/chromium.webkit/builders/ > Android%20Tests%20%28dbg%29/builds/17664/steps/provision_ > devices/logs/stdio. > > https://codereview.chromium.org/179333002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hm. I thought we had already updated everything to K? Fine by me. -- Dirk On Thu, Feb 27, 2014 at 11:19 AM, Armand Navabi <navabi@google.com> wrote: > Let's upgrade the devices on this bot as they are still on ICS. Added > dprake. Can we skip JB and go straight to N4's on K? > > > On Wed, Feb 26, 2014 at 5:38 PM, <tonyg@chromium.org> wrote: > >> A revert of this CL has been created in >> https://codereview.chromium.org/181043008/ by tonyg@chromium.org. >> >> The reason for reverting is: Caused >> >> http://build.chromium.org/p/chromium.webkit/builders/ >> Android%20Tests%20%28dbg%29/builds/17664/steps/provision_ >> devices/logs/stdio. >> >> https://codereview.chromium.org/179333002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Armand, what do you think about making the provision_device step fail if an unexpectedly old build is found? For instance, are you saying we should no longer have ICS anywhere? On Thu, Feb 27, 2014 at 11:19 AM, Dirk Pranke <dpranke@google.com> wrote: > Hm. I thought we had already updated everything to K? Fine by me. > > -- Dirk > > > On Thu, Feb 27, 2014 at 11:19 AM, Armand Navabi <navabi@google.com> wrote: > >> Let's upgrade the devices on this bot as they are still on ICS. Added >> dprake. Can we skip JB and go straight to N4's on K? >> >> >> On Wed, Feb 26, 2014 at 5:38 PM, <tonyg@chromium.org> wrote: >> >>> A revert of this CL has been created in >>> https://codereview.chromium.org/181043008/ by tonyg@chromium.org. >>> >>> The reason for reverting is: Caused >>> >>> http://build.chromium.org/p/chromium.webkit/builders/ >>> Android%20Tests%20%28dbg%29/builds/17664/steps/provision_ >>> devices/logs/stdio. >>> >>> https://codereview.chromium.org/179333002/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
No. I was just talking about the webkit bot. It is not the case that we should no longer have ICS bots. E.g. Android Tests (ICS GalaxyNexus)(dbg) linux on the upstream bot and the instrumentation-yakju-clankium-ics downstream among others. We support a lot of different device configurations and bots, so I do not think we want to fail if old builds are found. On Thu, Feb 27, 2014 at 11:28 AM, Tony Gentilcore <tonyg@chromium.org>wrote: > Armand, what do you think about making the provision_device step fail if > an unexpectedly old build is found? For instance, are you saying we should > no longer have ICS anywhere? > > > On Thu, Feb 27, 2014 at 11:19 AM, Dirk Pranke <dpranke@google.com> wrote: > >> Hm. I thought we had already updated everything to K? Fine by me. >> >> -- Dirk >> >> >> On Thu, Feb 27, 2014 at 11:19 AM, Armand Navabi <navabi@google.com>wrote: >> >>> Let's upgrade the devices on this bot as they are still on ICS. Added >>> dprake. Can we skip JB and go straight to N4's on K? >>> >>> >>> On Wed, Feb 26, 2014 at 5:38 PM, <tonyg@chromium.org> wrote: >>> >>>> A revert of this CL has been created in >>>> https://codereview.chromium.org/181043008/ by tonyg@chromium.org. >>>> >>>> The reason for reverting is: Caused >>>> >>>> http://build.chromium.org/p/chromium.webkit/builders/ >>>> Android%20Tests%20%28dbg%29/builds/17664/steps/provision_ >>>> devices/logs/stdio. >>>> >>>> https://codereview.chromium.org/179333002/ >>>> >>> >>> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
