1482884 - During adb push check if remote directory exists before removing basename for adb 1.0.36 and later

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement)

Description

[Bob Clary [:bc] (inactive)]

Bug 1285040 changed how adb push/pull works due to changes in adb for 1.0.36 and later. We actually still have a bug in adb push if the target directory already exists. You can see this locally by checking the test root while a unit tests is running locally and seeing that the initial pushing of the profile places it in /sdcard/tests rather than /sdcard/tests/profile which *may* be the source of some of our issues.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: adb-push.patch

Try run coming up.

Comment 2

[Bob Clary [:bc] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&filter-tier=1&filter-tier=2&filter-tier=3&group_state=expanded

Comment 3

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #0)
> You can see this locally by checking the test root while a
> unit tests is running locally and seeing that the initial pushing of the
> profile places it in /sdcard/tests rather than /sdcard/tests/profile which
> *may* be the source of some of our issues.

I don't see this. Which unit tests?

I am running adb 1.0.40 (has something changed *again*??)

adb shell rm -r /sdcard/tests
./mach mochitest <some-dir>
  - debugging shows request to push /tmp/tmpp8be1U/profile to /sdcard/tests/profile, which hits the existing check for adb > 1.0.36 and translates to push /tmp/tmpp8be1U/profile to /sdcard/tests -- creating /sdcard/tests/profile.

Comment 4

[Bob Clary [:bc] (inactive)]

I'm on 1.0.40 as well as is bitbar. I believe the tooltool manifest also downloads 1.0.40. They did change some things which at first I suspected were the problem but I'm not entirely sure from reading the patches. At first I used the android version to change the behavior but then I thought it is just a bug in our push since we can see the same error in production but if we are really running 1.0.40 in production then perhaps it is a change in adb. It may also be the interaction between the version of adb on the host and the version of adbd on the device which we do not take into consideration but I don't think so since the 4.3 emulators must be running an old version of adbd. The changes I found in https://android.googlesource.com/platform/system/core/+log/master/adb related to Android 8.1.

Take for example android reftest https://taskcluster-artifacts.net/SuPul9lmQr-BmLxz-wS8Ww/0/public/logs/live_backing.log

The symptom is:

[task 2018-08-14T12:02:07.564Z] 12:02:07     INFO -  chmod -R 777 /sdcard/tests/reftest/profile: Ignoring Error: args: adb -s emulator-5554 wait-for-device shell chmod -R 777 /sdcard/tests/reftest/profile; echo rc=$?, exitcode: 10, stdout: Unable to chmod /sdcard/tests/reftest/profile: No such file or directory

This occurs after the profile was supposedly pushed to the device.

https://searchfox.org/mozilla-central/source/layout/tools/reftest/remotereftest.py#293

I don't see this error in my try run though. See https://taskcluster-artifacts.net/J8tFbYIERv6f0giJFsdB2w/0/public/logs/live_backing.log

If you look while a test is running you can see that instead of pushing the profile to /sdcard/tests/reftest/profile it was pushed to /sdcard/tests/reftest. Note the chrome, extentions, prefs.js, user.js in the /sdcard/tests/reftest directory without this patch on my p2.

walleye:/sdcard/tests/reftest $ ls
chrome extensions fonts hyphenation permissions.sqlite prefs.js profile user.js 

I see the same on my arm 7.0 emulator

generic_arm64:/sdcard/tests/reftest # ls
chrome extensions fonts hyphenation permissions.sqlite prefs.js profile user.js 

Only after fennec is launched is /sdcard/tests/reftest/profile created and populated.

Perhaps this is a difference with the adbd on the x86 emulator? I don't have a build handy to test with but can build one to check.

Comment 5

[Geoff Brown [:gbrown]]

I was running 'mach mochitest', which pushes the profile to /sdcard/tests -- that seems to consistently create /sdcard/tests/profile correctly.

When I ran 'mach reftest' with none of your recent patches, I saw the 'No such file or directory', but the reftests ran correctly, suggesting to me that the profile was okay. Checking more, I saw two profile pushes, both to /sdcard/tests/reftest/profile: The first pushed profile files to /sdcard/tests/reftest; the second pushed profile files to /sdcard/tests/reftest/profile and set everything up correctly. The double push was caused by copyExtraFilesToProfile of course. If I remove copyExtraFilesToProfile, I get your results: We are on the same page!

Comment 6

[Geoff Brown [:gbrown]]

Comment on attachment 8999613 [details] [diff] [review]
adb-push.patch

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

This looks right. Hopefully this is the last time we need to revisit this issue!

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +175,5 @@
>                                        stdout=subprocess.PIPE,
>                                        stderr=subprocess.PIPE).communicate()
>              re_version = re.compile(r'Android Debug Bridge version (.*)')
>              self._adb_version = re_version.match(output[0]).group(1)
> +            self._logger.info('Using adb %s' % self._adb_version)

I like reporting the adb version, but I see multiple such reports, even compared to other adb.py messages:

 0:03.11 Using adb 1.0.40
 0:03.11 Using adb 1.0.40
 0:03.32 adbd running as root
 0:03.52 su 0 supported

Could the logging be moved to ADBDevice init?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e13bb9fa6df5
During adb push check if remote directory exists before removing basename for adb 1.0.36 and later, r=gbrown.

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e13bb9fa6df5