[Telemetry] Detect whether sudo is required for certain admin-commands.

[Telemetry] Detect whether sudo is required for certain admin-commands.

BUG=337048
NOTRY=True

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247692

[tonyg, 2014-01-28 20:17:21]

This should allow our benchmarks to work on the bots and locally.

[jeremy, 2014-01-28 20:45:31]

I don't think this is going to work in it's current form, the reason is that
when you execute a command as sudo, you may prompt to enter a password - this
requires telemetry to .communicte() with the sudo-ed subprocess and block until
the user has typed in the password.

In the current patch the command is launched in sudo but the password is not
entered, this means, for example in the case of powermetrics - that we'll start
executing the benchmark, run powermetrics which will prompt for a password but
block while the rest of the benchmark runs in the background.

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/__init__.py:114: def
LaunchApplication(self, application, parameters=None, admin=False):
Can you document the parameters in the comment?

nit: How about renaming to 'root', 'requires_admin' or somesuch.

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/mac_platform_backend.py (right):

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/mac_platform_backend.py:165: os_version
= int(os.uname()[2].split('.')[0])
osx_10_9_or_later = (os_version >= 13) then use that for admin= ?

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/posix_platform_backend.py (right):

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:75: def
IsSticky(path):
IsSetUID() ?

[tonyg, 2014-01-29 00:05:14]

Great point. I fixed it up so that it works properly now. PTAL

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/__init__.py:114: def
LaunchApplication(self, application, parameters=None, admin=False):
On 2014/01/28 20:45:31, jeremy wrote:
> Can you document the parameters in the comment?
> 
> nit: How about renaming to 'root', 'requires_admin' or somesuch.

Done.

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/mac_platform_backend.py (right):

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/mac_platform_backend.py:165: os_version
= int(os.uname()[2].split('.')[0])
On 2014/01/28 20:45:31, jeremy wrote:
> osx_10_9_or_later = (os_version >= 13) then use that for admin= ?

Good idea, I went a step further and fixed the GetOSVersionName() method to work
here.

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/posix_platform_backend.py (right):

https://codereview.chromium.org/137143011/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:75: def
IsSticky(path):
On 2014/01/28 20:45:31, jeremy wrote:
> IsSetUID() ?

Done.

[jeremy, 2014-01-29 00:27:23]

Very nice!

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/mac_platform_backend.py (right):

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/mac_platform_backend.py:22: LEOPARD =   
  platform_backend.OSVersion('leopard',       9)
nit I'd add a # 10.6 ... # 10.9 at the end of each of these lines for
readability...

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/mac_platform_backend.py:174:
is_mavericks = self.GetOSVersionName() >= MAVERICKS
s/is_mavericks/mavericks_or_later/ ?
Also below.

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/mac_platform_backend_unittest.py
(right):

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/mac_platform_backend_unittest.py:30:
self.assertGreater(mac_platform_backend.MAVERICKS,
Can we use these now? I thought the bots were still at python 2.6 ?
(http://docs.python.org/2.6/library/unittest.html)

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/posix_platform_backend.py (right):

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:71:
application = util.FindExecutableInPath(application)
Why are we moving away from find_executable?

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:77: return
os.stat(path).st_mode & stat.S_ISUID == stat.S_ISUID
nit surround everything before == with parenthesis - a bit more readable, IMHO.

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:94: if
parameters:
I think the isinstance check should be at the top of the function?

[tonyg, 2014-01-29 01:16:05]

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/mac_platform_backend.py (right):

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/mac_platform_backend.py:22: LEOPARD =   
  platform_backend.OSVersion('leopard',       9)
On 2014/01/29 00:27:23, jeremy wrote:
> nit I'd add a # 10.6 ... # 10.9 at the end of each of these lines for
> readability...

Done.

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/mac_platform_backend.py:174:
is_mavericks = self.GetOSVersionName() >= MAVERICKS
On 2014/01/29 00:27:23, jeremy wrote:
> s/is_mavericks/mavericks_or_later/ ?
> Also below.

Done.

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/mac_platform_backend_unittest.py
(right):

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/mac_platform_backend_unittest.py:30:
self.assertGreater(mac_platform_backend.MAVERICKS,
On 2014/01/29 00:27:23, jeremy wrote:
> Can we use these now? I thought the bots were still at python 2.6 ?
> (http://docs.python.org/2.6/library/unittest.html)

Yep. Just last week!

https://codereview.chromium.org/143823017/

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/posix_platform_backend.py (right):

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:71:
application = util.FindExecutableInPath(application)
On 2014/01/29 00:27:23, jeremy wrote:
> Why are we moving away from find_executable?

Oops, I didn't realize that. Fixed.

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:77: return
os.stat(path).st_mode & stat.S_ISUID == stat.S_ISUID
On 2014/01/29 00:27:23, jeremy wrote:
> nit surround everything before == with parenthesis - a bit more readable,
IMHO.

Done.

https://codereview.chromium.org/137143011/diff/70001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/posix_platform_backend.py:94: if
parameters:
On 2014/01/29 00:27:23, jeremy wrote:
> I think the isinstance check should be at the top of the function?

Done.

[jeremy, 2014-01-29 01:23:19]

LGTM!!!

[commit-bot: I haz the power, 2014-01-29 01:33:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/137143011/100001

[commit-bot: I haz the power, 2014-01-29 02:18:01]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-29 03:43:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/137143011/100001

[commit-bot: I haz the power, 2014-01-29 13:34:30]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-29 14:03:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/137143011/100001

[commit-bot: I haz the power, 2014-01-29 15:08:09]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-29 15:47:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/137143011/100001

[commit-bot: I haz the power, 2014-01-29 16:12:10]

Change committed as 247692