1151954 - Migrate releases-related/get_apk.sh to mozharness

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

This script allows release managers to download apk from our ftp:
http://hg.mozilla.org/build/braindump/file/tip/releases-related/get_apk.sh

We should convert it to a Python mozharness script.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

example of a mozharness script:
https://hg.mozilla.org/build/mozharness/file/tip/scripts/push_apk.py

Comment 2

[Tyler Ebel]

I would like to take a look at. Please assign to me.

Comment 3

[MikeLing]

I would start to work on this bug, and I will commit first patch as soon as possible, Please assign it to me.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Well, you both asked for this bug. I will assign it to the one you write the first patch...

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Oh, before I forget, managing the armv6 files is no longer necessary. This can be dropped. The code is going to be simplified.
For example:
http://hg.mozilla.org/build/braindump/file/tip/releases-related/get_apk.sh#l56

Comment 6

[MikeLing]

Sorry for the late reply. 
I haven't know much a bout shell, I can see three function in this shell script but I can't find where they be called. Could you offer me some document about that or tell me how these function works? Thank you very much!

Comment 7

[MikeLing]

(In reply to MikeLing from comment #6)
> Sorry for the late reply. 
> I haven't know much a bout shell, I can see three function in this shell
> script but I can't find where they be called. Could you offer me some
> document about that or tell me how these function works? Thank you very much!

Hm, I think I figure out how bash function goes. I will commit my first version patch ASAP :)

Comment 8

[MikeLing]

Hello~ 

Could you tell me more about this line(http://hg.mozilla.org/build/braindump/file/tip/releases-related/get_apk.sh#l48)? I don't know how to apply the 'MAJOR' into python.

And I would like to use argparse to fix this issues like:

>def parse_args()
>    parser = argparse.ArgumentParser()
>    parser.add_argument('version number', dest='version_number',help='version number to download')
>    parser.add_argument('-b', '--build',default=1, help='specify build number and default is 1',
>   					 dest='specify_number')
>    return parser.parse_args()
>.......
>def main():
>    action = parse_args()
>    if action.version_number not None:
>               download_function_called()
>    else:
>               exit()

It's a lot different with the example you give. So,please tell me if I couldn't code like that. Thank you :)

Comment 9

[Sylvestre Ledru [:Sylvestre]]

> I don't know how to apply the 'MAJOR' into python.
The MAJOR variable is used to detect the major version of Firefox to manage the different layout.
As mentioned in comment #5, this can be dropped (and I think MAJOR might be useless in this context).

> please tell me if I couldn't code like that. 
The argument management is done by Mozharness, you should use this API and not argparse.

Comment 10

[Keyan Pishdadian]

Hi, aspiring new contributor here. I'm going to try tackling this.

Comment 11

[Varun Joshi (:vjoshi)]

Hi, I'm a new contributor. I'd like to try handling this bug.

Comment 12

[Alex Johnson (:alex_johnson)]

Attachment: bug-1151954.patch

Comment 13

[Alex Johnson (:alex_johnson)]

Attachment: bug-1151954.patch

Fixed some nits.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8703368 [details] [diff] [review]
bug-1151954.patch

Alex, great to see progress on this and it looks a first version!

Some comments:
* when getting a 404, we should not retry, just fail with an error
* when no version is provided, we should download the two arm and the i386
* I think we can download the apk in the current directory (not build/)
* having a test would be great
* flake8 founds two warnings:
 - get_apk.py:86:39: E126 continuation line over-indented for hanging indent
 - get_apk.py:161:28: W292 no newline at end of file

Comment 15

[Alex Johnson (:alex_johnson)]

Sylvestre, I'm having a hard time finding where the test for push_apk is located.  I want to use that as a reference to make a test for get_apk.  Could you point me in the right direction?

Comment 16

[Sylvestre Ledru [:Sylvestre]]

For now, this is not great:
https://hg.mozilla.org/build/mozharness/file/tip/scripts/push_apk.py#l193

I think we need to experiment something better in the future!

Comment 17

[Alex Johnson (:alex_johnson)]

Attachment: bug-1151954.patch

Comment 18

[Alex Johnson (:alex_johnson)]

Attachment: bug-1151954.patch

The change to the .hgignore would've caused a merge conflict, got that fixed.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

About the test, we don't want to test the network (moreover, we don't want to update the code each time the mozilla favicon changes). We want to test the code itself (like with unit test)

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8703749 [details] [diff] [review]
bug-1151954.patch



>+        apk_checksum = self.file_sha512sum(apk_file)
Could you display a message that we are checking the checksum?
>+    def download(self, filename, url):
>+        ScriptMixin.mkdir_p(self, self.download_dir)
>+
>+        apk_url = url + ".apk"
You can probably declare .apk as a member of the class and reuse it everywhere

>+    def download_all(self, version, build, locale):
>+        filename_x86 = "fennec-" + version + "." + locale + "." + "android-" + self.get_arch_file("x86")
>+        url = self.generate_url(version, "x86", build, locale, self.get_android_layout("x86"), self.get_arch_file("x86"))
>+        self.download(filename_x86, url)
What about adding a variable
commonFileName = "fennec-" + version + "." + locale + "." + "android-"
and reuse it?
(or move that into a method)


>+        filename_arm_v9 = "fennec-" + version + "." + locale + ".android-arm-api-9"
>+        url = self.generate_url(version, "arm", build, locale, "android-api-9", self.get_arch_file("arm"))
>+        self.download(filename_arm_v9, url)
>+
>+        filename_arm_v11 = "fennec-" + version + "." + locale + ".android-arm-api-11"
>+        url = self.generate_url(version, "arm", build, locale, "android-api-11", self.get_arch_file("arm"))
>+        self.download(filename_arm_v11, url)
>+
>+    def get_android_layout(self, arch):
>+        if arch == "arm":
>+            return "v9_v11"
>+        else:
>+            return "android-"+arch
>+
>+    def get_arch_file(self, arch):
>+        if arch == "x86":
>+            # the filename contains i386 instead of x86
>+            return "i386"
>+        else:
>+            return arch
>+
>+    def download_apk(self):
>+        self.check_argument()
>+        version = self.config["version"]
>+        arch = self.config["arch"]
>+        build = str(self.config["build"])
>+        locale = self.config["locale"]
>+
>+        android_layout = self.get_android_layout(arch)
>+
>+        arch_file = self.get_arch_file(arch)
>+
>+        self.info("Downloading version " + version + " build #" + build
>+                  + " for arch " + arch + " (locale " + locale + ")")
>+        if arch == "all":
>+            self.download_all(version, build, locale)
>+        elif android_layout == "v9_v11":
>+            # When dealing with API v9 & V11, we want to rename the file
>+            # until bug 1122059 is fixed
>+            # Also manage the KO locale
>+            filename_arm_v9 = "fennec-" + version + "." + locale + ".android-arm-api-9"
This looks like a duplicate declaration, isn't it?

Besides that, it looks good.
When fixed, do you need me to push the change for you?

Comment 21

[Alex Johnson (:alex_johnson)]

I'm trying to come up with a better test to write, but I can't think of a way to do that without downloading an apk file, which are pretty large. It would make the test long. Could we make a test-candidates directory path on the ftp server with a test.apk file and an associated checksums file?

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Sorry, I wasn't clear enough here.
You don't need to download actual file.
You could test the various method like get_arch_file, get_android_layout, this kind of method unit tests.

Comment 23

[Alex Johnson (:alex_johnson)]

Attachment: bug-1151954.patch

I ended up changing a lot of the code in the process of subdiving the download() function into different functions for easy testing.  Could you review this again?  Sorry!  

And could you push it if it looks good? I only have LDAP level 1 access.

Comment 24

[Jordan Lund (:jlund)]

fyi - api-11 has changed to api-15 on trunk and will ride the trains

see: https://bugzilla.mozilla.org/show_bug.cgi?id=1219094

Comment 25

[Alex Johnson (:alex_johnson)]

I have added a config to the top of the script to allow changing the multi api names pretty easily.  Sylvestre, what do you think?

Comment 26

[Alex Johnson (:alex_johnson)]

Attachment: bug-1151954.patch

ScriptMixin doesn't need to be an argument for GetAPK.  Fixed that.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8708741 [details] [diff] [review]
bug-1151954.patch

This is great, many thanks. Need me to commit it for you?

Comment 28

[Alex Johnson (:alex_johnson)]

Yes please!

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Many thanks again, here it is:
https://hg.mozilla.org/build/mozharness/rev/15845edae9a1

Comment 30

[Release Engineering SlaveAPI Service]

mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/cb31491daf72

Comment 31

[Hal Wine [:hwine] use NI!]

I believe this was committed to the wrong repo -- all test scripts are in-tree now.

Please re-land where it will be of some use (I don't have the chops) ;)

Comment 32

[Hal Wine [:hwine] use NI!]

https://hg.mozilla.org/build/mozharness/rev/cb6179bb9b691f0ab6d0f665ba9236e5ac27cdec
bug 1151954 backout - needs to be in-tree; r=bustage

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18e0661777d9

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Oh, crap... I didn't notice that the hgignore is incorrect and Alex is not marked as author ... :(
sorry Alex...

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fda6cdfcb04

Comment 36

[Alex Johnson (:alex_johnson)]

Sylvestre, would you like me to move the changes to mozinbound and submit another patch?

Comment 37

[Sylvestre Ledru [:Sylvestre]]

I took care of that, don't worry :)

Comment 38

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/18e0661777d9
https://hg.mozilla.org/mozilla-central/rev/8fda6cdfcb04