1249120 - Autophone - use unrolled recursive chmod when -R not supported

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

As a follow up to bug 1243408, this attempts to improve the recursive chmod performance for devices which do not support -R by getting a recursive list of the path and building a script which will chmod each listed file/directory directly.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: bug-1249120-add-ls.patch

Used by second patch but generally useful as well.

Comment 2

[Bob Clary [:bc] (inactive)]

Comment on attachment 8720465 [details] [diff] [review]
bug-1249120-add-ls.patch

Found a slight issue with Nexus 4. I'll try again.

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: ls.sh

simple shell script which tests various incarnations of shell ls on devices.

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: ls-data-emu.txt

ls.sh on various emulators

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: ls-data-local.txt

ls.sh on nexus one (cyanogenmod), samsgung gs3, nexus 6p.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: ls-data-remote.txt

ls.sh on production devices. note the funky behavior of the nexus 4 devices. :-/

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: bug-1249120-add-ls-v2.patch

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: ls.py

simple python script to perform testing of ls.

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: test-nexus-4.txt

output for nexus-4-1 on autophone-1. I copied ls.py, adb*.py, version_codes.py to /tmp/ on autophone-1 and tested it there.

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: bug-1249120-unroll-chmod.patch

Comment 11

[Geoff Brown [:gbrown]]

Comment on attachment 8720815 [details] [diff] [review]
bug-1249120-add-ls-v2.patch

Review of attachment 8720815 [details] [diff] [review]:
-----------------------------------------------------------------

::: adb.py
@@ +1570,5 @@
> +        also the case of other sdcard related paths such as
> +        /storage/emulated/legacy but no adjustment is made in those
> +        cases.
> +
> +        The ls method works around a Nexus 4 bug which prevents

It seems really odd that it would be specific to the Nexus 4...but you have obviously done the research!

@@ +1572,5 @@
> +        cases.
> +
> +        The ls method works around a Nexus 4 bug which prevents
> +        recursive listing of directories on the sdcard unless the path
> +        ends with "/*" by adjusting sdcard paths ending in "/" to end

...and it seems odd that sdcard is special. Could it be because it is a link? Or related to the filesystem type or mount options? Regardless, "sdcard" is obviously a simple check and probably covers all autophone's concerns; I'm just offering thoughts in case you want to generalize further.

@@ +1581,5 @@
> +
> +        :param str path: The directory name on the device.
> +        :param bool recursive: Flag specifying if a recursive listing
> +            is to be returned. If recursive is False, the returned
> +            matches will be relative to the path. If recursive if True,

If recursive *is* True,

Comment 12

[Geoff Brown [:gbrown]]

Comment on attachment 8720819 [details] [diff] [review]
bug-1249120-unroll-chmod.patch

Review of attachment 8720819 [details] [diff] [review]:
-----------------------------------------------------------------

Good stuff.

You have me wondering now if there might be cases where it is possible to further optimize by generating a script entirely on device, avoiding the round-trips for ls and pushing the script...for another day!

::: adb.py
@@ +1433,2 @@
>                  if e.message.find('No such file or directory') != -1:
>                      self._logger.warning('chmod -R %s %s: Ignoring Error: %s' %

"Ignoring error" no longer seems appropriate.

@@ +1450,2 @@
>                  else:
> +                    tmpf.write('chmod %s %s\n' % (mask, entry))

The else body looks identical to the if body. Just my eyes?

Comment 13

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #11)
> Comment on attachment 8720815 [details] [diff] [review]
> bug-1249120-add-ls-v2.patch
> 
> 
> ...and it seems odd that sdcard is special. Could it be because it is a
> link? Or related to the filesystem type or mount options? Regardless,
> "sdcard" is obviously a simple check and probably covers all autophone's
> concerns; I'm just offering thoughts in case you want to generalize further.

I thought it might be link specific, but I tested the actual path to the external storage device and saw the same kind of behavior. I didn't check a different link though. It is definitely weird and I almost missed it in the original patch. External storage is definitely different from internal and maybe this was just a bug in the Android version that shipped with our Nexus 4s.

I hope to change over to using /data/local/ completely soon. I need to figure out the unit test failures I saw when I last tried. Then we will leave the idiosyncrasies of ownership, permissions and this kind of thing behind us. 

> 
> @@ +1581,5 @@
> > +
> > +        :param str path: The directory name on the device.
> > +        :param bool recursive: Flag specifying if a recursive listing
> > +            is to be returned. If recursive is False, the returned
> > +            matches will be relative to the path. If recursive if True,
> 
> If recursive *is* True,

Thanks.

(In reply to Geoff Brown [:gbrown] from comment #12)
> Comment on attachment 8720819 [details] [diff] [review]
> bug-1249120-unroll-chmod.patch
> 
> Review of attachment 8720819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good stuff.
> 
> You have me wondering now if there might be cases where it is possible to
> further optimize by generating a script entirely on device, avoiding the
> round-trips for ls and pushing the script...for another day!
> 

I thought about a recursive shell function, but it seemed more prone to failure. It is definitely something we might try to investigate. Hopefully Android 2.3 will be gone soon... Firefox 48? then we can drop all this work around stuff altogether along with the Nexus Ss.

> ::: adb.py
> @@ +1433,2 @@
> >                  if e.message.find('No such file or directory') != -1:
> >                      self._logger.warning('chmod -R %s %s: Ignoring Error: %s' %
> 
> "Ignoring error" no longer seems appropriate.
> 
> @@ +1450,2 @@
> >                  else:
> > +                    tmpf.write('chmod %s %s\n' % (mask, entry))
> 
> The else body looks identical to the if body. Just my eyes?

Not sure. It's late and I'll go back and check tomorrow. Thanks for looking at this. I'll try to get this straightened out in the morning and get this landed. The Nexus S devices are definitely falling behind with their new manifests and I think this will help. /me crosses fingers.

Comment 14

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #12)
>
> ::: adb.py
> @@ +1433,2 @@
> >                  if e.message.find('No such file or directory') != -1:
> >                      self._logger.warning('chmod -R %s %s: Ignoring Error: %s' %
> 
> "Ignoring error" no longer seems appropriate.
> 

It is definitely not appropriate the way this was changed. I'm not sure that was intentional though. I think I botched an edit.

> @@ +1450,2 @@
> >                  else:
> > +                    tmpf.write('chmod %s %s\n' % (mask, entry))
> 
> The else body looks identical to the if body. Just my eyes?

No, your eyes are fine. Thanks for catching this. Originally, I hard coded the directory mask to a different value from the mask argument, e.g. 775 or maybe 777. The thinking was that I didn't really want to give a file execute permission if really all I wanted to execute on the directories so I could search them. I then removed it since this would have made the behavior different depending on whether recursive chmod was available or not. I'll just remove the if and do one write.

Comment 15

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/acdd2701ac40d52450698fd33d62fd4b910baf68
https://github.com/mozilla/autophone/commit/621209971dfff4810d7c8e97b16d7baa68b1a2c6

rebooted autophone-{1,2,3} 2016-02-19 13:34