783463 - build fails with cairo-qt on non-Linux

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Jan Beich]

Attachment: Bug 783463 - libc++ and -std=gnu++11 want explicitly declared close/write.

widget/qt/nsSound.cpp:318:26: error: use of undeclared identifier 'write'
        size_t written = write(fd, audio, audio_len);
                         ^
widget/qt/nsSound.cpp:324:7: error: use of undeclared identifier 'close'
      close(fd);
      ^
2 errors generated.

Comment 2

[Jan Beich]

Attachment: Bug 783463 - Don't include <malloc.h>, it's unused.

malloc.h is implementation dependant unlike mozalloc.h.

In file included from widget/qt/nsNativeThemeQt.cpp:28:
In file included from ../../dist/system_wrappers/malloc.h:2:
/usr/include/malloc.h:3:2: error: "<malloc.h> has been replaced by <stdlib.h>"

Comment 3

[Jan Beich]

Attachment: Bug 783463 - Unbreak --enable-skia linking on Qt X11 targets.

../../gfx/skia/SkScalerContext.o: In function `SkScalerContext::getNextContext()':
gfx/skia/src/core/SkScalerContext.cpp:(.text._ZN15SkScalerContext14getNextContextEv+0x29): undefined reference to `SkFontHost::NextLogicalFont(unsigned int, unsigned int)'
../../gfx/skia/SkTypeface.o: In function `SkTypeface::UniqueID(SkTypeface const*)':
gfx/skia/src/core/SkTypeface.cpp:(.text._ZN10SkTypeface8UniqueIDEPKS_+0x1f): undefined reference to `SkFontHost::CreateTypeface(SkTypeface const*, char const*, SkTypeface::Style)'
../../gfx/skia/SkTypeface.o: In function `SkTypeface::Equal(SkTypeface const*, SkTypeface const*)':
gfx/skia/src/core/SkTypeface.cpp:(.text._ZN10SkTypeface5EqualEPKS_S1_+0x25): undefined reference to `SkFontHost::CreateTypeface(SkTypeface const*, char const*, SkTypeface::Style)'
gfx/skia/src/core/SkTypeface.cpp:(.text._ZN10SkTypeface5EqualEPKS_S1_+0x4c): undefined reference to `SkFontHost::CreateTypeface(SkTypeface const*, char const*, SkTypeface::Style)'
../../gfx/skia/SkTypeface.o: In function `SkTypeface::CreateFromName(char const*, SkTypeface::Style)':
gfx/skia/src/core/SkTypeface.cpp:(.text._ZN10SkTypeface14CreateFromNameEPKcNS_5StyleE+0xd): undefined reference to `SkFontHost::CreateTypeface(SkTypeface const*, char const*, SkTypeface::Style)'
../../gfx/skia/SkTypeface.o: In function `SkTypeface::CreateFromTypeface(SkTypeface const*, SkTypeface::Style)':
gfx/skia/src/core/SkTypeface.cpp:(.text._ZN10SkTypeface18CreateFromTypefaceEPKS_NS_5StyleE+0x7): undefined reference to `SkFontHost::CreateTypeface(SkTypeface const*, char const*, SkTypeface::Style)'
../../gfx/skia/SkTypeface.o: In function `SkTypeface::CreateFromStream(SkStream*)':
gfx/skia/src/core/SkTypeface.cpp:(.text._ZN10SkTypeface16CreateFromStreamEP8SkStream+0x1): undefined reference to `SkFontHost::CreateTypefaceFromStream(SkStream*)'
../../gfx/skia/SkTypeface.o: In function `SkTypeface::CreateFromFile(char const*)':
gfx/skia/src/core/SkTypeface.cpp:(.text._ZN10SkTypeface14CreateFromFileEPKc+0x1): undefined reference to `SkFontHost::CreateTypefaceFromFile(char const*)'
../../gfx/skia/SkTypeface.o: In function `SkTypeface::serialize(SkWStream*) const':
gfx/skia/src/core/SkTypeface.cpp:(.text._ZNK10SkTypeface9serializeEP9SkWStream+0x1): undefined reference to `SkFontHost::Serialize(SkTypeface const*, SkWStream*)'
../../gfx/skia/SkTypeface.o: In function `SkTypeface::Deserialize(SkStream*)':
gfx/skia/src/core/SkTypeface.cpp:(.text._ZN10SkTypeface11DeserializeEP8SkStream+0x1): undefined reference to `SkFontHost::Deserialize(SkStream*)'
../../gfx/skia/SkFontHost_FreeType.o: In function `ref_ft_face(unsigned int)':
gfx/skia/src/ports/SkFontHost_FreeType.cpp:(.text._ZL11ref_ft_facej+0x3a): undefined reference to `SkFontHost::OpenStream(unsigned int)'
gfx/skia/src/ports/SkFontHost_FreeType.cpp:(.text._ZL11ref_ft_facej+0x151): undefined reference to `SkFontHost::GetFileName(unsigned int, char*, unsigned long, int*)'
../../gfx/skia/SkFontHost_tables.o: In function `SkFontHost::CountTables(unsigned int)':
gfx/skia/src/ports/SkFontHost_tables.cpp:(.text._ZN10SkFontHost11CountTablesEj+0xe): undefined reference to `SkFontHost::OpenStream(unsigned int)'
../../gfx/skia/SkFontHost_tables.o: In function `SkFontHost::GetTableTags(unsigned int, unsigned int*)':
gfx/skia/src/ports/SkFontHost_tables.cpp:(.text._ZN10SkFontHost12GetTableTagsEjPj+0x11): undefined reference to `SkFontHost::OpenStream(unsigned int)'
../../gfx/skia/SkFontHost_tables.o: In function `SkFontHost::GetTableSize(unsigned int, unsigned int)':
gfx/skia/src/ports/SkFontHost_tables.cpp:(.text._ZN10SkFontHost12GetTableSizeEjj+0x11): undefined reference to `SkFontHost::OpenStream(unsigned int)'
../../gfx/skia/SkFontHost_tables.o: In function `SkFontHost::GetTableData(unsigned int, unsigned int, unsigned long, unsigned long, void*)':
gfx/skia/src/ports/SkFontHost_tables.cpp:(.text._ZN10SkFontHost12GetTableDataEjjmmPv+0x1f): undefined reference to `SkFontHost::OpenStream(unsigned int)'
/usr/local/bin/ld: libxul.so: hidden symbol `_ZN10SkFontHost15NextLogicalFontEjj' isn't defined
/usr/local/bin/ld: final link failed: Bad value

Comment 4

[Jan Beich]

Attachment: Bug 783463 - Don't include GLDefs.h before QtOpenGL headers.

This seems to be broken after bug 778519. BasicLayers.h <- ShadowLayers.h <- GLDefs.h

In file included from widget/qt/nsWindow.cpp:11:
In file included from /usr/local/include/qt4/QtOpenGL/QGLFunctions:1:
In file included from /usr/local/include/qt4/QtOpenGL/qglfunctions.h:50:
/usr/local/include/qt4/QtOpenGL/qgl.h:358:61: error: use of undeclared identifier 'GL_TEXTURE_2D'
    GLuint bindTexture(const QImage &image, GLenum target = GL_TEXTURE_2D,
                                                            ^
/usr/local/include/qt4/QtOpenGL/qgl.h:359:39: error: use of undeclared identifier 'GL_RGBA'
                       GLint format = GL_RGBA);
                                      ^
/usr/local/include/qt4/QtOpenGL/qgl.h:360:63: error: use of undeclared identifier 'GL_TEXTURE_2D'
    GLuint bindTexture(const QPixmap &pixmap, GLenum target = GL_TEXTURE_2D,
                                                              ^
/usr/local/include/qt4/QtOpenGL/qgl.h:361:39: error: use of undeclared identifier 'GL_RGBA'
                       GLint format = GL_RGBA);
                                      ^

Comment 5

[Jan Beich]

Comment on attachment 652682 [details] [diff] [review]
Bug 783463 - Don't include GLDefs.h before QtOpenGL headers.

Check how it affects GLdouble_defined workaround from bug 778024. Perhaps, it's no longer needed.

Comment 6

[Oleg Romashin (:romaxa)]

Comment on attachment 652682 [details] [diff] [review]
Bug 783463 - Don't include GLDefs.h before QtOpenGL headers.

check http://hg.mozilla.org/mozilla-central/rev/da1a2c7ecb4d
I think Bug 779726 must fix that problem completely.

Comment 7

[Jan Beich]

Comment on attachment 652682 [details] [diff] [review]
Bug 783463 - Don't include GLDefs.h before QtOpenGL headers.

(In reply to Oleg Romashin (:romaxa) from comment #6)
> I think Bug 779726 must fix that problem completely.

Tested, it does fix.

Comment 8

[Oleg Romashin (:romaxa)]

Comment on attachment 652680 [details] [diff] [review]
Bug 783463 - Unbreak --enable-skia linking on Qt X11 targets.

I would review this patch if you remove SkFontHost_tables.cpp which actually duplicated here :(

Comment 9

[Jan Beich]

Attachment: Bug 783463 - Unbreak --enable-skia linking on Qt X11 targets.

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 652792 [details] [diff] [review]
Bug 783463 - Unbreak --enable-skia linking on Qt X11 targets.


>-ifeq (Linux,$(OS_TARGET))
>+ifneq (,$(MOZ_X11)$(filter Linux,$(OS_TARGET)))

this condition looks hard to read...
make it simple like:
ifneq (,$(filter FreeBSD Linux,$(OS_TARGET)))

make sure it works for you

Comment 11

[Jan Beich]

(In reply to Oleg Romashin (:romaxa) from comment #10)
> >-ifeq (Linux,$(OS_TARGET))
> >+ifneq (,$(MOZ_X11)$(filter Linux,$(OS_TARGET)))
> 
> this condition looks hard to read...
> make it simple like:
> ifneq (,$(filter FreeBSD Linux,$(OS_TARGET)))

Do fonts handled differently on various Unix flavors that X11 exists?
I'll try to list some:

  # how to specify Darwin/X11 ?
  ifneq (,$(filter DragonFly FreeBSD NetBSD OpenBSD Linux GNU GNU_% SunOS,$(OS_TARGET)))

So, pretty much everwhere cairo-gtk2-x11 exists and it probably misses some. While rendering scalable fonts without X11 probably works only on Linux (directfb?).

Comment 12

[Oleg Romashin (:romaxa)]

>   # how to specify Darwin/X11 ?
>   ifneq (,$(filter DragonFly FreeBSD NetBSD OpenBSD Linux GNU GNU_%
> SunOS,$(OS_TARGET)))

Are you able to test on all these targets? does Qt work there?

Comment 13

[George Wright (:gw280) (needinfo me!)]

What exactly are you trying to do with this conditional?

SkFontHost_linux is a bit of a hack to be honest; we shouldn't be using it and I'm working on fixing that. Basically it has hardcoded font paths in it and if those font paths aren't present on your platform then it's effectively a null fonthost.

Comment 14

[Jan Beich]

(In reply to George Wright (:gw280) from comment #13)
> What exactly are you trying to do with this conditional?

Make testing skia on cairo-qt a bit easier. Are you trying to throttle feedback until its more polished/finished? If so I'll tag as checkin-needed to at least get non-skia patches in.

Comment 15

[George Wright (:gw280) (needinfo me!)]

(In reply to Jan Beich from comment #14)
> (In reply to George Wright (:gw280) from comment #13)
> > What exactly are you trying to do with this conditional?
> 
> Make testing skia on cairo-qt a bit easier. Are you trying to throttle
> feedback until its more polished/finished? If so I'll tag as checkin-needed
> to at least get non-skia patches in.

No, I mean, what exactly is that conditional supposed to do? It seems to me that it says:

if we're X11 or any platform except Linux, then build SkFontHost_linux. I'm not sure what you're trying to achieve by doing that? Will this result in SkFontHost_linux being built on a Win32 platform with Qt?

Comment 16

[Jan Beich]

(In reply to George Wright (:gw280) from comment #15)
> what exactly is that conditional supposed to do?

If either MOZ_X11 is defined or OS_TARGET contains Linux word then add more files to CPPSRCS. Notice that ifneq checks against empty string. MOZ_X11 is empty only when it's undefined, filter clause is empty when it doesn't match any word.

You can often find smth like the following in the tree

  ifneq (,$(JS_SHARED_LIBRARY)$(MOZ_NATIVE_ZLIB))

or

  ifneq (,$(filter Linux FreeBSD,$(OS_TARGET)))

I've just expanded the idea to use both ways together.

Comment 17

[George Wright (:gw280) (needinfo me!)]

D'oh, I got the semantics confused with filter-out..

I'm not 100% happy though that we'd have to build "SkFontHost_linux" on non-Linux platforms, but that's an issue for upstream to deal with. For now this will probably do (but please run it through all the trybots first) but I'll keep this in mind for when I land my new Skia font code.

Comment 18

[Oleg Romashin (:romaxa)]

this is under Qt widget backend ifdef, I tested on desktop Qt uild and it seems to builds fine.
it should not hurt other platforms anyway

Comment 19

[Jan Beich]

Comment on attachment 652792 [details] [diff] [review]
Bug 783463 - Unbreak --enable-skia linking on Qt X11 targets.

Let's see if a patch with both r+ and r- can be pushed instead of stalling due to lack of testing.

Comment 20

[Oleg Romashin (:romaxa)]

IIRC last concern from gw280 about this patch was to make it in two lines condition at least.
like:
if[ned] (MOZ_X11)
if[ned] (Linux)
endif
endif
that way it more readable

Comment 21

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 652792 [details] [diff] [review]
Bug 783463 - Unbreak --enable-skia linking on Qt X11 targets.

You should address romaxa's concerns. I can't override an r- (nor should I be able to).

On my side, I'm happy with the patch so long as the conditional becomes more readable. Either by way of a comment or (preferably) by splitting up that conditional.

Comment 22

[Jan Beich]

(In reply to Oleg Romashin (:romaxa) from comment #20)
> IIRC last concern from gw280 about this patch was to make it in two lines
> condition at least.
> like:
> if[ned] (MOZ_X11)
> if[ned] (Linux)
> endif
> endif
> that way it more readable

Is there || for conditional in gmake parlance? The following example is ugly.

  ifneq (,$(filter Linux,$(OS_TARGET)))
  CPPSRCS += \
	  SkFontHost_linux.cpp \
	  SkTime_Unix.cpp \
	  $(NULL)
  else
  ifneq (,$(MOZ_X11))
  CPPSRCS += \
	  SkFontHost_linux.cpp \
	  SkTime_Unix.cpp \
	  $(NULL)
  endif
  endif

Comment 23

[Jan Beich]

(In reply to George Wright (:gw280) from comment #21)
> Comment on attachment 652792 [details] [diff] [review]
> Bug 783463 - Unbreak --enable-skia linking on Qt X11 targets.
> 
> You should address romaxa's concerns. I can't override an r- (nor should I
> be able to).

I do not agree with them. Bug 779847 taught me how confusing ifeq else ifeq else ifeq else ... endif endif endif can become. Proceeding without the patch.

Comment 24

[Joe Drew (not getting mail)]

Jan, I understand that you have the best of intentions, but at Mozilla we have an established process for how code gets checked in, and you can't try to short-circuit the process by ignoring it.

If you believe you're right, please try to convince your reviewers. If you in fact *are* right, you'll prevail!

Comment 25

[Jan Beich]

(In reply to Joe Drew (:JOEDREW!) from comment #24)
> Jan, I understand that you have the best of intentions, but at Mozilla we
> have an established process for how code gets checked in, and you can't try
> to short-circuit the process by ignoring it.

Doh, I need to use whiteboard to checkin only specific patches.

> 
> If you believe you're right, please try to convince your reviewers. If you
> in fact *are* right, you'll prevail!

No, those concerns are valid but do not disqualify my intention to make testing cairo-qt with skia easier.

Comment 26

[Oleg Romashin (:romaxa)]

My concern is that MOZ_X11 - is not quite right condition here to enabled Sk*Linux/Unix compilation.
1) Because MOZ_X11 not exactly defining linux, 
2) Sk*Linux/Unix files which you covering with MOZ_X11 condition does not have even X11 includes so nothing common with X11 stuff there.

IIUC you are trying to make these files compiled for non Linux but FreeBSD target... so logical fix here would be just make simple || filter which looks like 
ifneq (,$(filter FreeBSD Linux,$(OS_TARGET)))
And at the same time it does not make sense to add other TARGETS in the filter just because they could work... I don't know anything about those targets and if you don't have it in hands and not able to test them in place, then don't add these targets... add the only target which you care about.

Comment 27

[George Wright (:gw280) (needinfo me!)]

(In reply to Jan Beich from comment #22)
> Is there || for conditional in gmake parlance? The following example is ugly.

Maybe this does what you want it to do: http://www.gnu.org/software/make/manual/html_node/Conditional-Functions.html

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=e0fee2e553ca

https://hg.mozilla.org/integration/mozilla-inbound/rev/93d2b46f35f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4f89f8c451

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/93d2b46f35f1
https://hg.mozilla.org/mozilla-central/rev/8e4f89f8c451

Comment 30

[Jan Beich]

The code changed quite a bit, so attachment 652792 [details] [diff] [review] no longer applies. Skia still fails to build but now due to bug 974272 comment 20.