Bug 80398 – [Qt] Add qmake config tests for JPEG and PNG library

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zoltan Horvath]

Continue from:
https://bugs.webkit.org/show_bug.cgi?id=32410#c23

(In reply to comment #23)
> (From update of attachment 128446 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128446&action=review
> 
> r=me.
> 
> > Source/WebCore/WebCore.pri:214
> > +    LIBS += -ljpeg -lpng12
> 
> It would be nice to replace the direct linkage with qmake config tests that print out a message or abort. That can be done in a follow-up patch of course :)

[Zoltan Horvath]

Created attachment 130572 [details]
proposed patch

It's my first contribution to config.test, it might need improved. :-)

[WebKit Review Bot]

Attachment 130572 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Tools/qmake/config.tests/jpeg/jpeg.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/qmake/config.tests/png/png.cpp:30:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Horvath]

Created attachment 130573 [details]
proposed patch, style fixed

NULLs of PNG fixed.
In case of JPEG I have to use this include order otherwise it won't work.

[WebKit Review Bot]

Attachment 130573 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Tools/qmake/config.tests/jpeg/jpeg.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Tools/qmake/config.tests/png/png.cpp:30:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zoltan Horvath]

Created attachment 130575 [details]
proposed patch, style fixed

[WebKit Review Bot]

Attachment 130575 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Tools/qmake/config.tests/jpeg/jpeg.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tor Arne Vestbø]

Comment on attachment 130575 [details]
proposed patch, style fixed

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

> Source/WebCore/WebCore.pri:221
> +    haveQt(5):!contains(config_test_jpeg, yes) error("JPEG library not found!")
> +    haveQt(5):!contains(config_test_png, yes) error("PNG 1.2 library not found!")

do one scope for Qt5, eg:

haveQt(5) {
    # Qt5 allows us to use config tests to check for the presence of these libraries
    !contains(config_test_libjpeg, yes): error("JPEG library not found!")
    !contains(config_test_libpng yes): error("PNG 1.2 library not found!")
}

also note the colon after each condition (except when using { )

Ideally we'd have these hard dependencies somewhere that gets run up-front, so that you don't have to build all of WTF and JSC until finding out you're missing dependencies, but let's deal with that later (we still want to allow people to build JSC only, etc).

> Tools/qmake/config.tests/jpeg/jpeg.pro:11
> +test.commands = test -f $$OBJECTS_DIR/jpeg.o
> +test.depends = $$OBJECTS_DIR/jpeg.o
> +QMAKE_EXTRA_TARGETS += test
> +
> +default.target = all
> +default.depends += test
> +QMAKE_EXTRA_TARGETS += default

You don't need this part. qmake + make will produce an executable named "jpeg" if everything is fine, so the test will pass

> Tools/qmake/config.tests/png/png.pro:11
> +test.commands = test -f $$OBJECTS_DIR/png.o
> +test.depends = $$OBJECTS_DIR/png.o
> +QMAKE_EXTRA_TARGETS += test
> +
> +default.target = all
> +default.depends += test
> +QMAKE_EXTRA_TARGETS += default

Same with this, not needed

> Tools/qmake/sync.profile:6
> +    png => {},
> +    jpeg => {},

i prefer they were named libpng and libjpg

[Tor Arne Vestbø]

(In reply to comment #7)
> Ideally we'd have these hard dependencies somewhere that gets run up-front, so that you don't have to build all of WTF and JSC until finding out you're missing dependencies, but let's deal with that later (we still want to allow people to build JSC only, etc).

Actually strike that, we'll run make qmake up front, so we'll catch these before builing anything (though we generate derived sources).

[Tor Arne Vestbø]

(In reply to comment #6)
> i prefer they were named libpng and libjpg

sorry, libjpeg

[Zoltan Horvath]

Created attachment 130578 [details]
proposed patch

Thanks for the review!

I modified the patch.

[WebKit Review Bot]

Attachment 130578 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Tools/qmake/config.tests/libjpeg/libjpeg.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tor Arne Vestbø]

Comment on attachment 130578 [details]
proposed patch

great, thanks!

[WebKit Review Bot]

Comment on attachment 130578 [details]
proposed patch

Rejecting attachment 130578 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11838919

[Tor Arne Vestbø]

Comment on attachment 130578 [details]
proposed patch

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

> Source/WebCore/ChangeLog:10
> +

Ugh, you need a Reviewed by NOBODY (OOPS!). line here to please the commit queue.

>> Tools/qmake/config.tests/libjpeg/libjpeg.cpp:28
>> +#include <jpeglib.h>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

You can probably fix this while you're at it.

[Zoltan Horvath]

(In reply to comment #14)
> (From update of attachment 130578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130578&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +
> 
> Ugh, you need a Reviewed by NOBODY (OOPS!). line here to please the commit queue.

Ups. My excessive use of vim's dd :) I'm going to land by hand and add the line.

> >> Tools/qmake/config.tests/libjpeg/libjpeg.cpp:28
> >> +#include <jpeglib.h>
> > 
> > Alphabetical sorting problem.  [build/include_order] [4]
> 
> You can probably fix this while you're at it.

stdio.h have to go before libjpeg.h because libjpeg.h needs size_t and FILE declarations. I removed stdlib.h because it's not necessary.

[Zoltan Horvath]

Landed in http://trac.webkit.org/changeset/110045.