Closed Bug 1102266 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: GonkIntegration, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S2 (19dec)

People

(Reporter: seinlin, Assigned: mwu)

References

Details

Attachments

(2 files, 4 obsolete files)

No description provided.
Blocks: gonk-L
Attached patch bug-1102266.patch (obsolete) — Splinter Review
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!
Attachment #8526554 - Flags: review?(mwu)
Assignee: nobody → kli
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 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.
Attachment #8526554 - Flags: review?(mwu)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #2)
> 
> Why disable NUWA/B2G_LOADER?

It is disabled temporarily only.
Attached patch bug-1102266.patch (obsolete) — Splinter Review
Michael, could you have a look to this patch? For webrtc, I think disable it explicitly could be more straightforward.
Attachment #8527407 - Flags: review?(mwu)
(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 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?
(In reply to Michael Vines [:m1] [:evilmachines] from comment #7)
> 
> Why disable WebRTC?

Need to wait bug 1098970 get fixed.
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?
Attachment #8527407 - Flags: review?(mwu)
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.
Assignee: kli → mwu
Attachment #8526554 - Attachment is obsolete: true
Attachment #8527407 - Attachment is obsolete: true
Attachment #8537851 - Flags: review?(dhylands)
This is based on Kai-Zhen's patch, but we take advantage of TARGET_C_INCLUDES to simplify some of the CPPFLAGS building.
Attachment #8537857 - Flags: review?(mh+mozilla)
Duplicate of this bug: 1103826
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.
Attachment #8537857 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8537851 [details] [review]
Export TARGET_C_INCLUDES for gecko

I made a comment on the PR.
Attachment #8537851 - Flags: review?(dhylands) → review+
(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..)
(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.
The webrtc disabling looks less terrible here and will be removed when the port is completed.
Attachment #8537857 - Attachment is obsolete: true
Attachment #8538192 - Flags: review?(mh+mozilla)
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" ?
Attachment #8538192 - Flags: review?(mh+mozilla) → review+
(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.
Attachment #8538192 - Attachment is obsolete: true
Landed again with FMP4 explicitly enabled on ICS.

https://hg.mozilla.org/integration/b2g-inbound/rev/60721a39769d
https://hg.mozilla.org/mozilla-central/rev/60721a39769d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S2 (19dec)
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]: ***
Flags: needinfo?(mwu)
I am trying to compile an Android L based build.
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.
Flags: needinfo?(mwu)
You need to log in before you can comment on or make changes to this bug.