Closed
Bug 889787
Opened 12 years ago
Closed 12 years ago
Define XP_LINUX globally
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla25
People
(Reporter: Ms2ger, Assigned: mshal)
Details
Attachments
(1 file, 2 obsolete files)
5.76 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
There's 5 makefiles that do
ifeq ($(OS_ARCH),Linux)
DEFINES += -DXP_LINUX
endif
We might as well centralize that.
Comment 1•12 years ago
|
||
I approve this message.
Assignee | ||
Comment 2•12 years ago
|
||
Is this what you guys had in mind?
try results: https://tbpl.mozilla.org/?tree=Try&rev=30dba9e25d9b
Attachment #776858 -
Flags: review?(ted)
Comment 3•12 years ago
|
||
Comment on attachment 776858 [details] [diff] [review]
Bug 889787 - Define XP_LINUX globally
Review of attachment 776858 [details] [diff] [review]:
-----------------------------------------------------------------
::: config/config.mk
@@ +804,5 @@
> DEFINES += -DNO_NSPR_10_SUPPORT
>
> +ifeq ($(OS_ARCH),Linux)
> +DEFINES += -DXP_LINUX
> +endif
I would probably AC_DEFINE this in configure to match the other XP_ defines we have, like:
http://mxr.mozilla.org/mozilla-central/source/configure.in#1920
but this will work, it's just encoding that knowledge into the make backend instead of the configure frontend...
Attachment #776858 -
Flags: review?(ted) → review+
Updated•12 years ago
|
Assignee: nobody → mshal
Reporter | ||
Comment 4•12 years ago
|
||
Seems better for make and no worse for tup... Let's take it, unless our tup guy objects :)
Assignee | ||
Comment 5•12 years ago
|
||
Ahh I didn't notice the others were defined in configure. I've updated my Makefile-gepper to include that for searching :). I don't think either way is better/worse for tup, and I'd prefer to keep things consistent. This builds locally on Linux - I'll run again on try if it looks good.
Attachment #776858 -
Attachment is obsolete: true
Attachment #777162 -
Flags: review?(ted)
Comment 6•12 years ago
|
||
Comment on attachment 777162 [details] [diff] [review]
Bug 889787 - Define XP_LINUX globally
Review of attachment 777162 [details] [diff] [review]:
-----------------------------------------------------------------
::: configure.in
@@ +2038,5 @@
> MOZ_PGO_OPTIMIZE_FLAGS="-O3"
> MOZ_OPTIMIZE_FLAGS="-Os -freorder-blocks $MOZ_OPTIMIZE_SIZE_TWEAK"
> fi
>
> + AC_DEFINE(XP_LINUX)
I wonder if we should do this in js/src/configure.in as well just for consistency?
Attachment #777162 -
Flags: review?(ted) → review+
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> Comment on attachment 777162 [details] [diff] [review]
> Bug 889787 - Define XP_LINUX globally
>
> Review of attachment 777162 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: configure.in
> @@ +2038,5 @@
> > MOZ_PGO_OPTIMIZE_FLAGS="-O3"
> > MOZ_OPTIMIZE_FLAGS="-Os -freorder-blocks $MOZ_OPTIMIZE_SIZE_TWEAK"
> > fi
> >
> > + AC_DEFINE(XP_LINUX)
>
> I wonder if we should do this in js/src/configure.in as well just for
> consistency?
I'm not sure - it doesn't look like anything in js/src uses XP_LINUX, and I'm hesitant to add something that isn't actually used.
Unfortunately, the try run with AC_DEFINE(XP_LINUX) failed on Android - it has target=i386-linux-android, which gets caught by the *-android*|*-linuxandroid*) condition, so it doesn't go into the *-*linux*) branch. Instead I've added a separate case "$target" so that both will get the XP_LINUX define.
Assignee | ||
Comment 8•12 years ago
|
||
This one looks happier on try. Sorry for all the re-reviews :/
Attachment #777162 -
Attachment is obsolete: true
Attachment #777373 -
Flags: review?(ted)
Updated•12 years ago
|
Attachment #777373 -
Flags: review?(ted) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Comment 10•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•