Closed Bug 1276927 Opened 4 years ago Closed 4 years ago

Build B2G with modern C++11 compiler and STL

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
Tracking Status
firefox50 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 2 obsolete files)

58 bytes, text/x-review-board-request
fabrice
: review+
Details
58 bytes, text/x-review-board-request
fabrice
: review+
Details
58 bytes, text/x-review-board-request
fabrice
: review+
Details
58 bytes, text/x-review-board-request
dvander
: review+
Details
58 bytes, text/x-review-board-request
valentin
: review+
Details
58 bytes, text/x-review-board-request
mshal
: review+
Details
58 bytes, text/x-review-board-request
gsvelto
: review+
Details
58 bytes, text/x-review-board-request
fabrice
: review+
Details
58 bytes, text/x-review-board-request
glandium
: review+
Details
64 bytes, text/x-github-pull-request
fabrice
: review+
Details | Review
49 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
52 bytes, text/x-github-pull-request
gerard-majax
: review+
Details | Review
Very soon, STLport will be removed and Gecko will use only support C++11 and later. B2G must be converted to build with a modern C++11 compiler and STL. This change is also required for the WebTablet/FirefoxPad project.
Attached file gonk-misc (obsolete) —
With the attached patches, B2G is build with Android-based build scripts. That is C++11 (gcc-4.9) with libc++. You'll have to install the Android NDK and SDK, as described in [1], sec 'Bootstrap dependencies'. Choose 'Firefox for Android.' These patches work for me with aries-l and flame-kk. On aries-l, you'll also need the fix to platform_frameworks_native.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build
Attachment #8758578 - Attachment is obsolete: true
See Also: → 1229391
See Also: → 1095125
Comment on attachment 8758568 [details]
Bug 1276927: Fix B2G widget code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56832/diff/1-2/
Attachment #8758568 - Attachment description: MozReview Request: Bug 1276927: Fix B2G widget code to build with Android NDK → MozReview Request: Bug 1276927: Fix B2G widget code to build with Android NDK, r?fabrice
Attachment #8758569 - Attachment description: MozReview Request: Bug 1276927: Fix B2G sandboxing code to build with Android NDK → MozReview Request: Bug 1276927: Fix B2G sandboxing code to build with Android NDK, r?fabrice
Attachment #8758570 - Attachment description: MozReview Request: Bug 1276927: Fix DOM code to build with Android NDK → MozReview Request: Bug 1276927: Fix DOM code to build with Android NDK, r?fabrice
Attachment #8758571 - Attachment description: MozReview Request: Bug 1276927: Remove Gonk from JS build scripts where possible → MozReview Request: Bug 1276927: Remove Gonk from JS builds where possible, r?waldo
Attachment #8758572 - Attachment description: MozReview Request: Bug 1276927: Remove Bionic dependencies from Necko → MozReview Request: Bug 1276927: Remove Bionic dependencies from Necko, r?fabrice
Attachment #8758573 - Attachment description: MozReview Request: Bug 1276927: Fix breakpad to build on B2G → MozReview Request: Bug 1276927: Fix breakpad to build on B2G, r?ted
Attachment #8758574 - Attachment description: MozReview Request: Bug 1276927: Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux → MozReview Request: Bug 1276927: Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux, r?fabrice
Attachment #8758575 - Attachment description: MozReview Request: Bug 1276927: Define HAVE_ANDROID_OS before including 'android_filesystem_config.h' → MozReview Request: Bug 1276927: Define HAVE_ANDROID_OS before including 'android_filesystem_config.h', r?fabrice
Attachment #8758576 - Attachment description: MozReview Request: Bug 1276927: Build B2G with Android build scripts where possible → MozReview Request: Bug 1276927: Build B2G with Android build scripts where possible, r?glandium
Attachment #8758568 - Flags: review?(fabrice)
Attachment #8758569 - Flags: review?(fabrice)
Attachment #8758570 - Flags: review?(fabrice)
Attachment #8758571 - Flags: review?(jwalden+bmo)
Attachment #8758572 - Flags: review?(fabrice)
Attachment #8758573 - Flags: review?(ted)
Attachment #8758574 - Flags: review?(fabrice)
Attachment #8758575 - Flags: review?(fabrice)
Attachment #8758576 - Flags: review?(mh+mozilla)
Comment on attachment 8758569 [details]
Bug 1276927: Fix B2G sandboxing code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56834/diff/1-2/
Comment on attachment 8758570 [details]
Bug 1276927: Fix DOM code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56836/diff/1-2/
Comment on attachment 8758571 [details]
Bug 1276927: Remove Gonk from JS builds where possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56838/diff/1-2/
Comment on attachment 8758572 [details]
Bug 1276927: Remove Bionic dependencies from Necko,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56840/diff/1-2/
Comment on attachment 8758573 [details]
Bug 1276927: Fix breakpad to build on B2G,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56842/diff/1-2/
Comment on attachment 8758574 [details]
Bug 1276927: Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56844/diff/1-2/
Comment on attachment 8758575 [details]
Bug 1276927: Define HAVE_ANDROID_OS before including 'android_filesystem_config.h',

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56846/diff/1-2/
Comment on attachment 8758576 [details]
Bug 1276927: Build B2G with Android build scripts where possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56848/diff/1-2/
Comment on attachment 8758665 [details] [review]
Github pull request for platform_frameworks_native

This is a new repository. Do we still have to mirror it on git.m.o?
Attachment #8758665 - Flags: review?(fabrice)
Attachment #8758666 - Flags: review?(fabrice)
Attachment #8758687 - Flags: review?(fabrice)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #21)
> Comment on attachment 8758573 [details]
> MozReview Request: Bug 1276927: Fix breakpad to build on B2G, r?ted
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/56842/diff/1-2/

This patch also fixes bug 1270879.
The change https://reviewboard.mozilla.org/r/56848/diff/2#index_header has a conflict with master you probably have to rebase.
I applied your changes onto commit 463212f69f93f826f64693ce003ff71bb8c45d06, they fix that issue in my environment. 

gcc -v
Using built-in specs.
COLLECT_GCC=/usr/bin/gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/5.3.1/lto-wrapper
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,objc,obj-c++,fortran,ada,go,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --disable-libgcj --with-isl --enable-libmpx --enable-gnu-indirect-function --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 5.3.1 20160406 (Red Hat 5.3.1-6) (GCC)
Comment on attachment 8758576 [details]
Bug 1276927: Build B2G with Android build scripts where possible,

https://reviewboard.mozilla.org/r/56848/#review53866

::: build/moz.configure/toolchain.configure:64
(Diff revision 2)
>  # Android NDK
>  # ==============================================================
>  
>  @depends('--disable-compile-environment', build_project, '--help')
>  def android_ndk_include(compile_env, build_project, _):
> -    if compile_env and build_project in ('mobile/android', 'js'):
> +    if compile_env and build_project in ('b2g', 'mobile/android', 'js'):

Here you'll want to check gonkdir instead, since, afaik, you can build b2g without gonk.

::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/android.py
(Diff revision 2)
> -    'bionic/libc/arch-arm/include',
> -    'bionic/libc/include',
> -    'bionic/libstdc++/include',
> -    'bionic/libc/kernel/common',
> -    'bionic/libc/kernel/arch-arm',
> -    'bionic/libm/include',
> -    'bionic/libm/include/arm',
> -    'bionic/libthread_db/include',

AFAIK, this is not used, so you don't need to remove this.

::: old-configure.in:2887
(Diff revision 2)
>  dnl ========================================================
>  dnl Ensure Android SDK and build-tools versions depending on
>  dnl mobile target.
>  dnl ========================================================
>  
> -if test -z "$gonkdir" ; then
> +dnl if test -z "$gonkdir" ; then

Please remove lines instead of commenting them. That said, you don't want the Android SDK for B2G. It's for the java stuff.
Attachment #8758576 - Flags: review?(mh+mozilla)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #25)
> Comment on attachment 8758665 [details] [review]
> Github pull request for platform_frameworks_native
> 
> This is a new repository. Do we still have to mirror it on git.m.o?

No, we don't need any mirroring anymore.
https://reviewboard.mozilla.org/r/56848/#review53866

> Here you'll want to check gonkdir instead, since, afaik, you can build b2g without gonk.

I think, I'd have to add 'gonkdir' to the function's parameters. Instead I added the other b2g targets (b2g/dev, b2g/graphene) to the list. Seems less intrusive. OK?

> AFAIK, this is not used, so you don't need to remove this.

In some places, header files in bionic interfered with the ones in the NDK. In those cases, I removed bionic from the preprocessor's search path. While I'm at it, I'd like to remove all references to bionic. OK?

> Please remove lines instead of commenting them. That said, you don't want the Android SDK for B2G. It's for the java stuff.

I reverted this back to the old code.
Comment on attachment 8758666 [details] [review]
Github pull request for gonk-misc

Updated Github pull request:

  - don't set SDK path
  - NDK path not configurable with B2G_ANDROID_NDK_PATH
  - comment fixes
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #30)
> https://reviewboard.mozilla.org/r/56848/#review53866
> 
> > Here you'll want to check gonkdir instead, since, afaik, you can build b2g without gonk.
> 
> I think, I'd have to add 'gonkdir' to the function's parameters. Instead I
> added the other b2g targets (b2g/dev, b2g/graphene) to the list. Seems less
> intrusive. OK?

b2g/dev and b2g/graphene don't build against gonk. b2g does, but it also builds without gonk. So you need to check gonkdir.

> > AFAIK, this is not used, so you don't need to remove this.
> 
> In some places, header files in bionic interfered with the ones in the NDK.
> In those cases, I removed bionic from the preprocessor's search path. While
> I'm at it, I'd like to remove all references to bionic. OK?

We avoid changing third party code when we can, and here, it's, afaict, not used at all. So it's better to leave it alone.
(In reply to Mike Hommey [:glandium] from comment #32)
> > > AFAIK, this is not used, so you don't need to remove this.
> > 
> > In some places, header files in bionic interfered with the ones in the NDK.
> > In those cases, I removed bionic from the preprocessor's search path. While
> > I'm at it, I'd like to remove all references to bionic. OK?
> 
> We avoid changing third party code when we can, and here, it's, afaict, not
> used at all. So it's better to leave it alone.

OK. Makes sense.
(In reply to Mike Hommey [:glandium] from comment #32)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #30)
> > https://reviewboard.mozilla.org/r/56848/#review53866
> > 
> > > Here you'll want to check gonkdir instead, since, afaik, you can build b2g without gonk.
> > 
> > I think, I'd have to add 'gonkdir' to the function's parameters. Instead I
> > added the other b2g targets (b2g/dev, b2g/graphene) to the list. Seems less
> > intrusive. OK?
> 
> b2g/dev and b2g/graphene don't build against gonk. b2g does, but it also
> builds without gonk. So you need to check gonkdir.

I see.

My test now looks like this:

  @depends('--disable-compile-environment', build_project, '--with-gonk', '--help')
  def android_ndk_include(compile_env, build_project, gonkdir, _):
      if compile_env and (gonkdir or build_project in ('mobile/android', 'js')):
          return 'android-ndk.configure'

I got this working, but the only solution I found is to move most of the code in 'b2g/moz.configure' to 'build/moz.configure/toolchain.configure'. Otherwise mozbuild tells me that '--with-gonk' is an unknown option. If I use 'gonkdir' in the @depends statement, the error message is that 'wrapper should depend on --help'.

Is there a more elegant solution than the one I have? I tried to understand how 'build_project' works, but I don't really see the difference. (Sorry for the noob questions.)
Flags: needinfo?(mh+mozilla)
Comment on attachment 8758568 [details]
Bug 1276927: Fix B2G widget code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56832/diff/2-3/
Attachment #8758568 - Attachment description: MozReview Request: Bug 1276927: Fix B2G widget code to build with Android NDK, r?fabrice → Bug 1276927: Fix B2G widget code to build with Android NDK,
Attachment #8758569 - Attachment description: MozReview Request: Bug 1276927: Fix B2G sandboxing code to build with Android NDK, r?fabrice → Bug 1276927: Fix B2G sandboxing code to build with Android NDK,
Attachment #8758570 - Attachment description: MozReview Request: Bug 1276927: Fix DOM code to build with Android NDK, r?fabrice → Bug 1276927: Fix DOM code to build with Android NDK,
Attachment #8758571 - Attachment description: MozReview Request: Bug 1276927: Remove Gonk from JS builds where possible, r?waldo → Bug 1276927: Remove Gonk from JS builds where possible,
Attachment #8758572 - Attachment description: MozReview Request: Bug 1276927: Remove Bionic dependencies from Necko, r?fabrice → Bug 1276927: Remove Bionic dependencies from Necko,
Attachment #8758573 - Attachment description: MozReview Request: Bug 1276927: Fix breakpad to build on B2G, r?ted → Bug 1276927: Fix breakpad to build on B2G,
Attachment #8758574 - Attachment description: MozReview Request: Bug 1276927: Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux, r?fabrice → Bug 1276927: Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux,
Attachment #8758575 - Attachment description: MozReview Request: Bug 1276927: Define HAVE_ANDROID_OS before including 'android_filesystem_config.h', r?fabrice → Bug 1276927: Define HAVE_ANDROID_OS before including 'android_filesystem_config.h',
Attachment #8758576 - Attachment description: MozReview Request: Bug 1276927: Build B2G with Android build scripts where possible, r?glandium → Bug 1276927: Build B2G with Android build scripts where possible,
Attachment #8758574 - Flags: review?(fabrice) → review?(gsvelto)
Attachment #8758576 - Flags: review?(mh+mozilla)
Comment on attachment 8758569 [details]
Bug 1276927: Fix B2G sandboxing code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56834/diff/2-3/
Comment on attachment 8758570 [details]
Bug 1276927: Fix DOM code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56836/diff/2-3/
Comment on attachment 8758571 [details]
Bug 1276927: Remove Gonk from JS builds where possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56838/diff/2-3/
Comment on attachment 8758572 [details]
Bug 1276927: Remove Bionic dependencies from Necko,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56840/diff/2-3/
Comment on attachment 8758573 [details]
Bug 1276927: Fix breakpad to build on B2G,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56842/diff/2-3/
Comment on attachment 8758574 [details]
Bug 1276927: Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56844/diff/2-3/
Comment on attachment 8758575 [details]
Bug 1276927: Define HAVE_ANDROID_OS before including 'android_filesystem_config.h',

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56846/diff/2-3/
Comment on attachment 8758576 [details]
Bug 1276927: Build B2G with Android build scripts where possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56848/diff/2-3/
Comment on attachment 8758574 [details]
Bug 1276927: Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux,

https://reviewboard.mozilla.org/r/56844/#review54738

LGTM
Attachment #8758574 - Flags: review?(gsvelto) → review+
Attachment #8758568 - Flags: review?(fabrice) → review+
Comment on attachment 8758569 [details]
Bug 1276927: Fix B2G sandboxing code to build with Android NDK,

https://reviewboard.mozilla.org/r/56834/#review54904
Attachment #8758569 - Flags: review?(fabrice) → review+
Comment on attachment 8758570 [details]
Bug 1276927: Fix DOM code to build with Android NDK,

https://reviewboard.mozilla.org/r/56836/#review54906
Attachment #8758570 - Flags: review?(fabrice) → review+
https://reviewboard.mozilla.org/r/56840/#review54910

I don't know if that's ok to also remove this code for Fennec.
Attachment #8758575 - Flags: review?(fabrice) → review+
Comment on attachment 8758575 [details]
Bug 1276927: Define HAVE_ANDROID_OS before including 'android_filesystem_config.h',

https://reviewboard.mozilla.org/r/56846/#review54916
Attachment #8758665 - Flags: review?(fabrice) → review+
Attachment #8758666 - Flags: review?(fabrice) → review?(lissyx+mozillians)
Attachment #8758687 - Flags: review?(fabrice) → review?(lissyx+mozillians)
Attachment #8758576 - Flags: review?(mh+mozilla)
Comment on attachment 8758576 [details]
Bug 1276927: Build B2G with Android build scripts where possible,

https://reviewboard.mozilla.org/r/56848/#review54964

::: build/moz.configure/toolchain.configure:80
(Diff revision 3)
> -@depends('--disable-compile-environment', build_project, '--help')
> -def android_ndk_include(compile_env, build_project, _):
> -    if compile_env and build_project in ('mobile/android', 'js'):
> +@depends('--disable-compile-environment', build_project, '--with-gonk', '--help')
> +def android_ndk_include(compile_env, build_project, gonkdir, _):
> +    if compile_env and (gonkdir or build_project in ('mobile/android', 'js')):
>          return 'android-ndk.configure'

I'd rather not move --with-gonk here just for this, and I was trying to think of a way to do what you want to do, with gonkdir, and couldn't think of anything nice.

Then I took a step back. What do we need the Android NDK for? For mobile/android builds, for js targeting Android, and for b2g targeting gonk.

Also:
- A mobile/android build is always targeting Android.
- A b2g build may be targeting gonk, or some other target, but it won't be targeting Android.

But, all in all, what is Gonk? More or less Android, without the Java layer. As far as build targets are concerned, gonk and Android are, in fact, indistinguishable.

So, really, what this means, is that our requirement to include android-ndk.configure is, now, "build target is Android" (and building with a compile environment).

Which means this should now work with:

@depends('--disable-compile-environment', target, '--help')
def android_ndk_include(compile_env, target, _):
    if compile_env and target.os == 'Android':
        return 'android-ndk.configure'

It was not possible to do this before because b2g wasn't using android-ndk.configure.

::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/android.py
(Diff revision 3)
> -    'bionic/libc/arch-arm/include',
> -    'bionic/libc/include',
> -    'bionic/libstdc++/include',
> -    'bionic/libc/kernel/common',
> -    'bionic/libc/kernel/arch-arm',
> -    'bionic/libm/include',
> -    'bionic/libm/include/arm',
> -    'bionic/libthread_db/include',

See previous review and subsequent comments.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #50)
> Comment on attachment 8758576 [details]
> Bug 1276927: Build B2G with Android build scripts where possible,
> 
> Which means this should now work with:
> 
> @depends('--disable-compile-environment', target, '--help')
> def android_ndk_include(compile_env, target, _):
>     if compile_env and target.os == 'Android':
>         return 'android-ndk.configure'

Fantastic! This works even better. The only thing is that I had to add '--help' to 'target' and a few other options to resolve dependencies.

> ::: media/webrtc/trunk/tools/gyp/pylib/gyp/generator/android.py
> (Diff revision 3)
> > -    'bionic/libc/arch-arm/include',
> > -    'bionic/libc/include',
> > -    'bionic/libstdc++/include',
> > -    'bionic/libc/kernel/common',
> > -    'bionic/libc/kernel/arch-arm',
> > -    'bionic/libm/include',
> > -    'bionic/libm/include/arm',
> > -    'bionic/libthread_db/include',
> 
> See previous review and subsequent comments.

Hmm, this should not have been in the patch actually. Will be fixed in the next iteration.
https://reviewboard.mozilla.org/r/56848/#review53866

> In some places, header files in bionic interfered with the ones in the NDK. In those cases, I removed bionic from the preprocessor's search path. While I'm at it, I'd like to remove all references to bionic. OK?

Fixed in next revision.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #51)
> (In reply to Mike Hommey [:glandium] from comment #50)
> > Comment on attachment 8758576 [details]
> > Bug 1276927: Build B2G with Android build scripts where possible,
> > 
> > Which means this should now work with:
> > 
> > @depends('--disable-compile-environment', target, '--help')
> > def android_ndk_include(compile_env, target, _):
> >     if compile_env and target.os == 'Android':
> >         return 'android-ndk.configure'
> 
> Fantastic! This works even better. The only thing is that I had to add
> '--help' to 'target' and a few other options to resolve dependencies.

Ah crap... that's not something I'd like to happen. That has implications that moz.configure is not ready to deal with. So I guess the least worse thing to do at the moment, is to define a dummy gonkdir function before this like:
https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/init.configure#796

Something like this:

@depends('--help')
def gonkdir(_):
    return None

Then you can have @depends(gonkdir) for android_ndk_include and do the check you were doing with --with-gonk.

Then, please file a followup bug so make things use target eventually.
(In reply to [:fabrice] Fabrice Desré from comment #48)
> https://reviewboard.mozilla.org/r/56840/#review54910
> 
> I don't know if that's ok to also remove this code for Fennec.

I think Fennec might not be affected. As far as I've been told, Fennec always builds with ANDROID_VERSION ~ 9 to support old Android releases. And the required header <resolv_netid.h> isn't available in the NDK, so Fennec builds should fail.

I'll ask a Necko peer for a review.
Comment on attachment 8758666 [details] [review]
Github pull request for gonk-misc

just a small nit about if/else but good to me :)
Attachment #8758666 - Flags: review?(lissyx+mozillians) → review+
Attachment #8758687 - Flags: review?(lissyx+mozillians) → review+
(In reply to Mike Hommey [:glandium] from comment #53)
> Something like this:
> 
> @depends('--help')
> def gonkdir(_):
>     return None
> 
> Then you can have @depends(gonkdir) for android_ndk_include and do the check
> you were doing with --with-gonk.

I use this code:

  @depends('--help')
  def gonkdir(_):
      return None

  @depends('--disable-compile-environment', build_project, gonkdir, '--help')
  def android_ndk_include(compile_env, build_project, gonkdir, _):
      if compile_env and (gonkdir or build_project in ('mobile/android', 'js')):
          return 'android-ndk.configure'

I always get 'mozbuild.configure.options.InvalidOptionError: Unknown option: --with-android-ndk' when running the config scripts. I think it evaluates 'gonkdir' to 'None' and therefore never returns android-ndk.configure.
Flags: needinfo?(mh+mozilla)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #56)
> (In reply to Mike Hommey [:glandium] from comment #53)
> > Something like this:
> > 
> > @depends('--help')
> > def gonkdir(_):
> >     return None
> > 
> > Then you can have @depends(gonkdir) for android_ndk_include and do the check
> > you were doing with --with-gonk.
> 
> I use this code:
> 
>   @depends('--help')
>   def gonkdir(_):
>       return None

Did you put this before https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/init.configure#796 as per comment 53?
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #57)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #56)
> > (In reply to Mike Hommey [:glandium] from comment #53)
> > > Something like this:
> > > 
> > > @depends('--help')
> > > def gonkdir(_):
> > >     return None
> > > 
> > > Then you can have @depends(gonkdir) for android_ndk_include and do the check
> > > you were doing with --with-gonk.
> > 
> > I use this code:
> > 
> >   @depends('--help')
> >   def gonkdir(_):
> >       return None
> 
> Did you put this before
> https://dxr.mozilla.org/mozilla-central/source/build/moz.configure/init.
> configure#796 as per comment 53?

I misunderstood you comment 53. If I put the dummy 'gonkdir' function to that location, I get:

  mozbuild.configure.ConfigureError: `android_ndk_include` depends on '--help' and `wrapper`. `wrapper` must depend on '--help'

which is fixable by making the real |gonkdir| function also depend on '--help'.
Comment on attachment 8758568 [details]
Bug 1276927: Fix B2G widget code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56832/diff/3-4/
Attachment #8758571 - Flags: review?(jwalden+bmo) → review?(dvander)
Attachment #8758572 - Flags: review?(mcmanus)
Attachment #8758573 - Flags: review?(ted) → review?(mshal)
Attachment #8758576 - Flags: review?(mh+mozilla)
Comment on attachment 8758569 [details]
Bug 1276927: Fix B2G sandboxing code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56834/diff/3-4/
Comment on attachment 8758570 [details]
Bug 1276927: Fix DOM code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56836/diff/3-4/
Comment on attachment 8758571 [details]
Bug 1276927: Remove Gonk from JS builds where possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56838/diff/3-4/
Comment on attachment 8758572 [details]
Bug 1276927: Remove Bionic dependencies from Necko,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56840/diff/3-4/
Comment on attachment 8758573 [details]
Bug 1276927: Fix breakpad to build on B2G,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56842/diff/3-4/
Comment on attachment 8758574 [details]
Bug 1276927: Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56844/diff/3-4/
Comment on attachment 8758575 [details]
Bug 1276927: Define HAVE_ANDROID_OS before including 'android_filesystem_config.h',

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56846/diff/3-4/
Comment on attachment 8758576 [details]
Bug 1276927: Build B2G with Android build scripts where possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56848/diff/3-4/
Comment on attachment 8758666 [details] [review]
Github pull request for gonk-misc

Updated Github pull request:

  - simplified test/set B2G_ANDROID_NDK_PATH
Attachment #8758572 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8758572 - Flags: review?(valentin.gosu) → review+
Attachment #8758573 - Flags: review?(mshal) → review+
Comment on attachment 8758576 [details]
Bug 1276927: Build B2G with Android build scripts where possible,

https://reviewboard.mozilla.org/r/56848/#review55320

::: b2g/moz.configure
(Diff revision 4)
> -set_config('ANDROID_NDK',
> -           depends_if(gonkdir)(lambda x: os.path.join(x, 'ndk')))

I'd expect removing this forces you to pass --with-android-ndk which you didn't need before. What you can do is something like:

imply_option('--with-android-ndk', depends_if(gonkdir)(lambda x: os.path.join(x, 'ndk'))))

::: old-configure.in:191
(Diff revision 4)
> -    CFLAGS="-mandroid -fno-short-enums -fno-exceptions $CFLAGS"
> -    CXXFLAGS="-mandroid -fno-short-enums -fno-exceptions $CXXFLAGS $STLPORT_CPPFLAGS"
> +    CPPFLAGS="-I$gonkdir/system -I$gonkdir/system/core/include -I$gonkdir/hardware/libhardware/include -I$gonkdir/external/valgrind/fxos-include $GONK_INCLUDES $CPPFLAGS"
> +
>      dnl Add -llog by default, since we use it all over the place.
>      LIBS="$LIBS -llog"
>  
> -    LDFLAGS="-mandroid -L$gonkdir/out/target/product/$GONK_PRODUCT/obj/lib -Wl,-rpath-link=$gonkdir/out/target/product/$GONK_PRODUCT/obj/lib --sysroot=$gonkdir/out/target/product/$GONK_PRODUCT/obj/ $LDFLAGS"
> +    LDFLAGS="-L$gonkdir/out/target/product/$GONK_PRODUCT/obj/lib -Wl,-rpath-link=$gonkdir/out/target/product/$GONK_PRODUCT/obj/lib $LDFLAGS"

You'll need to rebase on top of bug 1262052.
Attachment #8758576 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/56848/#review55320

> You'll need to rebase on top of bug 1262052.

Done locally.
https://reviewboard.mozilla.org/r/56848/#review55320

> I'd expect removing this forces you to pass --with-android-ndk which you didn't need before. What you can do is something like:
> 
> imply_option('--with-android-ndk', depends_if(gonkdir)(lambda x: os.path.join(x, 'ndk'))))

The NDK that is located in the B2G directory is not useful for building Gecko. There already is a default NDK selected in the default-config script. [1] What I could do is to move this default here instead.

[1] https://github.com/mozilla-b2g/gonk-misc/pull/273/commits/8b88e0ddcfb658e3486694d0503251d7ba136591#diff-ad0844719b7ffa19d01c90446b218880R30
https://reviewboard.mozilla.org/r/56848/#review55320

> The NDK that is located in the B2G directory is not useful for building Gecko. There already is a default NDK selected in the default-config script. [1] What I could do is to move this default here instead.
> 
> [1] https://github.com/mozilla-b2g/gonk-misc/pull/273/commits/8b88e0ddcfb658e3486694d0503251d7ba136591#diff-ad0844719b7ffa19d01c90446b218880R30

I tried to implement this, but it doesn't work because 'os' isn't know in the configure script. I added 'import os', which results in the error: 'raise ImportError('Importing modules is forbidden')'.

Can we leave it as it is?
Comment on attachment 8758568 [details]
Bug 1276927: Fix B2G widget code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56832/diff/4-5/
Attachment #8758572 - Flags: review?(fabrice)
Attachment #8758576 - Flags: review?(mh+mozilla)
Comment on attachment 8758569 [details]
Bug 1276927: Fix B2G sandboxing code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56834/diff/4-5/
Comment on attachment 8758570 [details]
Bug 1276927: Fix DOM code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56836/diff/4-5/
Comment on attachment 8758571 [details]
Bug 1276927: Remove Gonk from JS builds where possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56838/diff/4-5/
Comment on attachment 8758572 [details]
Bug 1276927: Remove Bionic dependencies from Necko,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56840/diff/4-5/
Comment on attachment 8758573 [details]
Bug 1276927: Fix breakpad to build on B2G,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56842/diff/4-5/
Comment on attachment 8758574 [details]
Bug 1276927: Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56844/diff/4-5/
Comment on attachment 8758575 [details]
Bug 1276927: Define HAVE_ANDROID_OS before including 'android_filesystem_config.h',

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56846/diff/4-5/
Comment on attachment 8758576 [details]
Bug 1276927: Build B2G with Android build scripts where possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56848/diff/4-5/
https://reviewboard.mozilla.org/r/56848/#review55320

> I tried to implement this, but it doesn't work because 'os' isn't know in the configure script. I added 'import os', which results in the error: 'raise ImportError('Importing modules is forbidden')'.
> 
> Can we leave it as it is?

As discussed on IRC, I kept this code as it is.
Comment on attachment 8758576 [details]
Bug 1276927: Build B2G with Android build scripts where possible,

https://reviewboard.mozilla.org/r/56848/#review56274
Attachment #8758576 - Flags: review?(mh+mozilla) → review+
Pushed by tdz@users.sourceforge.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4220e8faa3
Fix B2G widget code to build with Android NDK, r=fabrice
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f95658a29b
Fix B2G sandboxing code to build with Android NDK, r=fabrice
https://hg.mozilla.org/integration/mozilla-inbound/rev/0086e164a0d6
Fix DOM code to build with Android NDK, r=fabrice
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d8bc4c921f
Remove Gonk from JS builds where possible, r=dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a18f4684bdd
Remove Bionic dependencies from Necko, r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7cc17d184bd
Fix breakpad to build on B2G, r=mshal
https://hg.mozilla.org/integration/mozilla-inbound/rev/18924ea45917
Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux, r=gsvelto
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6c190b08824
Define HAVE_ANDROID_OS before including 'android_filesystem_config.h', r=fabrice
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f4144c4dbc
Build B2G with Android build scripts where possible, r=glandium
Comment on attachment 8758568 [details]
Bug 1276927: Fix B2G widget code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56832/diff/5-6/
Comment on attachment 8758569 [details]
Bug 1276927: Fix B2G sandboxing code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56834/diff/5-6/
Comment on attachment 8758570 [details]
Bug 1276927: Fix DOM code to build with Android NDK,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56836/diff/5-6/
Comment on attachment 8758571 [details]
Bug 1276927: Remove Gonk from JS builds where possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56838/diff/5-6/
Comment on attachment 8758572 [details]
Bug 1276927: Remove Bionic dependencies from Necko,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56840/diff/5-6/
Comment on attachment 8758573 [details]
Bug 1276927: Fix breakpad to build on B2G,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56842/diff/5-6/
Comment on attachment 8758574 [details]
Bug 1276927: Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56844/diff/5-6/
Comment on attachment 8758575 [details]
Bug 1276927: Define HAVE_ANDROID_OS before including 'android_filesystem_config.h',

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56846/diff/5-6/
Comment on attachment 8758576 [details]
Bug 1276927: Build B2G with Android build scripts where possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56848/diff/5-6/
Pushed by tdz@users.sourceforge.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5702f8571cb
Fix B2G widget code to build with Android NDK, r=fabrice
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb69f71885c
Fix B2G sandboxing code to build with Android NDK, r=fabrice
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa274c30c016
Fix DOM code to build with Android NDK, r=fabrice
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f3ce8ed0d1
Remove Gonk from JS builds where possible, r=dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/26c264de3544
Remove Bionic dependencies from Necko, r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/318e39c41b79
Fix breakpad to build on B2G, r=mshal
https://hg.mozilla.org/integration/mozilla-inbound/rev/bacf083383fe
Add V4L ioctl 'VIDIOC_S_HW_FREQ_SEEK' if undefined by Linux, r=gsvelto
https://hg.mozilla.org/integration/mozilla-inbound/rev/442a1085ccee
Define HAVE_ANDROID_OS before including 'android_filesystem_config.h', r=fabrice
https://hg.mozilla.org/integration/mozilla-inbound/rev/c00929700590
Build B2G with Android build scripts where possible, r=glandium
Breakpad build flags were not working correctly on all platforms.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c00929700590
Duplicate of this bug: 1275531
Duplicate of this bug: 1270879
https://reviewboard.mozilla.org/r/56842/#review58586

::: toolkit/crashreporter/google-breakpad/src/common/android/include/link.h:46
(Diff revision 6)
>  
>  #ifdef __cplusplus
>  extern "C" {
>  #endif  // __cplusplus
>  
> +#if defined(ANDROID) && ANDROID_VERSION <= 20

You've made a change to an upstream Breakpad header here. If you don't submit this upstream it'll get lost the next time we sync with upstream (probably bug 1264367).

You can submit patches against upstream by following these directions:
https://chromium.googlesource.com/breakpad/breakpad/+/master/#To-request-change-review

If you ping me or tag me on the review I can help ensure it gets landed.
I didn't know. Thanks for the info!
Flags: needinfo?(tzimmermann)
It looks like there's already a bug report for removing this file: https://bugs.chromium.org/p/chromium/issues/detail?id=358831
Flags: needinfo?(tzimmermann)
Flags: needinfo?(tzimmermann)
I opened a bug report at [1] and uploaded the patch for review at [2].

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=629088
[2] https://codereview.chromium.org/2156173002/
A fix landed in the upstream Breakpad repo.
Flags: needinfo?(tzimmermann)
You need to log in before you can comment on or make changes to this bug.