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

RESOLVED FIXED in mozilla1.9.3a1

Status

RESOLVED FIXED
9 years ago
8 months ago

People

(Reporter: jacek, Assigned: jacek)

Tracking

Trunk
mozilla1.9.3a1
x86
Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

9 years ago
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
(Assignee)

Comment 1

9 years ago
Created attachment 389937 [details] [diff] [review]
Fix
Attachment #389937 - Flags: review?(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.)
(Assignee)

Comment 3

9 years ago
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.
Attachment #389937 - Attachment is obsolete: true
Attachment #392706 - Flags: review?(ted.mielczarek)
Attachment #389937 - Flags: review?(ted.mielczarek)

Updated

9 years ago
Duplicate of this bug: 478194

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

9 years ago
Attachment #392706 - Flags: review?(ted.mielczarek) → review-

Comment 5

9 years ago
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.
(Assignee)

Comment 6

9 years ago
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.
Attachment #392706 - Attachment is obsolete: true
Attachment #393173 - Flags: review?(cls)

Comment 7

9 years ago
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.
Attachment #393173 - Flags: review?(cls) → review-

Comment 8

9 years ago
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.
Attachment #393173 - Attachment is obsolete: true
Attachment #393257 - Flags: review?(benjamin)
(Assignee)

Comment 9

9 years ago
(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.
(Assignee)

Comment 10

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

Updated

9 years ago
Attachment #393510 - Flags: review+

Updated

9 years ago
Attachment #393257 - Flags: review?(benjamin) → review+

Updated

9 years ago
Keywords: checkin-needed

Updated

9 years ago
Summary: Crosscompiling on Linux with MinGW uses system includes. → Remove XCFLAGS from default compile line to fix mingw cross-compile issues
Version: unspecified → Trunk

Comment 11

9 years ago
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?
Attachment #393257 - Flags: superreview?(wtc)

Comment 12

9 years ago
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

9 years ago
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

9 years ago
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

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

review- on the NSPR part of this patch.
Attachment #393257 - Flags: superreview?(wtc) → superreview-
Assignee: nobody → jacek
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

9 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
cls: thanks.  I've filed Bug 516029 about the fontconfig/fontconfig.h issue.
Depends on: 516029
Blocks: 506493
Flags: in-testsuite-
Target Milestone: --- → mozilla1.9.3a1
Created attachment 428910 [details] [diff] [review]
(Fv1-CC) Copy it to comm-central (1.9.2+)
[Checkin: Comment 22]
Attachment #428910 - Flags: review?(bugspam.Callek)
Attachment #428910 - Attachment description: (Fv1-CC) Copy it to comm-central → (Fv1-CC) Copy it to comm-central (1.9.2+)
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+
Attachment #428910 - Flags: review?(bugspam.Callek) → review+
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
Attachment #428910 - Attachment description: (Fv1-CC) Copy it to comm-central (1.9.2+) → (Fv1-CC) Copy it to comm-central (1.9.2+) [Checkin: Comment 22]

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.