Can you point out an example build where we try to run `su -c sh -c ndc resolver flushdefaultif`?

I looked through a few builds and it appears that we're behaving as intended:

 -  telemetry attempts to enable root:

(CRITICAL) 2017-04-06 10:38:28,047 timeout_retry.Run:174  (TimeoutThread-3-for-MainThread) Exception on EnableRoot(0324216001899, retries=3, timeout=30), attempt 3 of 4: CommandFailedError('(device: 0324216001899) Cannot enable root in user builds.',)
(WARNING) 2017-04-06 10:38:28,048 android_platform_backend.__init__:98  Unable to root 0324216001899

 - we check if su is available:

(INFO) 2017-04-06 10:38:28,048 cmd_helper._ValidateAndLogCommand:161  [host]> /b/c/b/Android_Release__NVIDIA_Shield_TV_/irJqI1GK/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb -s 0324216001899 shell '( su 0 ls /root && ! ls /root );echo %$?'

 - it isn't, so telemetry runs the ndc command without su:

(INFO) 2017-04-06 10:39:00,582 cmd_helper._ValidateAndLogCommand:161  [host]> /b/c/b/Android_Release__NVIDIA_Shield_TV_/irJqI1GK/third_party/catapult/devil/bin/deps/linux2/x86_64/bin/adb -s 0324216001899 shell '( ndc resolver flushdefaultif );echo %$?'

Ah, so that's what's happening! Sorry for the noise, "su -c sh -c ndc resolver flushdefaultif" was my misinterpretation of the code.

BTW, why does it fail with "Cannot enable root in user builds"?
johnw@ said that "adb root" succeeds on the device. Maybe there is something wrong with "self.IsUserBuild()" check?

We just check whether it's a user build and bail w/o attempting to run `adb root` if so: https://codesearch.chromium.org/chromium/src/third_party/catapult/devil/devil/android/device_utils.py?rcl=9a55abab029cb9ae94f5160ded11b09a4638a955&l=447

Perhaps Nvidia reports wrong build type, i.e. "user" and not "userdebug"?
The device is disconnected now, so I can't check.
Would it be safe to skip this check and try "adb root" and see if it succeeds?

So, I've verified and Nvidia's image says it's "user", but "adb root" works fine.
This telemetry restriction was introduced in https://codereview.chromium.org/772003002 without any justification.
The previous version removed in https://codereview.chromium.org/1290773009 didn't seem to have this restriction.
My guess it's an optimization?

One note, old code wanted to see "restarting adbd as root" in the output, but in this case, "adb root" is silent (if was not root before, does say "adbd is already running as root" if is already root).

Wow, are those CLs a flashback :)

I believe we did this because none of the devices we typically use support adb root for user builds and the current implementation presents a much clearer error message. (That it's slightly faster is nice but probably was not the motivation.)

I think our potential options here are:
 1) Remove the user build check from DeviceUtils.EnableRoot. I'm not sure that we want to try to root a user build device in the general case, as it generally shouldn't work, so I don't really like this option.
 2) Add a flag to DeviceUtils.EnableRoot that skips the user-build check (w/ the default value not skipping it). I'm generally not wild about flags, but this could be a reasonable place for one.
 3) Don't change DeviceUtils, and make clients who wish to do this kind of thing call AdbWrapper.Root() on their own. My concern here would be that clients would be liable to forget to wait for the device to come back after rooting.

I guess I'd lean toward #2, but I'm interested to hear the others' thoughts.

(also, I wouldn't use the old code as a reference for a lot of reasons.)

I would change EnableRoot to something like:

try:
  adb.Root()  # If it works, great. Whatever.
except:
  if device.IsUserBuild():
    raise Exception('Trying to enable root un user build, generally that doesn't work!')
  else:
    raise  # Just re-raise.

Eh, I could be ok with that.

Ok, I'll take this.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c375deb97e7a347219d0fadc843131592285b93

commit 0c375deb97e7a347219d0fadc843131592285b93
Author: catapult-deps-roller@chromium.org <catapult-deps-roller@chromium.org>
Date: Wed Apr 12 19:40:52 2017

Roll src/third_party/catapult/ f57871dac..90b692aaf (4 commits)

https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git/+log/f57871dac2cd..90b692aafa2d

$ git log f57871dac..90b692aaf --date=short --no-merges --format='%ad %ae %s'
2017-04-12 nednguyen Remove common/ module's dependency on telemetry/
2017-04-12 mikecase Fix force stop using wrong "ps" cmd.
2017-04-12 mikecase Reenable testPersistentShell in adb_wrapper_devicetest.py
2017-04-12 perezju [devil] If possible, allow to enable root on user builds.

Created with:
  roll-dep src/third_party/catapult
BUG=707319, 710479 


Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=sullivan@chromium.org

Change-Id: Ib321a42e166102e0f5aaa7ba2031b027aec32799
Reviewed-on: https://chromium-review.googlesource.com/475756
Reviewed-by: <catapult-deps-roller@chromium.org>
Commit-Queue: <catapult-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#464113}
[modify] https://crrev.com/0c375deb97e7a347219d0fadc843131592285b93/DEPS

Seems to work:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Android%20Release%20%28NVIDIA%20Shield%20TV%29/builds/275

Thanks!