Closed Bug 543976 Opened 10 years ago Closed 10 years ago

Clean up Maemo defines

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set

Tracking

(status1.9.2 .2-fixed)

RESOLVED FIXED
mozilla1.9.3a2
Tracking Status
status1.9.2 --- .2-fixed

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(3 files, 1 obsolete file)

We are adding support for Maemo 6 and noticed that the Maemo 5 configuration is a bit of a mess.

We are proposing that there is only one define that determines if you are running MAEMO or not:

MOZ_PLATFORM_MAEMO

If set (with ac_add_options --with-maemo-version=#), it will be the version of the MAEMO that you are targeting (Currently 5 or 6).

Two other #defines will be:

MOZ_PLATFORM_MAEMO_CFLAGS
MOZ_PLATFORM_MAEMO_LIBS

These will include the common flags needed to build and link.  We think that this approach will make the maemo section of config a bit easier to understand and extend.  It will also be easier as a developer to figure out which flags you need to add.
Attached patch m-c patch (obsolete) — Splinter Review
Assignee: nobody → mozbugz
Attachment #424985 - Flags: review?
Attachment #424986 - Flags: review?(mark.finkle)
Attachment #424985 - Flags: review? → review?(ted.mielczarek)
Attachment #424986 - Flags: review?(mark.finkle) → review+
Blocks: 544208
Comment on attachment 424985 [details] [diff] [review]
m-c patch

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>+dnl = Maemo checks
>+dnl ========================================================
>+
>+MOZ_ARG_WITH_STRING(maemo-version,
>+[  --with-maemo-version=MAEMO_SDK_TARGET_VER
>+                        Maemo SDK Version],
>+  MAEMO_SDK_TARGET_VER=$withval)
>+
>+case "$MAEMO_SDK_TARGET_VER" in
>+5)
>+    MOZ_PLATFORM_MAEMO=5
>+    ;;
>+
>+6)
>+    MOZ_PLATFORM_MAEMO=6
>+    ;;
>+*)
>+    ;;
>+esac

You should probably error on unknown versions here.

>+
>+if test $MOZ_PLATFORM_MAEMO; then
>+   AC_DEFINE_UNQUOTED([MOZ_PLATFORM_MAEMO], $MAEMO_SDK_TARGET_VER)

Use the MOZ_PLATFORM_MAEMO variable you just defined instead of MAEMO_SDK_TARGET_VER.

>+
>+   if test -z "$MOZ_ENABLE_DBUS"; then
>+       AC_MSG_ERROR([DBus is required when building for Maemo])
>+   fi
>+   
>+   MOZ_GFX_OPTIMIZE_MOBILE=1
>+   MOZ_WEBGL_GLX=
>+
>+   if test $MOZ_PLATFORM_MAEMO == 5; then

I think you want one '=' here, I believe '==' is a bashism or something.

>+      dnl if we have Xcomposite we should also have Xdamage and Xfixes
>+      AC_CHECK_HEADERS([X11/extensions/Xdamage.h], [],
>+                       [AC_MSG_ERROR([Couldn't find X11/extentsions/Xdamage.h which is required for composited plugins.])])

Typo in extensions in the comment.

>+      AC_CHECK_LIB(Xcomposite, XCompositeRedirectWindow, [XCOMPOSITE_LIBS="-lXcomposite -lXdamage -lXfixes"],
>+                   [MISSING_X="$MISSING_X -lXcomposite"], $XLIBS)
>+
>+      AC_SUBST(XCOMPOSITE_LIBS)
>+
>+      PKG_CHECK_MODULES(LIBHILDONMIME,libhildonmime)
>+      MOZ_PLATFORM_MAEMO_LIBS="$MOZ_PLATFORM_MAEMO_LIBS $LIBHILDONMIME_LIBS"
>+      MOZ_PLATFORM_MAEMO_CFLAGS="$MOZ_PLATFORM_MAEMO_CFLAGS $LIBHILDONMIME_CFLAGS"
>+
>+      PKG_CHECK_MODULES(LIBOSSO,libosso)
>+      MOZ_PLATFORM_MAEMO_LIBS="$MOZ_PLATFORM_MAEMO_LIBS $LIBOSSO_LIBS"
>+      MOZ_PLATFORM_MAEMO_CFLAGS="$MOZ_PLATFORM_MAEMO_CFLAGS $LIBOSSO_CFLAGS"
>+
>+      PKG_CHECK_MODULES(LIBHILDONFM,hildon-fm-2)
>+      MOZ_PLATFORM_MAEMO_LIBS="$MOZ_PLATFORM_MAEMO_LIBS $LIBHILDONFM_LIBS"
>+      MOZ_PLATFORM_MAEMO_CFLAGS="$MOZ_PLATFORM_MAEMO_CFLAGS $LIBHILDONFM_CFLAGS"

I think you are likely to want to error here if some of these libs are not found, no? If you're missing libhildonmime or libosso things aren't going to work properly, right?

>diff --git a/content/canvas/src/Makefile.in b/content/canvas/src/Makefile.in
>--- a/content/canvas/src/Makefile.in
>+++ b/content/canvas/src/Makefile.in
>@@ -54,22 +54,22 @@ CPPSRCS	= \
> 	CanvasUtils.cpp \
> 	nsCanvasRenderingContext2D.cpp \
> 	$(NULL)
> 
> # Canvas 3D Pieces
> 
> ifdef MOZ_WEBGL
> 
>-ifeq (1_1,$(MOZ_X11)_$(NS_OSSO))
>+ifeq (1_1,$(MOZ_X11)_$(MOZ_PLATFORM_MAEMO))

These checks are broken, MOZ_PLATFORM_MAEMO is always going to be '5' or '6' as you've defined it.

r- for those few things but overall it looks pretty good.
Attachment #424985 - Flags: review?(ted.mielczarek) → review-
Attachment #424985 - Attachment is obsolete: true
Attachment #425922 - Flags: review?
Attachment #425922 - Flags: review? → review?(ted.mielczarek)
Blocks: 545084
Attachment #425922 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 425922 [details] [diff] [review]
m-c patch v.2
[Checkin: Comment 6 & 8]

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>+MOZ_ARG_WITH_STRING(maemo-version,
>+[  --with-maemo-version=MAEMO_SDK_TARGET_VER
>+                        Maemo SDK Version],
>+  MAEMO_SDK_TARGET_VER=$withval)
>+
>+case "$MAEMO_SDK_TARGET_VER" in
>+5)
>+    MOZ_PLATFORM_MAEMO=5
>+    ;;
>+
>+6)
>+    MOZ_PLATFORM_MAEMO=6
>+    ;;
>+
>+-1)
>+    dnl We aren't compiling for Maemo, move on.
>+    ;;
>+*)
>+    AC_MSG_ERROR([Unknown Maemo Version.  Try setting --with-maemo-version to something else.])

I'd probably have this say "supported versions are 5 and 6".

Looks good otherwise, r=me
Attachment #425922 - Flags: approval1.9.2.2?
Comment on attachment 425922 [details] [diff] [review]
m-c patch v.2
[Checkin: Comment 6 & 8]

a=beltzner, dougt assures me this is all IFDEFery.
Attachment #425922 - Flags: approval1.9.2.2? → approval1.9.2.2+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a90ba7fec2a1

when this clears, i will push m-b bits.
I'm seeing a couple of error messages in my Mac OS X build as a result of this (although they seem to be harmless, the build still completes):

/Users/jonathan/mozdev/mc/config/preprocessor.pl:/Users/jonathan/mozdev/mc/config/system-headers:1012: error evaluating if: invalid argument: '(MOZ_PLATFORM_MAEMO == 5)'

/Users/jonathan/mozdev/mc/js/src/config/preprocessor.pl:/Users/jonathan/mozdev/mc/js/src/config/system-headers:1012: error evaluating if: invalid argument: '(MOZ_PLATFORM_MAEMO == 5)'
it looks like it is only perl that is getting confused.  I am using 5.8.9, but do not see this problem.  Its an easy fix to this (#if defined(MOZ_PLATFORM_MAEMO)  && (MOZ_PLATFORM_MAEMO == 5)).  But I like to understand why you are seeing this and I am not.
http://hg.mozilla.org/mobile-browser/rev/e716e0c7768b

Leaving open to resolve jonathan's error.
After some experimentation, it seems that preprocessor.pl does not allow parens or spaces in the argument to #if. Removing them eliminates the error messages.

(This was seen when building for i386 on OS X 10.6; possibly preprocessor.pl is only used here in cross-compilation setups?)
Attachment #426128 - Flags: review?(mozbugz)
(In reply to comment #12)
I'm using Fedora 12, x86_64

After comment #11, I got a compile error (like bug 402892 comment #57) with "--enable-gio" in configure script.

And attachment 426128 [details] [diff] [review] fix it.
Jonathan -- thank you:

http://hg.mozilla.org/mozilla-central/rev/c96143454881
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2e21cc41bb66

Matsuura - can you update and verify this fixes the compile error?
Yes.
The compile error now fixed.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #426128 - Flags: review?(mozbugz) → review+
Depends on: 546074
Attachment #426128 - Attachment description: clean up #if arguments to eliminate preprocessor.pl error → clean up #if arguments to eliminate preprocessor.pl error [Checkin: Comment 14]
Attachment #425922 - Attachment description: m-c patch v.2 → m-c patch v.2 [Checkin: Comment 6 & 8]
Attachment #424986 - Attachment description: m-b patch → m-b patch [Checkin: Comment 11]
Target Milestone: --- → mozilla1.9.3a2
Version: unspecified → Trunk
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.