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

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.

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.

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.

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?

(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.

Improved patch!!!!!:):)

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)

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)