Closed Bug 880610 Opened 11 years ago Closed 11 years ago

(gonk-jb)Add build flag to support both bluez/bluedroid stacks

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
1.3 Sprint 3 - 10/25

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

Attachments

(4 files, 13 obsolete files)

b2g-desktop error log
10.21 KB, text/plain
Details
Bug 880610: Part3-[Bluetooth]bluedroid-Prototype: BluetootServiceBluedroid, r=echou
16.88 KB, patch
Details | Diff | Splinter Review
Bug 880610: Part2-Add build flag in UnixSocket to distinguish between bluez and bluedroid stack, r=echou
5.56 KB, patch
Details | Diff | Splinter Review
Bug 880610: Part1-Add build flag to support both bluez/bluedroid stacks, r=glandium
7.58 KB, patch
Details | Diff | Splinter Review 
Current code under gecko/ipc/dbus does not use MOZ_ENABLE_DBUS to check.
In b2g jb, dbus no longer be available anymore by default. 
We should use proper flag to disable building RawDBusConnection.cpp, DBusUtils.cpp, DBusThread.cpp.
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
This patch introduces two build flag in configure.in to support two different Bluetooth stacks for Gecko.
Attachment #808975 - Flags: review?(echou)
Change flag to MOZ_B2G_BT_BLUEZ for static library mozdbus_s
Attachment #808977 - Flags: review?(echou)
Attachment #808976 - Attachment description: Build dbus related code when bluez is the target support stack → 0002-Build dbus related code when bluez is the target support stack
Attachment #808975 - Attachment description: Add two flags to support two different stacks → 0001-Add two flags to support two different stacks
I comment out BluetoothDroidGonkService due to the implementation need to be discussed further.
Attachment #808980 - Flags: review?(echou)
Comment on attachment 808975 [details] [diff] [review]
0001-Add two flags to support two different stacks

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

Hi Shawn,

Please pick nits I mentioned.

Since I'm not very familiar with our build system, please ask experts for another review for each patch. I think mwu could help. :)

::: configure.in
@@ +217,4 @@
>      18)
>          GONK_INCLUDES="-I$gonkdir/frameworks/native/include -I$gonkdir/frameworks/av/include -I$gonkdir/frameworks/av/include/media -I$gonkdir/frameworks/av/include/camera -I$gonkdir/frameworks/native/include/media/openmax -I$gonkdir/frameworks/av/media/libstagefright/include"
>          if test -d "$gonkdir/external/bluetooth/bluez"; then
> +            GONK_INCLUDES="$GONK_INCLUDES -I$gonkdir/external/dbus -I$gonkdir/external/bluetooth/bluez/lib"

Can't see the difference. Is there any reason for using '=' instead of '+='?

@@ +7423,5 @@
> +fi
> +AC_SUBST(MOZ_B2G_BT_BLUEZ)
> +
> +dnl ========================================================
> +dnl = Enable MOZ Bluez for B2G (Gonk usually)

"Bluedroid"
Attachment #808975 - Flags: review?(echou)
See bug 896063 comment 15, the difference is change ' GONK_INCLUDES+=' -I$gonkdir " to ="$GONK_INCLUDES. We have seen several people build break here (JB + BlueZ), so I think it's better to change it. When we worked on flatfish tablet, a few people reported this.
Attachment #809045 - Flags: review?(echou) → review?(mwu)
Comment on attachment 809045 [details] [diff] [review]
0001-Add-MOZ_B2G_BT_BLUEZ-and-MOZ_B2G_BT_BLUEDROID-flag-f.patch

Looks good to me, but switching review to glandium just in case he has some suggestions.
Attachment #809045 - Flags: review?(mwu)
Attachment #809045 - Flags: review?(mh+mozilla)
Attachment #809045 - Flags: feedback+
It's reasonable to include dbus only when BlueZ is used. I'll r+ for both patch 2 and patch 3 once patch 1 got r+ since the build flag MOZ_B2G_BT_BLUEZ is defined in patch 1.
Comment on attachment 809045 [details] [diff] [review]
0001-Add-MOZ_B2G_BT_BLUEZ-and-MOZ_B2G_BT_BLUEDROID-flag-f.patch

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

Note part 2 and 3 need to land as one patch, otherwise if someone is bisecting and ends up on part 2, build will fail.

Actually, the 4 parts really ought to be a single patch.

::: configure.in
@@ +7418,5 @@
>  dnl ========================================================
> +dnl = Enable MOZ Bluez for B2G (Gonk usually)
> +dnl ========================================================
> +if test -n "$MOZ_B2G_BT_BLUEZ"; then
> +    AC_DEFINE(MOZ_B2G_BT_BLUEZ)

Please don't use AC_DEFINEs here, considering that's only used in one directory (and even then, I'm suggesting not to use ifdefs, see upcoming comment)
Attachment #809045 - Flags: review?(mh+mozilla) → review-
Comment on attachment 808980 [details] [diff] [review]
0004-Build-dbus-bluez-related-code-if-flag-is-MOZ_B2G_BT_.patch

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +15,4 @@
>  ** See the License for the specific language governing permissions and
>  ** limitations under the License.
>  */
> +#ifdef MOZ_B2G_BT_BLUEZ

That's going to break linux builds with dbus enabled.

Anyways, in general you don't want to make c++ files empty with a #ifdef, you just want to not build them at all. So please modify moz.build instead.
Attachment #808980 - Flags: review?(echou) → review-
Attachment #808976 - Attachment is obsolete: true
Attachment #808976 - Flags: review?(echou)
Attachment #808977 - Attachment is obsolete: true
Attachment #808977 - Flags: review?(echou)
Summary: (gonk-jb)Add build flag to check it's necessary to build dbus related ipc code → (gonk-jb)Add build flag to check it's necessary to support both bluez/bluedroid stacks
Summary: (gonk-jb)Add build flag to check it's necessary to support both bluez/bluedroid stacks → (gonk-jb)Add build flag to support both bluez/bluedroid stacks
Attached patch bug880610 (obsolete) — — Splinter Review 
JB 4.3 had removed both external/bluetooth/bluez and external/dbus. This patch is trying to support both two stacks. MOZ_B2G_BT_BLUEZ and MOZ_B2G_BT_BLUEDROID are been introduced. There will be other patches depend on these two flag to separate implementation.
Attachment #816843 - Flags: feedback?(kyle)
Attachment #816843 - Attachment is patch: true
Attachment #816843 - Attachment mime type: message/rfc822 → text/plain
Comment on attachment 816843 [details] [diff] [review]
bug880610

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

Things look ok, though we can't add B2G_BT_BLUEZ under the ENABLE_DBUS block on linux. I'm adding mwu as fb here since he might have some recommendations on cleaning up configure.ac a bit more.

::: configure.in
@@ +4899,5 @@
>  
>  if test "$MOZ_ENABLE_GTK" -o "$MOZ_ENABLE_QT"
>  then
>      MOZ_ENABLE_DBUS=1
> +    MOZ_B2G_BT_BLUEZ=1

We don't want to add this here, as dbus is used for multiple things on linux systems, and we aren't activating desktop bluetooth. This will also break builds since we don't mandate bluez as part of the build.
Attachment #816843 - Flags: feedback?(kyle) → feedback?(mwu)
Kyle, thanks for your comments. You are right, this shall not be added. I will also verify it on b2g-desktop with or without enable-bt to see if anything breaks.
If "--enable-b2g-bt" is only for b2g-desktop (I am not very sure), can I make an assumption that it's bluez based? Since now only JB 4.3 gonk uses bluedroid and remove all dbus dependency. And I have to change config MOZ_B2G_BT to MOZ_B2G_BT_BLUEZ in ipc/moz.build.

MOZ_ARG_ENABLE_BOOL(b2g-bt,
[  --enable-b2g-bt      Set compile flags necessary for compiling Bluetooth API for B2G ],
    MOZ_B2G_BT=1,
    MOZ_B2G_BT= )
if test -n "$MOZ_B2G_BT"; then
    AC_DEFINE(MOZ_B2G_BT)
+   AC_DEFINE(MOZ_B2G_BT_BLUEZ)
fi
Flags: needinfo?(kyle)
This is still not correct. It seems even I added flag like Comment 15, even building b2g-desktop with enable-b2g-bt, still build breaks.
Comment 15, 16 are totally wrong. Please ignore them.
Flags: needinfo?(kyle)
Something I should've mentioned earlier today too: There's a good chance things may not compile. :)

I was pretty much the only one running bluetooth on b2g-desktop for a long time, and I had to fix tons of compile issues when things didn't get updated (there's a lot of code branches that are only exercised on desktop, so tbpl doesn't catch errors since it doesn't put bluetooth in desktop builds). Seeing I haven't run bluetooth on b2g-desktop in at least a month, that may be an issue again.
Hi Kyle,
Now I have another patch. Now I can build pass for b2g-desktop with bt and bluez flag enabled. And find another bug which is related to b2g-desktop build. See bug 926757.
But with the latest central + gaia master, my b2g-desktop cannot go to home screen, so i cannot verify function. But I'm sure b2g-desktop get compiled now.
Attached file b2g-desktop error log — 
I cannot go into home screen. So I cannot verify b2g-desktop bluetooth function.
Attachment #816843 - Attachment is obsolete: true
Attachment #816843 - Flags: feedback?(mwu)
Hi Kyle,
It does not make sense to make an assumption that b2g-desktop enable-b2g-bt also enable flag for bluez. So I add another option "enable-b2g-bt-bluez".
The patch also contains fix for build break, since it's quite trivial (just move from protected to public). Any suggestion is welcome.
Attachment #817008 - Flags: feedback?(kyle)
Flags: needinfo?(kyle)
It looks like i need to combine bug 880613 patch for UnixSocket to avoid build break in JB 4.3(+bluedroid), even though b2g-desktop and ics+bluez version is fine.
Attachment #817008 - Attachment description: 2nd version-Bug880610 → Bug 880610:Part1-Add build flag to support both bluez/bluedroid stacks
This patch add bluez flag for bluez socket. It can avoid build break while building JB 4.3+bluedroid. This patch based on Part1 bluez flag.
Attachment #817129 - Attachment description: Bug880613: Part2-Add build flag in UnixSocket to distinguish between bluez and bluedroid stack → Bug880610: Part2-Add build flag in UnixSocket to distinguish between bluez and bluedroid stack
Attachment #817129 - Attachment is obsolete: true
Attachment #817008 - Attachment description: Bug 880610:Part1-Add build flag to support both bluez/bluedroid stacks → Bug880610:Part1-Add build flag to support both bluez/bluedroid stacks
Comment on attachment 817008 [details] [diff] [review]
Bug880610:Part1-Add build flag to support both bluez/bluedroid stacks

To enable b2g-desktop bt, two ac_add_options "--enable-b2g-bt --enable-b2g-bt-bluez" are required to be added.
Comment on attachment 817008 [details] [diff] [review]
Bug880610:Part1-Add build flag to support both bluez/bluedroid stacks

Since there are two bluetooth stacks we need to support, we need to have flags to distinguish two different bluetooth stack api. It evolves dbus, socket interface, and different profiles. This is why i use AC_DEFINE here.
Attachment #817008 - Flags: review?(mh+mozilla)
Attachment #817008 - Flags: feedback?(kyle) → feedback+
Comment on attachment 817008 [details] [diff] [review]
Bug880610:Part1-Add build flag to support both bluez/bluedroid stacks

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

::: configure.in
@@ +7279,5 @@
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(b2g-bt-bluez,
> +[  --enable-b2g-bt-bluez      Set compile flags necessary for compiling Bluetooth API for B2G ],
> +    MOZ_B2G_BT_BLUEZ=1,
> +    MOZ_B2G_BT_BLUEZ= )

Is that option ever going to be used?

@@ +7285,5 @@
> +dnl ========================================================
> +dnl = Enable MOZ Bluez for B2G (Gonk usually)
> +dnl ========================================================
> +if test -n "$MOZ_B2G_BT_BLUEZ"; then
> +    AC_DEFINE(MOZ_B2G_BT_BLUEZ)

Both defines are used in exactly one cpp file. Please just define them in the corresponding makefile.

::: dom/bluetooth/BluetoothService.cpp
@@ +47,5 @@
> +#ifndef MOZ_B2G_BT_BLUEDROID
> +#include "BluetoothGonkService.h"
> +#else
> +// to enable bluedroid gonk service
> +//#include "BluetoothDroidGonkService.h"

Why keep the commented out code?

@@ +310,5 @@
>  
>  #if defined(MOZ_BLUETOOTH_GONK)
> +#ifdef MOZ_B2G_BT_BLUEDROID
> +// enable bluedroid service here
> +//  return new BluetoothDroidGonkService();

Likewise.
Attachment #817008 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #27)
> Comment on attachment 817008 [details] [diff] [review]
> Bug880610:Part1-Add build flag to support both bluez/bluedroid stacks
> 
> Review of attachment 817008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +7279,5 @@
> > +dnl ========================================================
> > +MOZ_ARG_ENABLE_BOOL(b2g-bt-bluez,
> > +[  --enable-b2g-bt-bluez      Set compile flags necessary for compiling Bluetooth API for B2G ],
> > +    MOZ_B2G_BT_BLUEZ=1,
> > +    MOZ_B2G_BT_BLUEZ= )
> 
> Is that option ever going to be used?
> 
The original idea is to provide this option for building b2g-desktop.
To enable bluetooth fucntion for b2g-desktop with build option '--enable-b2g-bt'.
Since b2g-desktop bt uses bluez stack, this is why I add this option.
> @@ +7285,5 @@
> > +dnl ========================================================
> > +dnl = Enable MOZ Bluez for B2G (Gonk usually)
> > +dnl ========================================================
> > +if test -n "$MOZ_B2G_BT_BLUEZ"; then
> > +    AC_DEFINE(MOZ_B2G_BT_BLUEZ)
> 
> Both defines are used in exactly one cpp file. Please just define them in
> the corresponding makefile.
> 
Not only one cpp file use MOZ_B2G_BT_BLUEZ, future changes may also depend on this flag.
I'm not sure I understand it correctly, would you mind elaborating more detail?

> ::: dom/bluetooth/BluetoothService.cpp
> @@ +47,5 @@
> > +#ifndef MOZ_B2G_BT_BLUEDROID
> > +#include "BluetoothGonkService.h"
> > +#else
> > +// to enable bluedroid gonk service
> > +//#include "BluetoothDroidGonkService.h"
> 
> Why keep the commented out code?
> 
> @@ +310,5 @@
> >  
> >  #if defined(MOZ_BLUETOOTH_GONK)
> > +#ifdef MOZ_B2G_BT_BLUEDROID
> > +// enable bluedroid service here
> > +//  return new BluetoothDroidGonkService();
> 
> Likewise.
I will remove comment out code.
Flags: needinfo?(mh+mozilla)
(In reply to Shawn Huang from comment #28)
> (In reply to Mike Hommey [:glandium] from comment #27)
> > Comment on attachment 817008 [details] [diff] [review]
> > Bug880610:Part1-Add build flag to support both bluez/bluedroid stacks
> > 
> > Review of attachment 817008 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: configure.in
> > @@ +7279,5 @@
> > > +dnl ========================================================
> > > +MOZ_ARG_ENABLE_BOOL(b2g-bt-bluez,
> > > +[  --enable-b2g-bt-bluez      Set compile flags necessary for compiling Bluetooth API for B2G ],
> > > +    MOZ_B2G_BT_BLUEZ=1,
> > > +    MOZ_B2G_BT_BLUEZ= )
> > 
> > Is that option ever going to be used?
> > 
> The original idea is to provide this option for building b2g-desktop.
> To enable bluetooth fucntion for b2g-desktop with build option
> '--enable-b2g-bt'.

Why isn't it the default for b2g-desktop?

> Since b2g-desktop bt uses bluez stack, this is why I add this option.
> > @@ +7285,5 @@
> > > +dnl ========================================================
> > > +dnl = Enable MOZ Bluez for B2G (Gonk usually)
> > > +dnl ========================================================
> > > +if test -n "$MOZ_B2G_BT_BLUEZ"; then
> > > +    AC_DEFINE(MOZ_B2G_BT_BLUEZ)
> > 
> > Both defines are used in exactly one cpp file. Please just define them in
> > the corresponding makefile.
> > 
> Not only one cpp file use MOZ_B2G_BT_BLUEZ, future changes may also depend
> on this flag.
> I'm not sure I understand it correctly, would you mind elaborating more
> detail?

Add something like:
ifdef MOZ_B2G_BT_BLUEZ
DEFINES += -DMOZ_B2G_BT_BLUEZ
endif

in the Makefile.in near the .cpp file that needs the define, instead of using an AC_DEFINE in configure.in.
Flags: needinfo?(mh+mozilla)
Target Milestone: --- → 1.3 Sprint 3 - 10/25
> > The original idea is to provide this option for building b2g-desktop.
> > To enable bluetooth fucntion for b2g-desktop with build option
> > '--enable-b2g-bt'.
> 
> Why isn't it the default for b2g-desktop?
Bluetooth function is not default function for b2g-desktop now.
I don't know why it's the default function for b2g-desktop, maybe qDot knows the history.
> 
> Add something like:
> ifdef MOZ_B2G_BT_BLUEZ
> DEFINES += -DMOZ_B2G_BT_BLUEZ
> endif
I did as you said to remove AC_DEFINE and AC_SUBST for both MOZ_B2G_BT_BLUEZ and MOZ_B2G_BT_BLUEDROID, but I will have a problem on moz.build

In ipc/moz.build, if i removed AC_DEFINE, dbus folder will not be included.
+if CONFIG['MOZ_B2G_BT_BLUEZ']:
     DIRS += ['dbus']

Thanks for your patience, would you please advise here?
Target Milestone: 1.3 Sprint 3 - 10/25 → ---
Flags: needinfo?(mh+mozilla)
Target Milestone: --- → 1.3 Sprint 3 - 10/25
(In reply to Shawn Huang from comment #30)
> I did as you said to remove AC_DEFINE and AC_SUBST for both MOZ_B2G_BT_BLUEZ
> and MOZ_B2G_BT_BLUEDROID, but I will have a problem on moz.build
> 
> In ipc/moz.build, if i removed AC_DEFINE, dbus folder will not be included.
> +if CONFIG['MOZ_B2G_BT_BLUEZ']:
>      DIRS += ['dbus']

I guess you also removed the AC_SUBST. You need to keep that.
Flags: needinfo?(mh+mozilla)
(In reply to Shawn Huang from comment #30)

> Bluetooth function is not default function for b2g-desktop now.
> I don't know why it's the default function for b2g-desktop, maybe qDot knows
> the history.

A couple of reasons:

- B2G desktop is expected to work on all platforms, bluez is linux only
- Even on linux, people may not have bluez/kernel headers sitting around.

For the moment, Bluetooth on B2G Desktop is considered a developers only feature, and I doubt we'll see actual WebAPI desktop bluetooth any time soon. But it's REALLY nice to have around to debug B2G. I don't see an issue with it being  "add to the mozconfig" flag similar to what it's been for the past 18 months though.
Flags: needinfo?(kyle)
Comment on attachment 817660 [details] [diff] [review]
Bug880610:Part1-Add build flag to support both bluez/bluedroid stacks

Revised patch has done:1.Remove AC_DEFINE 2.Remove comment out code
Attachment #817660 - Flags: review?(mh+mozilla)
Comment on attachment 817660 [details] [diff] [review]
Bug880610:Part1-Add build flag to support both bluez/bluedroid stacks

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

r+ with the following fixed.

::: configure.in
@@ +7281,5 @@
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(b2g-bt-bluez,
> +[  --enable-b2g-bt-bluez      Set compile flags necessary for compiling Bluetooth API for B2G ],
> +    MOZ_B2G_BT_BLUEZ=1,
> +    MOZ_B2G_BT_BLUEZ= )

So, reading your comments about this, I'd say you don't need an actual configure flag for this. If you want to enable it on a desktop build, just add "MOZ_B2G_BT_BLUEZ=1" in your mozconfig.

::: dom/bluetooth/BluetoothService.cpp
@@ +307,5 @@
>  #endif
>  
>  #if defined(MOZ_BLUETOOTH_GONK)
> +#ifdef MOZ_B2G_BT_BLUEDROID
> +#else

#ifndef
Attachment #817660 - Flags: review?(mh+mozilla) → review+
> So, reading your comments about this, I'd say you don't need an actual
> configure flag for this. If you want to enable it on a desktop build, just
> add "MOZ_B2G_BT_BLUEZ=1" in your mozconfig.
I will remove it. 

> ::: dom/bluetooth/BluetoothService.cpp
> @@ +307,5 @@
> >  #endif
> >  
> >  #if defined(MOZ_BLUETOOTH_GONK)
> > +#ifdef MOZ_B2G_BT_BLUEDROID
> > +#else
> 
> #ifndef
+#ifdef MOZ_B2G_BT_BLUEDROID
+#else
I left it blank because we have another patch set which enables basic bluedroid implementation.
And that patch will be updated after this patch
Attachment #817131 - Flags: review?(echou) → review+
Depends on: 928135
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #32)
> (In reply to Shawn Huang from comment #30)
> 
> > Bluetooth function is not default function for b2g-desktop now.
> > I don't know why it's the default function for b2g-desktop, maybe qDot knows
> > the history.
> 
> A couple of reasons:
> 
> - B2G desktop is expected to work on all platforms, bluez is linux only
> - Even on linux, people may not have bluez/kernel headers sitting around.
> 
> For the moment, Bluetooth on B2G Desktop is considered a developers only
> feature, and I doubt we'll see actual WebAPI desktop bluetooth any time
> soon. But it's REALLY nice to have around to debug B2G. I don't see an issue
> with it being  "add to the mozconfig" flag similar to what it's been for the
> past 18 months though.
Tracking on Bug 928494.
backout: https://hg.mozilla.org/integration/b2g-inbound/rev/51e1f24155fd

The patches has been backed out since this causes bug 928135. Bug 928214 should be able to solve bug 928135 this but it didn't.
Base on bug 928135, bug 880610 needs to backout first.  Carsten, can you backout first?
Flags: needinfo?(cbook)
(In reply to Eric Chou [:ericchou] [:echou] from comment #47)
> backout: https://hg.mozilla.org/integration/b2g-inbound/rev/51e1f24155fd
> 
> The patches has been backed out since this causes bug 928135. Bug 928214
> should be able to solve bug 928135 this but it didn't.

i have seen this was checked-in into b2g-inbound and so should be land also on the next merge on mozilla-central when this tree is merged to m-c
Flags: needinfo?(cbook)
Part 3 is needed for prototype implementation. It was merged from Bug 928214. Patches are good to go.
Blocks: 930719
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: