Closed Bug 966814 Opened 6 years ago Closed 5 years ago

Build with disabled X11 on linux failing to compile

Categories

(Firefox Build System :: General, defect)

ARM
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: romaxa, Assigned: gcp)

References

Details

Attachments

(1 file, 2 obsolete files)

gfx/cairo/cairo/src/cairo-ft.h:79:3: error: #error Cairo was not compiled with support for the freetype font backend

netwerk/protocol/http/nsHttpHandler.cpp:686:5: error: no matching function for call to 'nsCString::AssignLiteral()'
CAIRO_HAS_FT_FONT need to be enabled for non X11 too

Add Linux user agent for non X11 linux target
Attachment #8369292 - Flags: review?(mh+mozilla)
Comment on attachment 8369292 [details] [diff] [review]
Fix build on Non X11 Linux target

Review of attachment 8369292 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the configure.in part.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +682,5 @@
>      "Macintosh"
>  #elif defined(MOZ_X11)
>      "X11"
> +#elif defined(XP_UNIX)
> +    "Linux"

That seems wrong, considering XP_UNIX != Linux, even if for now there probably aren't XP_UNIXes without X11, but that's likely to change.

That being said, I'm not a network peer, so that would have to get through one anyways.
Attachment #8369292 - Flags: review?(mcmanus)
Attachment #8369292 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8369292 [details] [diff] [review]
Fix build on Non X11 Linux target

This is fundamentally a build change and I'm happy to delegate that to :glandium. Thanks for looping me in.
Attachment #8369292 - Flags: review?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #3)
> Comment on attachment 8369292 [details] [diff] [review]
> Fix build on Non X11 Linux target
> 
> This is fundamentally a build change and I'm happy to delegate that to
> :glandium. Thanks for looping me in.

That actually affects the UA we expose on those builds. I now realize that's not really a netwerk module thing, but more of a Content HTTP Headers module thing.

Gerv, what do you think the UA should expose instead of "X11" on non-X11 unix builds? "Unix", maybe?
Flags: needinfo?(gerv)
(In reply to Mike Hommey [:glandium] from comment #4)
> Gerv, what do you think the UA should expose instead of "X11" on non-X11
> unix builds? "Unix", maybe?

Oh, dear :-| I should have seen this coming.

I think there's a strong case for them continuing to say "X11". Reasoning: that's how "Unix" is "spelt" in most user-agent sniffers out there. No-one actually looks for "X11" because they want to distinguish machines running X from those running Wayland, or something else (do they?). What changes could one possibly want to make to one's web page for that reason? The desktop technology used should be irrelevant to the page. 

We could try some options in our compatibility testing software. lmandel: I'm a bit out of the loop. What's the current status here? What would you recommend we do about this?

Gerv
Flags: needinfo?(gerv)
So should I apply X11 for else condition or use some other string?
lmandel could you give suggestion what to use here? X11 or Unix or any other string?
Flags: needinfo?(lmandel)
Would it be ok instead of "if defined(X11)", use something like "if defined(XP_UNIX)" or "if defined(LINUX)" ?
or just set unconditional else for all other platforms and assume they are like "X11",
so it would work for some Free/OpenBSD/Unix/Linux et.c. ?
Attachment #8369292 - Attachment is obsolete: true
Attachment #8380038 - Flags: review?(lmandel)
As Gerv said, we can run some automated tests to try and understand the fallout from an UA change. However, unless we have a good reason to make an UA change (how do we expect the new token to be recognized and used?), I suggest that we keep the UA consistent to avoid issues. (This will certainly not be the first UA that lies.)
Flags: needinfo?(lmandel)
Comment on attachment 8380038 [details] [diff] [review]
Fix build on Non X11 Linux target

Review of attachment 8380038 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not the right person to review this code change. I'll needinfo Jason to find an appropriate person.
Attachment #8380038 - Flags: review?(lmandel)
Jason - Can you please find someone suitable to review the attached patch?
Flags: needinfo?(jduell.mcbugs)
Comment on attachment 8380038 [details] [diff] [review]
Fix build on Non X11 Linux target

Review of attachment 8380038 [details] [diff] [review]:
-----------------------------------------------------------------

+r on the necko bits.  Handing off to roc to find someone for the cairo piece.
Attachment #8380038 - Flags: review?(roc)
Flags: needinfo?(jduell.mcbugs)
Blocks: 1042525
Assignee: nobody → gpascutto
https://tbpl.mozilla.org/?tree=Try&rev=90432cd1c9ed

I have this problem, but looks like this patch needs some further tweaking.
This contains an additional fix for freetype which makes try happy:
https://tbpl.mozilla.org/?tree=Try&rev=78bc204a5408
Attachment #8380038 - Attachment is obsolete: true
Attachment #8462666 - Flags: review?(roc)
Comment on attachment 8462666 [details] [diff] [review]
Patch 1. Fix Non X11 build

Review of attachment 8462666 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me but why am I being asked to review this? I'm not a peer of the build system or Necko.
Attachment #8462666 - Flags: review?(roc)
Attachment #8462666 - Flags: review?(mh+mozilla)
>Looks good to me but why am I being asked to review this? I'm not a peer of the build system or Necko.

Because you r+ed the part of the patch that I changed originally.
Comment on attachment 8462666 [details] [diff] [review]
Patch 1. Fix Non X11 build

Review of attachment 8462666 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +655,5 @@
>  #elif defined(XP_WIN)
>      "Windows"
>  #elif defined(XP_MACOSX)
>      "Macintosh"
> +#elif defined(XP_UNIX)

This could probably stand a comment saying that we use X11 here for backwards-compat.
Attachment #8462666 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/b697c227effe
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [qa-]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.