[webkit-reviews] review denied: [Bug 80398] [Qt] Add qmake config tests for JPEG and PNG library : [Attachment 130575] proposed patch, style fixed

[bugzilla-daemon at webkit.org]

Tor Arne Vestbø <vestbo at webkit.org> has denied Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 80398: [Qt] Add qmake config tests for JPEG and PNG library
https://bugs.webkit.org/show_bug.cgi?id=80398

Attachment 130575: proposed patch, style fixed
https://bugs.webkit.org/attachment.cgi?id=130575&action=review

------- Additional Comments from Tor Arne Vestbø <vestbo at webkit.org>
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