Closed Bug 1217039 Opened 5 years ago Closed 4 years ago

Extract Android NDK binary URL logic into one place in |mach bootstrap|

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: nalexander, Unassigned)

References

Details

(Whiteboard: [lang=python][good first bug])

Attachments

(1 file, 2 obsolete files)

Right now the NDK binary URL is sprinkled through-out python/mozboot/mozboot/*.py -- search for "android-ndk".  A nice first bug is to extract that URL into the android.py module, including options for architecture (linux, macosx, windows in future) and automatically choosing 64-bit variants.

To fix this, add a function calculating the correct URL to https://dxr.mozilla.org/mozilla-central/source/python/mozboot/mozboot/android.py and use it in all the sub-modules.  This'll make one less place where the precise version is baked into the build system.
Hi,

I could chip onto this one a little I think! Would be my second bug. -)

I just wanted to confirm what exactly we're trying to do:

Right now I've managed to find all occurrences of android-ndk under python/mozboot/mozboot/*.py. They seem to be isolated into debian.py and osx.py.

I'm not exactly sure what we're trying to do after this. Are we changing the ndk_path to point to the android.py with the correct options, etc?

Thanks!
Mohamed
Flags: needinfo?(nalexander)
Hi!

(In reply to Mohamed Hammoud from comment #1)
> Hi,
> 
> I could chip onto this one a little I think! Would be my second bug. -)
> 
> I just wanted to confirm what exactly we're trying to do:
> 
> Right now I've managed to find all occurrences of android-ndk under
> python/mozboot/mozboot/*.py. They seem to be isolated into debian.py and
> osx.py.

Looking at https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=android-ndk%20path%3A*.py&redirect=true, I see matches in {archlinux,debian,osx}.py.  They all look similar -- android-ndk-VERSION-OS-x86{_64}.bin.

> I'm not exactly sure what we're trying to do after this. Are we changing the
> ndk_path to point to the android.py with the correct options, etc?

Extract a helper in android.py that calculates the download URL, using a single version in android.py, calculating the 64-bit in android.py, and taking as an input parameter the OS (darwin, linux).  Then use the helper in each of the .py files.
Flags: needinfo?(nalexander)
I would like to work on this bug. Could you please assign it to me?
(In reply to Harshit Bansal from comment #3)
> I would like to work on this bug. Could you please assign it to me?

Hi Harshit, I generally assign after a patch is up and has some feedback.  But you've claimed this by asking for it!

Can I suggest working on Bug 1221200 instead of this ticket?  That ticket is more valuable, since it'll help people get started building Fennec with a fast |mach artifact| based build, rather than a slow complete-with-C++ build.
(In reply to Nick Alexander :nalexander from comment #4)
> (In reply to Harshit Bansal from comment #3)
> > I would like to work on this bug. Could you please assign it to me?
> 
> Hi Harshit, I generally assign after a patch is up and has some feedback. 
> But you've claimed this by asking for it!
> 
> Can I suggest working on Bug 1221200 instead of this ticket?  That ticket is
> more valuable, since it'll help people get started building Fennec with a
> fast |mach artifact| based build, rather than a slow complete-with-C++ build.

Harshit, let's ramp up to Bug 1221200.  This ticket has decent instructions in https://bugzilla.mozilla.org/show_bug.cgi?id=1217039#c0, try following them?

The hardest part will be testing, since you'll really only be able to test on one machine.  You should be able to run |mach bootstrap| locally and observe your changes while developing locally.
Attached patch third.patch (obsolete) — Splinter Review
I was actually asking for instructions for BUG 1221200. For BUG
1217039 already have very good instructions and I have already
prepared a patch for it which I am now going to upload.
The only problem with BUG 1217039 is that how should I test it. For
testing it, I will have to run |mach bootstrap| with changed build
configuration for building Firefox for android which "I think(but not
sure)" may effect my current working environment.
However, I have double checked all the changes in patch.
Attachment #8684323 - Flags: review?(gps)
Attachment #8684323 - Flags: checkin?
Attachment #8684323 - Flags: review?(gps) → review?(mshal)
Comment on attachment 8684323 [details] [diff] [review]
third.patch

>         if is_64bits:
>-            self.ndk_url = 'https://dl.google.com/android/ndk/android-ndk-r10e-linux-x86_64.bin'
>+            #self.ndk_url = 'https://dl.google.com/android/ndk/android-ndk-r10e-linux-x86_64.bin'

You should just delete this line (and the others like it) instead of commenting it out.

>         else:
>-            self.ndk_url = 'https://dl.google.com/android/ndk/android-ndk-r10e-linux-x86.bin'
>+            #self.ndk_url = 'https://dl.google.com/android/ndk/android-ndk-r10e-linux-x86.bin'
>+            raise Exception('You need a 64-bit version of Linux to build Firefox for Android.')

Why are we adding an exception here (and in debian.py)? It looks like Android should build fine with 32-bit Linux - only OSX has the 64-bit restriction. Maybe android_ndk_url() needs another parameter that specifies x86_64 or x86?
Attachment #8684323 - Flags: review?(mshal)
Attachment #8684323 - Flags: feedback+
Attachment #8684323 - Flags: checkin?
(In reply to Michael Shal [:mshal] from comment #7)
> Comment on attachment 8684323 [details] [diff] [review]
> third.patch
> 
> >         if is_64bits:
> >-            self.ndk_url = 'https://dl.google.com/android/ndk/android-ndk-r10e-linux-x86_64.bin'
> >+            #self.ndk_url = 'https://dl.google.com/android/ndk/android-ndk-r10e-linux-x86_64.bin'
> 
> You should just delete this line (and the others like it) instead of
> commenting it out.
> 
> >         else:
> >-            self.ndk_url = 'https://dl.google.com/android/ndk/android-ndk-r10e-linux-x86.bin'
> >+            #self.ndk_url = 'https://dl.google.com/android/ndk/android-ndk-r10e-linux-x86.bin'
> >+            raise Exception('You need a 64-bit version of Linux to build Firefox for Android.')
> 
> Why are we adding an exception here (and in debian.py)? It looks like
> Android should build fine with 32-bit Linux - only OSX has the 64-bit
> restriction. Maybe android_ndk_url() needs another parameter that specifies
> x86_64 or x86?

I think this is correct.  Let's adjust the URL based on sys.maxint (like I said in https://bugzilla.mozilla.org/show_bug.cgi?id=1217039#c2).

mshal: thanks for reviewing this.  I'm happy to do further reviews.
Attached patch third.patch (obsolete) — Splinter Review
Improved patch!!!!!:):)
Attachment #8684323 - Attachment is obsolete: true
Attachment #8685059 - Flags: review?(mshal)
Attachment #8685059 - Flags: checkin?(mshal)
Comment on attachment 8685059 [details] [diff] [review]
third.patch

Looking good!

>+    if maxsize > 2**32 :
>+	return (base_url+ver+'-'+os_name+'-x86_64.bin')
>+    else :
>+	return (base_url+ver+'-'+os_name+'-x86.bin')

You could simplify this a little if you want by doing:

if maxsize > 2**32:
    arch = 'x86_64'
else:
    arch = 'x86'
return (base_url+ver+'-'+os_name+'-'+arch+'.bin')

(untested, but you get the idea)
Attachment #8685059 - Flags: review?(mshal) → review+
Attachment #8685059 - Flags: checkin?(mshal)
https://hg.mozilla.org/mozilla-central/rev/8084077bd124
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1227600
Attached patch third(new).patchSplinter Review
Attachment #8685059 - Attachment is obsolete: true
Attachment #8691582 - Flags: review?(gps)
Comment on attachment 8691582 [details] [diff] [review]
third(new).patch

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

This has some minor style nits. But it is otherwise fine.

::: python/mozboot/mozboot/android.py
@@ +236,5 @@
>  def suggest_mozconfig(sdk_path=None, ndk_path=None):
>      print(MOBILE_ANDROID_MOZCONFIG_TEMPLATE % (sdk_path, ndk_path))
> +
> +def android_ndk_url(os_name, ver='r10e') :
> +    from sys import maxsize

You can import sys at the file level.

@@ +242,5 @@
> +
> +    if maxsize > 2**32:
> +       arch = 'x86_64'
> +    else :
> +       arch = 'x86'

So, I'm pretty sure sys.maxsize is a proxy for whether the current Python is built in 32-bit or 64-bit mode. There is no guarantee that a 64-bit Python will be running on 64-bit hardware. There is also no guarantee that you'll be performing a 64-bit Firefox build on 64-bit hardware.

But I see from another patch we already employ this pattern, so it's probably good enough.

@@ +244,5 @@
> +       arch = 'x86_64'
> +    else :
> +       arch = 'x86'
> +
> +    return (base_url+ver+'-'+os_name+'-'+arch+'.bin')
\ No newline at end of file

You don't need the parenthesis. Also, Python typically uses string formatting operators:

return '%s%s-%s-%s.bin' % (base_url, ver, os_name, arch)
Attachment #8691582 - Flags: review?(gps) → review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.