Closed Bug 1016467 Opened 6 years ago Closed 5 years ago

mozversion should allow getting information out of android .apk

Categories

(Testing :: Mozbase, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla32

People

(Reporter: wlach, Assigned: wlach)

References

Details

Attachments

(2 files, 2 obsolete files)

For eideticker and other Android testing scenarios, it would be useful to be able to get version information out of the .apk.
:bc might find this interesting.
In the end I decided to overload the meaning of "binary", as it seemed slightly more elegant than a separate "apk" keyword arg.
Assignee: nobody → wlachance
Attachment #8429400 - Flags: review?(dave.hunt)
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.
Attachment #8429400 - Flags: review?(dave.hunt) → review+
Attached patch Updated patch (obsolete) — Splinter Review
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.
Attachment #8429400 - Attachment is obsolete: true
Attachment #8430354 - Flags: review?(dave.hunt)
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.
Attachment #8430354 - Flags: review?(dave.hunt) → review+
(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.
Updated patch per Dave's request, carrying forward r+
Attachment #8430354 - Attachment is obsolete: true
Attachment #8431015 - Flags: review+
Setting checkin-needed. No try run because no in-tree automation actually uses mozversion. I ran the mozbase unit tests and they work fine.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e7f524cc6188
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Forgot to add the unit test to the repo. I just retested it and it still works fine, so let's land it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file Unit test
Just the missing file, no diff. If it looks reasonable I'll push it (already tested it locally)
Attachment #8474774 - Flags: review?(ahalberstadt)
Comment on attachment 8474774 [details]
Unit test

Thanks!
Attachment #8474774 - Flags: review?(ahalberstadt) → review+
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
Flags: needinfo?(wlachance)
(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.
(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
Flags: needinfo?(wlachance)
(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
https://hg.mozilla.org/mozilla-central/rev/98862f599253
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.