Bug 80398

Created attachment 130572 [details] proposed patch It's my first contribution to config.test, it might need improved. :-)

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.

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.

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.

Created attachment 130575 [details] proposed patch, style fixed

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.

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

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

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

Created attachment 130578 [details] proposed patch Thanks for the review! I modified the patch.

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.

Comment on attachment 130578 [details] proposed patch great, thanks!

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

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.

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

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