1016467 - mozversion should allow getting information out of android .apk

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

For eideticker and other Android testing scenarios, it would be useful to be able to get version information out of the .apk.

Comment 1

[William Lachance (:wlach)]

:bc might find this interesting.

Comment 2

[William Lachance (:wlach)]

Attachment: Support getting information out of .apk

In the end I decided to overload the meaning of "binary", as it seemed slightly more elegant than a separate "apk" keyword arg.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8429400 [details] [diff] [review]
Support getting information out of .apk

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

r=me with a few nits addressed.

::: testing/mozbase/docs/mozversion.rst
@@ +29,5 @@
>  ---binary
>  '''''''''
>  
> +This is the path to the target application binary or .apk. If this is
> +then the current directory is checked for the existance of an

You lost the word 'omitted' somehow. :)

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +47,5 @@
> +                else:
> +                    self.warn('Unable to find %s' % filename)
> +        else:
> +            for filename, section in (('application.ini', 'App'),
> +                                      ('platform.ini', 'Build')):

Nit: Could we avoid repeating the filenames and sections by defining these before our if statement?

@@ +215,4 @@
>                      self._info[desired_props[key]] = value
>  
>  
> +def get_version(binary=None, apk=None, sources=None, dm_type=None, host=None):

I think you want to remove apk from the keyword arguments if you're using binary.

Comment 4

[William Lachance (:wlach)]

Attachment: Updated patch

So I just realized that my previous patch had some bugs that caused stuff not to work and broke tests. :) In the midst of fixing that, I also refactored some things to hopefully make more sense (e.g. creating a special "FennecVersion" class). I also added a test to make sure the apk stuff works.

Since the end result is a substantially changed patch, thought it would be best if I ran it by you one more time for review.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8430354 [details] [diff] [review]
Updated patch

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

Nice improvement and thanks for the test. Just one question but the answer doesn't affect my review.

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +234,5 @@
>      """
>      try:
> +        if binary and zipfile.is_zipfile(binary) and 'AndroidManifest.xml' in \
> +           zipfile.ZipFile(binary, 'r').namelist():
> +            version = FennecVersion(binary)

Should this be LocalFennecVersion? Is there a way to retrieve the APK from a device? If so, this would leave us open to a future enhancement for RemoteFennecVersion too.

Comment 6

[William Lachance (:wlach)]

(In reply to Dave Hunt (:davehunt) from comment #5)
> Comment on attachment 8430354 [details] [diff] [review]
> Updated patch
> 
> Review of attachment 8430354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice improvement and thanks for the test. Just one question but the answer
> doesn't affect my review.
> 
> ::: testing/mozbase/mozversion/mozversion/mozversion.py
> @@ +234,5 @@
> >      """
> >      try:
> > +        if binary and zipfile.is_zipfile(binary) and 'AndroidManifest.xml' in \
> > +           zipfile.ZipFile(binary, 'r').namelist():
> > +            version = FennecVersion(binary)
> 
> Should this be LocalFennecVersion? Is there a way to retrieve the APK from a
> device? If so, this would leave us open to a future enhancement for
> RemoteFennecVersion too.

It would require a rooted device to do this, and I'm not sure if it would really be that useful even so (since typically in a test harness like eideticker, you install an apk before running the test). But it does no harm to leave this possibility open, so I'll change the name.

Comment 7

[William Lachance (:wlach)]

Attachment: FennecVersion -> LocalFennecVersion

Updated patch per Dave's request, carrying forward r+

Comment 8

[William Lachance (:wlach)]

Setting checkin-needed. No try run because no in-tree automation actually uses mozversion. I ran the mozbase unit tests and they work fine.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f524cc6188

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[William Lachance (:wlach)]

Forgot to add the unit test to the repo. I just retested it and it still works fine, so let's land it.

Comment 12

[William Lachance (:wlach)]

Attachment: Unit test

Just the missing file, no diff. If it looks reasonable I'll push it (already tested it locally)

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 8474774 [details]
Unit test

Thanks!

Comment 14

[William Lachance (:wlach)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c69cc7b3c21d

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/db50d91e5895 for apparently breaking B2G Windows builds. (Or at least, they were passing prior to this commit, and haven't passed since...)

https://tbpl.mozilla.org/php/getParsedLog.php?id=46208118&tree=Mozilla-Inbound

Comment 16

[William Lachance (:wlach)]

(In reply to Wes Kocher (:KWierso) from comment #15)
> I had to back this out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/db50d91e5895 for
> apparently breaking B2G Windows builds. (Or at least, they were passing
> prior to this commit, and haven't passed since...)
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=46208118&tree=Mozilla-
> Inbound

Yes, this patch does appear to be at fault. I'll look into it, fixing it shouldn't be hard.

Comment 17

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #16)
> (In reply to Wes Kocher (:KWierso) from comment #15)
> > I had to back this out in
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/db50d91e5895 for
> > apparently breaking B2G Windows builds. (Or at least, they were passing
> > prior to this commit, and haven't passed since...)
> > 
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=46208118&tree=Mozilla-
> > Inbound
> 
> Yes, this patch does appear to be at fault. I'll look into it, fixing it
> shouldn't be hard.


Looks like the problem is reopening a temporary file on windows. We have a method in mozfile to deal with this situation, let's see if it helps:

https://tbpl.mozilla.org/?tree=Try&rev=e761e00ec072

Comment 18

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #17)
> (In reply to William Lachance (:wlach) from comment #16)
> > (In reply to Wes Kocher (:KWierso) from comment #15)
> > > I had to back this out in
> > > https://hg.mozilla.org/integration/mozilla-inbound/rev/db50d91e5895 for
> > > apparently breaking B2G Windows builds. (Or at least, they were passing
> > > prior to this commit, and haven't passed since...)
> > > 
> > > https://tbpl.mozilla.org/php/getParsedLog.php?id=46208118&tree=Mozilla-
> > > Inbound
> > 
> > Yes, this patch does appear to be at fault. I'll look into it, fixing it
> > shouldn't be hard.
> 
> 
> Looks like the problem is reopening a temporary file on windows. We have a
> method in mozfile to deal with this situation, let's see if it helps:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=e761e00ec072

That went well, pushing an updated version to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/98862f599253

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/98862f599253