Closed
Bug 505739
Opened 13 years ago
Closed 13 years ago
Remove XCFLAGS from default compile line to fix mingw cross-compile issues
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: jacek, Assigned: jacek)
References
Details
Attachments
(3 files, 3 obsolete files)
3.46 KB,
patch
|
benjamin
:
review+
wtc
:
superreview-
|
Details | Diff | Splinter Review |
728 bytes,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
1.91 KB,
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Attachment #389937 -
Flags: review?(ted.mielczarek)
Comment 2•13 years ago
|
||
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•13 years ago
|
||
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)
Attachment #392706 -
Flags: review?(ted.mielczarek) → review-
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•13 years ago
|
||
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 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-
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•13 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•13 years ago
|
||
Updated•13 years ago
|
Attachment #393510 -
Flags: review+
Updated•13 years ago
|
Attachment #393257 -
Flags: review?(benjamin) → review+
Updated•13 years ago
|
Keywords: checkin-needed
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•13 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•13 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•13 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•13 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•13 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-
Updated•13 years ago
|
Assignee: nobody → jacek
Comment 16•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e3b289b0a31c http://hg.mozilla.org/mozilla-central/rev/a794f12d6642 Leaving open for NSPR stuff...
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment 17•13 years ago
|
||
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•13 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
Closed: 13 years ago
Resolution: --- → FIXED
Comment 19•13 years ago
|
||
cls: thanks. I've filed Bug 516029 about the fontconfig/fontconfig.h issue.
Updated•13 years ago
|
Comment 20•13 years ago
|
||
Attachment #428910 -
Flags: review?(bugspam.Callek)
Updated•13 years ago
|
Attachment #428910 -
Attachment description: (Fv1-CC) Copy it to comm-central → (Fv1-CC) Copy it to comm-central (1.9.2+)
Comment 21•13 years ago
|
||
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 22•13 years ago
|
||
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•4 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•