Fix adb_chrome_public_command_line for Android M & refactor into a helper .sh

Fix adb_chrome_public_command_line for Android M & refactor into a helper .sh

BUG=493878
Committed: https://crrev.com/c481d6771df7cfd731ab174b561388fca8596ce1
Cr-Commit-Position: refs/heads/master@{#332263}

[agrieve, 2015-06-01 15:26:21]

agrieve@chromium.org changed reviewers:
+ jbudorick@chromium.org

[jbudorick, 2015-06-01 15:43:24]

https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
File build/android/adb_command_line_functions.sh (right):

https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
build/android/adb_command_line_functions.sh:12: if [[ "$REQUIRES_SU" = 1 ]];
then
-eq

?

https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
build/android/adb_command_line_functions.sh:14: IS_OLD_STYLE=$(adb shell su -c
echo 1 2>/dev/null)
I'm not crazy about this.

https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
build/android/adb_command_line_functions.sh:17: SU_CMD="su 0"
Does this style work in older android versions?

[agrieve, 2015-06-01 15:58:19]

https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
File build/android/adb_command_line_functions.sh (right):

https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
build/android/adb_command_line_functions.sh:12: if [[ "$REQUIRES_SU" = 1 ]];
then
On 2015/06/01 15:43:24, jbudorick wrote:
> -eq
> 
> ?

Since REQUIRES_SU can be unset, I think = is more appropriate.

https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
build/android/adb_command_line_functions.sh:14: IS_OLD_STYLE=$(adb shell su -c
echo 1 2>/dev/null)
On 2015/06/01 15:43:24, jbudorick wrote:
> I'm not crazy about this.

suggestion for what you would be crazy for?

https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
build/android/adb_command_line_functions.sh:17: SU_CMD="su 0"
On 2015/06/01 15:43:24, jbudorick wrote:
> Does this style work in older android versions?

From my quick test, seems to work back got JB, but not on ICS.

[jbudorick, 2015-06-01 17:44:21]

https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
File build/android/adb_command_line_functions.sh (right):

https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
build/android/adb_command_line_functions.sh:14: IS_OLD_STYLE=$(adb shell su -c
echo 1 2>/dev/null)
On 2015/06/01 at 15:58:19, agrieve wrote:
> On 2015/06/01 15:43:24, jbudorick wrote:
> > I'm not crazy about this.
> 
> suggestion for what you would be crazy for?

I would break on SDK level (ro.build.version.sdk) -- maybe do su 0 for L+ and su
-c otherwise. (That's what we'll probably wind up doing on the python side in
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...)

[agrieve, 2015-06-01 19:22:36]

On 2015/06/01 17:44:21, jbudorick wrote:
>
https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
> File build/android/adb_command_line_functions.sh (right):
> 
>
https://codereview.chromium.org/1165753002/diff/20001/build/android/adb_comma...
> build/android/adb_command_line_functions.sh:14: IS_OLD_STYLE=$(adb shell su -c
> echo 1 2>/dev/null)
> On 2015/06/01 at 15:58:19, agrieve wrote:
> > On 2015/06/01 15:43:24, jbudorick wrote:
> > > I'm not crazy about this.
> > 
> > suggestion for what you would be crazy for?
> 
> I would break on SDK level (ro.build.version.sdk) -- maybe do su 0 for L+ and
su
> -c otherwise. (That's what we'll probably wind up doing on the python side in
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...)

Done

[jbudorick, 2015-06-01 19:23:54]

lgtm

[agrieve, 2015-06-01 20:20:22]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-06-01 20:23:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165753002/40001

[commit-bot: I haz the power, 2015-06-01 20:52:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-01 20:53:47]

Patchset 3 (id:??) landed as
https://crrev.com/c481d6771df7cfd731ab174b561388fca8596ce1
Cr-Commit-Position: refs/heads/master@{#332263}

[Dmitry Skiba, 2015-06-02 20:24:25]

dskiba@google.com changed reviewers:
+ dskiba@google.com

[Dmitry Skiba, 2015-06-02 20:24:25]

https://codereview.chromium.org/1165753002/diff/40001/build/android/adb_comma...
File build/android/adb_command_line_functions.sh (right):

https://codereview.chromium.org/1165753002/diff/40001/build/android/adb_comma...
build/android/adb_command_line_functions.sh:27: adb shell "cat $CMD_LINE_FILE
2>/dev/null | cut -d ' ' -s -f2-"
Previously (in adb_chrome_public_command_line) this line was:

adb shell cat $CMD_LINE_FILE | cut -d " " -f "2-" 2>/dev/null

i.e. cut was executed on the host, now it's executed on the target and fails.

[agrieve, 2015-06-03 00:19:24]

On 2015/06/02 20:24:25, dskiba wrote:
>
https://codereview.chromium.org/1165753002/diff/40001/build/android/adb_comma...
> File build/android/adb_command_line_functions.sh (right):
> 
>
https://codereview.chromium.org/1165753002/diff/40001/build/android/adb_comma...
> build/android/adb_command_line_functions.sh:27: adb shell "cat $CMD_LINE_FILE
> 2>/dev/null | cut -d ' ' -s -f2-"
> Previously (in adb_chrome_public_command_line) this line was:
> 
> adb shell cat $CMD_LINE_FILE | cut -d " " -f "2-" 2>/dev/null
> 
> i.e. cut was executed on the host, now it's executed on the target and fails.

Arg, thanks for catching this. I'll make a fix.

[agrieve, 2015-06-03 00:20:00]

On 2015/06/03 00:19:24, agrieve wrote:
> On 2015/06/02 20:24:25, dskiba wrote:
> >
>
https://codereview.chromium.org/1165753002/diff/40001/build/android/adb_comma...
> > File build/android/adb_command_line_functions.sh (right):
> > 
> >
>
https://codereview.chromium.org/1165753002/diff/40001/build/android/adb_comma...
> > build/android/adb_command_line_functions.sh:27: adb shell "cat
$CMD_LINE_FILE
> > 2>/dev/null | cut -d ' ' -s -f2-"
> > Previously (in adb_chrome_public_command_line) this line was:
> > 
> > adb shell cat $CMD_LINE_FILE | cut -d " " -f "2-" 2>/dev/null
> > 
> > i.e. cut was executed on the host, now it's executed on the target and
fails.
> 
> Arg, thanks for catching this. I'll make a fix.

https://codereview.chromium.org/1153343007

[Nico, 2015-07-28 20:42:30]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-07-28 20:42:31]

I happened to see this today. I think it's a bad idea to create shell helper
libraries. We shouldn't write any interesting scripts as bash scripts, as we
keep learning over and over (see e.g. crbug.com/330631 for a previous cleanup
effortr). All these scripts assume that adb is in the path instead of using
constants.GetAdbRoot() (
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
), not everyone speaks shell, not everyone uses bash, etc etc etc.