505739 - Remove XCFLAGS from default compile line to fix mingw cross-compile issues

Status: RESOLVED FIXED

Description

[Jacek Caban]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1
Build Identifier: 

In my case, -I/usr/include is passed to g++. It's added to CPPFLAGS because of AC_PATH_XTRA. We shouldn't use it in cross compiling.

Reproducible: Always

Comment 1

[Jacek Caban]

Created attachment 389937 [details] [diff] [review]
Fix

Comment 2

[Ted Mielczarek [:ted] [:ted.mielczarek]]

Comment on attachment 389937 [details] [diff] [review]
Fix

I don't think this is sufficient. Won't this break cross-compiling from one flavor of Linux to another? (Like cross-compiling from i386-linux to arm-linux.)

Comment 3

[Jacek Caban]

Created attachment 392706 [details] [diff] [review]
Fix

Thanks for review. You're right, although I think such compilation is risky because there is no guarantee that libs don't install architecture-specific include file. The attached patch checkes if host and target OSes are the same. It should work for this scenario.

Comment 5

[cls]

Comment on attachment 392706 [details] [diff] [review]
Fix

HOST_OS_ARCH & TARGET_OS aren't guaranteed to be the same even when not cross-compiling.  If you are going that route, you should just check for -z "$CROSS_COMPILE".

However, that route doesn't allow you to use --x-include=/path --x-libs=/path when cross-compiling from unix to unix, which is practically required to avoid this exact problem of using the wrong headers.

The correct thing may be to just avoid automatically adding X_CFLAGS to CPPFLAGS.  This should work fine for the -I/usr/include cases but I'm not completely certain about the other cases.

Comment 6

[Jacek Caban]

Created attachment 393173 [details] [diff] [review]
Set no_x before AC_PATH_XTRA.

I think I've found the right solution. There already is a check for no_x, but it's before no_x has any chance to be set. I've moved its setting and it fixed the problem for me.

I don't understand, why this check:
if test "$no_x" != "yes"; then
    CPPFLAGS="$CPPFLAGS $X_CFLAGS"
is done after C_PATH_XTRA and not before, but it doesn't matter for configure result.

Comment 7

[cls]

Comment on attachment 393173 [details] [diff] [review]
Set no_x before AC_PATH_XTRA.

no_x is set by the AC_PATH_X macro which is called by AC_PATH_XTRA.  no_x=yes when an X11 development environment isn't detected or --without-x or --with-x=no is specified.  So the no_x check is in the correct place.

Comment 8

[cls]

Created attachment 393257 [details] [diff] [review]
remove XCFLAGS from COMPILE_CFLAGS / COMPILE_CXXFLAGS

The more I look at the problem, the more I think that we're only fixing a symptom.  It should be perfectly valid to check for a feature without having the check adversely affect the build.  I think the real problem is that XCFLAGS is unconditionally added to the default COMPILE_FLAGS in config/config.mk.  The only bits that should require XCFLAGS are the X11-based widget & gfx bits and they should explicitly include the X11 cflags in TK_CFLAGS.

Comment 9

[Jacek Caban]

(In reply to comment #7)
> no_x is set by the AC_PATH_X macro which is called by AC_PATH_XTRA.  no_x=yes
> when an X11 development environment isn't detected or --without-x or
> --with-x=no is specified.  So the no_x check is in the correct place.

no_x is also explicitly set for Windows target, but it's done after AC_PATH_XTRA, so it's not taken into account there.

(In reply to comment #8)
> Created an attachment (id=393257) [details]
> remove XCFLAGS from COMPILE_CFLAGS / COMPILE_CXXFLAGS
> 
> The more I look at the problem, the more I think that we're only fixing a
> symptom.  It should be perfectly valid to check for a feature without having
> the check adversely affect the build.  I think the real problem is that XCFLAGS
> is unconditionally added to the default COMPILE_FLAGS in config/config.mk.  The
> only bits that should require XCFLAGS are the X11-based widget & gfx bits and
> they should explicitly include the X11 cflags in TK_CFLAGS.

Thanks for your patch. With this patch and --with-x=no cross compilation succeeded. By following your logic, X_CFLAGS shouldn't be added to CPPFLAGS as well. I've verified that it doesn't break Linux build and I don't need --with-x=no for cross compiling then. I will attach a patch.

Comment 10

[Jacek Caban]

Created attachment 393510 [details] [diff] [review]
Remove X_CFLAGS from CPPFLAGS.

Comment 11

[cls]

Comment on attachment 393257 [details] [diff] [review]
remove XCFLAGS from COMPILE_CFLAGS / COMPILE_CXXFLAGS

I'm still feeling my way around hg but don't we need NSPR owner review to check this in for CVS?

Comment 12

[Benjamin Smedberg]

Argh, I didn't see that this had NSPR bits. Ted is an NSPR peer now, I'd just ask him. And yeah, don't check the NSPR bits into mozilla-central: we only take tagged source updates from NSPR/CVS.

Comment 13

[Wan-Teh Chang]

I've always thought that XCFLAGS means "extra C flags" rather than
"C flags for X Windows".  See this MXR search for "XCFLAGS" in NSPR
and NSS:
http://mxr.mozilla.org/security/search?string=XCFLAGS

So we should be extra careful with the NSPR change in attachment
393257 [review].

Comment 14

[Wan-Teh Chang]

Also note how PSM passes C flags to NSS (not NSPR) in the XCFLAGS
make variable:
http://mxr.mozilla.org/mozilla-central/source/security/manager/Makefile.in#217

Comment 15

[Wan-Teh Chang]

Comment on attachment 393257 [details] [diff] [review]
remove XCFLAGS from COMPILE_CFLAGS / COMPILE_CXXFLAGS

review- on the NSPR part of this patch.

Comment 16

[Reed Loden [:reed] (use needinfo?)]

http://hg.mozilla.org/mozilla-central/rev/e3b289b0a31c

http://hg.mozilla.org/mozilla-central/rev/a794f12d6642

Leaving open for NSPR stuff...

Comment 17

[Hanspeter Niederstrasser]

I believe that this has broken compilation on Darwin/X11.  Today, compiling gfx/cairo/cairo/src/cairo-xlib-screen.c (and others in that dir) leads to errors like

cairo-xlib-screen.c
gcc -o cairo-xlib-screen.o -c -I../../../../dist/system_wrappers -include /src/mozilla-central/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"Darwin9.8.0\" -DOSARCH=Darwin -DPACKAGE_VERSION="\"moz\"" -DPACKAGE_BUGREPORT="\"http://bugzilla.mozilla.org/\"" -DMOZ_TREE_CAIRO -I/src/mozilla-central/gfx/cairo/cairo/src -I/src/mozilla-central/gfx/cairo/cairo/src -I. -I../../../../dist/include -I../../../../dist/include/nsprpub  -I/src/mozilla-central/obj-i386-apple-darwin9.8.0-browser/dist/include/nspr -I/src/mozilla-central/obj-i386-apple-darwin9.8.0-browser/dist/include/nss -I/sw/include   -I/sw/include/freetype2 -I/sw/include     -fPIC  -Wall -W -Wno-unused -Wpointer-arith -Wcast-align -W -Wno-long-long -fno-strict-aliasing -fpascal-strings -fno-common -pthread -pipe  -DNDEBUG -DTRIMMED -O2   -include ../../../../mozilla-config.h -DMOZILLA_CLIENT -Wp,-MD,.deps/cairo-xlib-screen.pp /src/mozilla-central/gfx/cairo/cairo/src/cairo-xlib-screen.c
In file included from /src/mozilla-central/gfx/cairo/cairo/src/cairo-xlib-screen.c:62:
../../../../dist/system_wrappers/fontconfig/fontconfig.h:3:40: error: fontconfig/fontconfig.h: No such file or directory


My .mozconfig file has --x-libraries=/usr/X11R6/lib and --x-includes=/usr/X11R6/include and this previously built mozilla-central w/out any problems.  I've temporarily set up a workaround here by adding "OS_INCLUDES += $(XCFLAGS)" ("TK_CFLAGS += $(XCFLAGS)" didn't work) to the "ifdef MOZ_X11" block in gfx/cairo/cairo/src/Makefile.in.  Is there a more appropriate place for this fix to submit as a patch?

Comment 18

[cls]

Hanspeter:
I didn't think that fontconfig/fontconfig.h was a standard X11 header.  It's installed under /usr/include/ on Fedora 10 and has its own pkg-config entry.  I'd say file a separate bug against gfx/cairo for not using the proper include paths to find that header.  They seem to do it for every other cairo variant.

wtc:
Wow, talk about a major disconnect!  The core mozilla tree has always treated XCFLAGS as X11 cflags with the expectation that extra cflags could be specified at configure time.  We can forget about the NSPR changes.

Comment 19

[Hanspeter Niederstrasser]

cls: thanks.  I've filed Bug 516029 about the fontconfig/fontconfig.h issue.

Comment 20

[Serge Gautherie (:sgautherie)]

Created attachment 428910 [details] [diff] [review]
(Fv1-CC) Copy it to comm-central (1.9.2+)
[Checkin: Comment 22]

Comment 21

[Justin Wood (:Callek)]

Comment on attachment 428910 [details] [diff] [review]
(Fv1-CC) Copy it to comm-central (1.9.2+)
[Checkin: Comment 22]

is different between 1.9.2 and m-c trunk, but I don't see anything here that would hurt on 1.9.2 by being out of sync in this instance, so r+

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 428910 [details] [diff] [review]
(Fv1-CC) Copy it to comm-central (1.9.2+)
[Checkin: Comment 22]


http://hg.mozilla.org/comm-central/rev/f65140743713