678385 - device managers probably need to be updated for package name + username

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Doug Turner (:dougt)]

see build/mobile/devicemanagerADB.py and similar.


blassey added support for package names to contain the developers username.  so, unofficial builds on my machine are:

org.mozilla.fennec_dougt


if I am running remote tests, we might need to update the devices managers to handle this cause.

i am unsure if it is a problem or not.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 2

[Geoff Brown [:gbrown]]

It is a problem for me! The remote xpcshell work (bug 668349) uses devicemanagerADB.getAppRoot, which fails if org.mozilla.fennec_unofficial|aurora|beta is not installed. The patch solves this problem.

Comment 3

[Geoff Brown [:gbrown]]

Pushed to try, along with my remote xpcshell patches, but many "Android opt" tests failed:

http://tbpl.mozilla.org/?tree=Try&rev=55446b4a1b98

We need to guard against os.getenv returning None (as when USER is not set):

File "/builds/tegra-049/test/build/tests/mochitest/devicemanagerADB.py", line 15, in __init__     packageName = 'org.mozilla.fennec_' + os.getenv('USER'); TypeError: cannot concatenate 'str' and 'NoneType' objects

Comment 4

[Doug Turner (:dougt)]

what about the non-adb version?

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Doug Turner (:dougt) from comment #4)
> what about the non-adb version?

it doesn't use the package name to my knowledge

Comment 6

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #3)
> Pushed to try, along with my remote xpcshell patches, but many "Android opt"
> tests failed:

To clarify my previous comment: When I pushed this patch to try, many tests failed because the USER environment variable was not set, causing os.getenv to return None, which could not be concatenated with a string.

Comment 7

[Geoff Brown [:gbrown]]

Attachment: slight change to blassey's patch that allows for os.getenv to return None

I have been using this version successfully in my xpcshell testing.

Comment 8

[Geoff Brown [:gbrown]]

My patch is okay on try:

http://tbpl.mozilla.org/?tree=Try&rev=ff5c3f0b0f6a

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 553873 [details] [diff] [review]
slight change to blassey's patch that allows for os.getenv to return None

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

::: build/mobile/devicemanagerADB.py
@@ +11,5 @@
>      self.retrylimit = retrylimit
>      self.retries = 0
>      self._sock = None
> +    if packageName == None and os.getenv('USER'):
> +      packageName = 'org.mozilla.fennec_' + os.getenv('USER');

actually, I take that back. If USER isn't set, then packageName will be set to None, right? That can't be what we want. this should be
if packageName == None:
   if os.getenv('USER'):
      packageName = 'org.mozilla.fennec_' + os.getenv('USER');
   else:
      'org.mozilla.fennec_';

Comment 10

[Geoff Brown [:gbrown]]

My thinking was that if USER is not set, it's best to proceed as though we do not know the package name. Setting packageName to None avoids use of the run-as trick (which is okay) and may also affect getAppRoot (depending on where Fennec is installed on the device). Your way may be less likely to cause problems for getAppRoot...I'll try it out.

Comment 11

[Geoff Brown [:gbrown]]

Attachment: slight change to blassey's patch that allows for os.getenv to return None

updated as suggested

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 554301 [details] [diff] [review]
slight change to blassey's patch that allows for os.getenv to return None

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

looks good, get review and land it

::: build/mobile/devicemanagerADB.py
@@ +391,2 @@
>  
>      # Failure (either not installed or not a recognized platform)

please add some logging to indicate we hit this case

Comment 13

[Geoff Brown [:gbrown]]

Attachment: patch for review, based on blassey's original work

Identical to patch with feedback+, with requested warning added.

Comment 14

[Ed Morley [:emorley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5378c0b9525a

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/5378c0b9525a