Bug 88763 – [Qt] Add configure test for zlib and set WTF_USE_ZLIB if found

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Simon Hausmann]

The websocket code can use zlib directory for compressiond/decompression. We should enable that and unskip the corresponding layout tests


LayoutTests/http/tests/websocket/tests/hybi/compressed-control-frame.html
LayoutTests/http/tests/websocket/tests/hybi/deflate-frame-invalid-parameter.html
LayoutTests/http/tests/websocket/tests/hybi/deflate-frame-parameter.html
LayoutTests/http/tests/websocket/tests/hybi/extensions.html
LayoutTests/http/tests/websocket/tests/hybi/handshake-fail-by-extensions-header.html

[qi]

Created attachment 146861 [details]
patch

zlib should be included in qt5, so by default websocket extension should work now in qt5. I don't know how about qt4.8, so I move the test cases into qt-4.8 skipped.

[Simon Hausmann]

Comment on attachment 146861 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146861&action=review

> Tools/qmake/config.tests/libzlib/libzlib.pro:2
> +OBJECTS_DIR = obj

Don't you also need a LIBS += -lz here?

[qi]

Yes, I didn't include the lib by myself, it just work. Looks like qt5 already include it:

qt5/qtbase/src/3rdparty/zlib/

[Simon Hausmann]

(In reply to comment #3)
> Yes, I didn't include the lib by myself, it just work. Looks like qt5 already include it:
> 
> qt5/qtbase/src/3rdparty/zlib/

Right, but that's not going to work anymore after

    https://codereview.qt-project.org/#change,28141

We should not be using the zlib symbols from QtCore, we should use the system one.

[qi]

I updated my qt5, but I still didn't get that patch. And from the comments:

This commit does not in itself solve the issue of how to let Qt
libraries outside of qtbase use the same bundled zlib, but it is
a prerequisite for that.

Looks like we have to wait for a while to really get the fix.

Question: if I used LIBS += -lz, how do I know I use qt symbols or system symbols now?

[Simon Hausmann]

(In reply to comment #5)
> I updated my qt5, but I still didn't get that patch. And from the comments:
> 
> This commit does not in itself solve the issue of how to let Qt
> libraries outside of qtbase use the same bundled zlib, but it is
> a prerequisite for that.
> 
> Looks like we have to wait for a while to really get the fix.
> 
> Question: if I used LIBS += -lz, how do I know I use qt symbols or system symbols now?

On Linux (or more specifically ELF) this is only determined at load time (or when prelinking). I think we should do the right thing and use -lz because we also _do_ use the system zlib _header_ files that come with it - we don't use the headers shipped inside Qt.

Anyway, in the common case Qt should also be using the system zlib anyway instead.

[qi]

Created attachment 147078 [details]
patch2

I am just wondering if we use system symbols then we have new depend.
I have another stupid question: why -lz not -lzlib (I suppose is this one, but doesn't work)

[Simon Hausmann]

(In reply to comment #7)
> Created an attachment (id=147078) [details]
> patch2
> 
> I am just wondering if we use system symbols then we have new depend.

Yes, we do, we should and will depend on the system z library.

> I have another stupid question: why -lz not -lzlib (I suppose is this one, but doesn't work)

Because the library is called libz.so, not libzlib.so :)

[Simon Hausmann]

Comment on attachment 147078 [details]
patch2

r=me, but please set cq+ manually or land it manually and watch the bots closely.

[qi]

Manually Committed r120193: <http://trac.webkit.org/changeset/120193>

[Laszlo Gombos]

Comment on attachment 147078 [details]
patch2

Setting the patch obsolete as it is landed.