966814 - Build with disabled X11 on linux failing to compile

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Oleg Romashin (:romaxa)]

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()'

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Fix build on Non X11 Linux target

CAIRO_HAS_FT_FONT need to be enabled for non X11 too

Add Linux user agent for non X11 linux target

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[Patrick McManus [:mcmanus]]

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.

Comment 4

[Mike Hommey [:glandium]]

(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?

Comment 5

[Gervase Markham [: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

Comment 6

[Oleg Romashin (:romaxa)]

So should I apply X11 for else condition or use some other string?

Comment 7

[Oleg Romashin (:romaxa)]

lmandel could you give suggestion what to use here? X11 or Unix or any other string?

Comment 8

[Oleg Romashin (:romaxa)]

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. ?

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Fix build on Non X11 Linux target

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

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.)

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Jason - Can you please find someone suitable to review the attached patch?

Comment 13

[Jason Duell]

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.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

https://tbpl.mozilla.org/?tree=Try&rev=90432cd1c9ed

I have this problem, but looks like this patch needs some further tweaking.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Fix Non X11 build

This contains an additional fix for freetype which makes try happy:
https://tbpl.mozilla.org/?tree=Try&rev=78bc204a5408

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 17

[Gian-Carlo Pascutto [:gcp]]

>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 18

[(not currently active) Ted Mielczarek]

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.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b697c227effe

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b697c227effe