1102266 - [gonk-l] Update gecko configure.in for newer api level

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Comment 1

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: bug-1102266.patch

Michael, 

Could you review this patch? I tried, we need to add the two condition checks for MOZ_WEBRTC, MOZ_FMP4; Otherwise, they will be enabled later.

Thanks!

Comment 2

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 8526554 [details] [diff] [review]
bug-1102266.patch

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

::: configure.in
@@ +297,5 @@
> +        AC_SUBST(MOZ_AUDIO_OFFLOAD)
> +        AC_DEFINE(MOZ_AUDIO_OFFLOAD)
> +        MOZ_FMP4=
> +        MOZ_WEBRTC=
> +        MOZ_NUWA_PROCESS=

Why disable NUWA/B2G_LOADER?

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 8526554 [details] [diff] [review]
bug-1102266.patch

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

::: configure.in
@@ +3886,4 @@
>  fi
>  MOZ_EME=1
>  MOZ_FFMPEG=
> +if test -n "$MOZ_WEBRTC"; then

This looks wrong. It looks like it'll turn off webrtc everywhere unless it's already explicitly turned on, which probably isn't what you intended.

Comment 4

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #2)
> 
> Why disable NUWA/B2G_LOADER?

It is disabled temporarily only.

Comment 5

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: bug-1102266.patch

Michael, could you have a look to this patch? For webrtc, I think disable it explicitly could be more straightforward.

Comment 6

[Michael Vines [:m1] [:evilmachines]]

(In reply to Kai-Zhen Li [:seinlin] from comment #4)
> (In reply to Michael Vines [:m1] [:evilmachines] from comment #2)
> > 
> > Why disable NUWA/B2G_LOADER?
> 
> It is disabled temporarily only.

Could you please explain why?

Comment 7

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 8527407 [details] [diff] [review]
bug-1102266.patch

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

::: configure.in
@@ +297,5 @@
> +        AC_SUBST(MOZ_AUDIO_OFFLOAD)
> +        AC_DEFINE(MOZ_AUDIO_OFFLOAD)
> +        # Disable fmp4 and webrtc before related bugs got fixed
> +        MOZ_FMP4=
> +        MOZ_WEBRTC=0

Why disable WebRTC?

Comment 8

[Kai-Zhen Li [:seinlin][:kli]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #7)
> 
> Why disable WebRTC?

Need to wait bug 1098970 get fixed.

Comment 9

[Michael Wu [:mwu]]

Comment on attachment 8527407 [details] [diff] [review]
bug-1102266.patch

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

::: configure.in
@@ +3888,5 @@
> +if test x"$MOZ_WEBRTC" = x"0"; then
> +  MOZ_WEBRTC=
> +else
> +  MOZ_WEBRTC=1
> +fi

Hmm this isn't too pretty..

Can you remove webrtc disabling for now?

Comment 10

[Michael Wu [:mwu]]

Attachment: Export TARGET_C_INCLUDES for gecko

This lets us simplify some of the configure logic in gecko by letting the Android build system tell us what the basic C includes are. Right now, we reproduce that knowledge in our configure files, and the basic set of includes have changed for lollipop.

Comment 11

[Michael Wu [:mwu]]

Attachment: Update configure.in to support gonk-L

This is based on Kai-Zhen's patch, but we take advantage of TARGET_C_INCLUDES to simplify some of the CPPFLAGS building.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8537857 [details] [diff] [review]
Update configure.in to support gonk-L

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

::: configure.in
@@ +3878,5 @@
> +if test x"$MOZ_WEBRTC" = x"0"; then
> +  MOZ_WEBRTC=
> +else
> +  MOZ_WEBRTC=1
> +fi

Use --disable-webrtc if you want to disable webrtc. No need to add this.

@@ +5329,5 @@
>  dnl ========================================================
>  dnl = Built-in fragmented MP4 support.
>  dnl ========================================================
>  
> +if test "$OS_TARGET" = Android -a x"$MOZ_WIDGET_TOOLKIT" != x"gonk"; then

Those x's are unnecessary.

Comment 14

[Dave Hylands [:dhylands]]

Comment on attachment 8537851 [details] [review]
Export TARGET_C_INCLUDES for gecko

I made a comment on the PR.

Comment 15

[Michael Wu [:mwu]]

(In reply to Dave Hylands [:dhylands] from comment #14)
> Comment on attachment 8537851 [details] [review]
> Export TARGET_C_INCLUDES for gecko
> 
> I made a comment on the PR.

PR updated with your suggestion. (seems like the force update ate your comment, though..)

Comment 16

[Michael Wu [:mwu]]

(In reply to Mike Hommey [:glandium] from comment #13)
> Comment on attachment 8537857 [details] [diff] [review]
> Update configure.in to support gonk-L
> 
> Review of attachment 8537857 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +3878,5 @@
> > +if test x"$MOZ_WEBRTC" = x"0"; then
> > +  MOZ_WEBRTC=
> > +else
> > +  MOZ_WEBRTC=1
> > +fi
> 
> Use --disable-webrtc if you want to disable webrtc. No need to add this.
> 

We don't want to disable webrtc through mozconfig though - that's in a different repo. I don't like this solution much but disabling features through variables makes things a bit easier. I can do this in a different place that'll look a bit more idiomatic.

> @@ +5329,5 @@
> >  dnl ========================================================
> >  dnl = Built-in fragmented MP4 support.
> >  dnl ========================================================
> >  
> > +if test "$OS_TARGET" = Android -a x"$MOZ_WIDGET_TOOLKIT" != x"gonk"; then
> 
> Those x's are unnecessary.

Will fix.

Comment 17

[Michael Wu [:mwu]]

Attachment: Update configure.in to support gonk-L, v2

The webrtc disabling looks less terrible here and will be removed when the port is completed.

Comment 18

[Michael Wu [:mwu]]

Landed the gonk-misc side of this bug.

https://github.com/mozilla-b2g/gonk-misc/commit/8aee09c106f479f36c57b2a29af72d455e359211

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8538192 [details] [diff] [review]
Update configure.in to support gonk-L, v2

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

::: configure.in
@@ +5097,5 @@
>      esac
>  fi
>  
> +dnl Temporary until webrtc works on gonk-L
> +if test -n "$gonkdir" -a "$ANDROID_VERSION" = "21"; then

-ge 21 ?

@@ +5330,4 @@
>  dnl = Built-in fragmented MP4 support.
>  dnl ========================================================
>  
> +if test "$OS_TARGET" = Android -a "$MOZ_WIDGET_TOOLKIT" != "gonk"; then

Not that it makes much difference, but -z "$gonkdir" ?

Comment 20

[Michael Wu [:mwu]]

(In reply to Mike Hommey [:glandium] from comment #19)
> Comment on attachment 8538192 [details] [diff] [review]
> Update configure.in to support gonk-L, v2
> 
> Review of attachment 8538192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +5097,5 @@
> >      esac
> >  fi
> >  
> > +dnl Temporary until webrtc works on gonk-L
> > +if test -n "$gonkdir" -a "$ANDROID_VERSION" = "21"; then
> 
> -ge 21 ?
> 

Sure. This test won't last long though, so it doesn't matter much..

> @@ +5330,4 @@
> >  dnl = Built-in fragmented MP4 support.
> >  dnl ========================================================
> >  
> > +if test "$OS_TARGET" = Android -a "$MOZ_WIDGET_TOOLKIT" != "gonk"; then
> 
> Not that it makes much difference, but -z "$gonkdir" ?

Will do.

Comment 21

[Michael Wu [:mwu]]

Attachment: Update configure.in to support gonk-L, v3

Comment 22

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e9750b2a3e42

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G ICS bustage.
https://hg.mozilla.org/integration/b2g-inbound/rev/17e5beb4e2a9

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1031740&repo=b2g-inbound

Comment 24

[Michael Wu [:mwu]]

Landed again with FMP4 explicitly enabled on ICS.

https://hg.mozilla.org/integration/b2g-inbound/rev/60721a39769d

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60721a39769d

Comment 26

[Anshul]

Hello Michael, this change seems to be causing the following compilation issue.

gonk-misc/Unicode.h:20:23: fatal error: sys/types.h: No such file or directory
 #include <sys/types.h>
                       ^
compilation terminated.
configure: failed program was:

#line 2831 "configure"
#include "confdefs.h"

int main(){return(0);}
configure: error: installation or configuration problem: C++ compiler cannot create executables.
*** Fix above errors and then restart with\
               "make -f client.mk build"
make[3]: *** [configure] Error 1
make[2]: ***

Comment 27

[Anshul]

I am trying to compile an Android L based build.

Comment 28

[Michael Wu [:mwu]]

Make sure your gonk-misc repo is updated. If you still have this problem, attach or email me your config.log so I can see exactly what is breaking.