42947 2010-07-25 11:01:28 -0700 run-webkit-tests should check first for $WEBKIT_TESTFONTS is set 2010-07-28 05:20:18 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC Linux RESOLVED FIXED Qt P2 Normal --- 1 maheshk maheshk commit-queue dumi hausmann kling laszlo.gombos tonikitoo yael oldest_to_newest 255823 0 maheshk 2010-07-25 11:01:28 -0700 run-webkit-tests shows crashed for every test run on env where $WEBKIT_TESTFONTS is not set or not set to right font directory. Scripts should show error in this case and terminate 255824 1 62526 maheshk 2010-07-25 11:42:34 -0700 Created attachment 62526 patch Before running layoutTest check for WEBKIT_TESTFONTS set and exists 255861 2 62526 darin 2010-07-25 18:25:13 -0700 Comment on attachment 62526 patch Change seems fine. ChangeLog has a tab in it, so commit-queue would reject it. 255892 3 62540 maheshk 2010-07-25 21:11:33 -0700 Created attachment 62540 removed tab removed ChangeLog tabs for commit scripts to work 255904 4 62540 commit-queue 2010-07-25 22:46:28 -0700 Comment on attachment 62540 removed tab Rejecting patch 62540 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1 Last 500 characters of output: 't find file to patch at input line 5 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |Index: Scripts/old-run-webkit-tests |=================================================================== |--- Scripts/old-run-webkit-tests (revision 64021) |+++ Scripts/old-run-webkit-tests (working copy) -------------------------- No file to patch. Skipping patch. 2 out of 2 hunks ignored patching file ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Full output: http://queues.webkit.org/results/3593499 256525 5 62526 eric 2010-07-27 05:04:09 -0700 Comment on attachment 62526 patch Cleared Darin Adler's review+ from obsolete attachment 62526 so that this bug does not appear in http://webkit.org/pending-commit. 256535 6 kling 2010-07-27 06:01:30 -0700 Committed r64123: <http://trac.webkit.org/changeset/64123> 256761 7 62540 eric 2010-07-27 12:42:50 -0700 Comment on attachment 62540 removed tab Seems it would be better to just check out the fonts for them and store them somewhere. Like WebKitLibraries or the build directory or whatever. 256765 8 dumi 2010-07-27 12:47:30 -0700 i don't think webkit's cygwin distribution comes with git (i don't have it), so i think we should either add it there, or at the very least change the error message on windows and tell users how to get the fonts without git. also, out of curiosity, why did it become a problem now? were new tests added? and if those fonts are indeed needed by the gtk and win ports, then maybe they should be moved to a less qt-specific location? 256779 9 62540 eric 2010-07-27 13:15:48 -0700 Comment on attachment 62540 removed tab Is isCygwin() right? Won't that error for Apple's Win port? Maybe it's right for it to do so? If we don't have git, we could fail to auto-pull the fonts, but if we do, we should just pull them. I also suspect it should be possible to pull them w/o git. :) 257035 10 maheshk 2010-07-27 22:30:30 -0700 (In reply to comment #8) > i don't think webkit's cygwin distribution comes with git (i don't have it), so i think we should either add it there, or at the very least change the error message on windows and tell users how to get the fonts without git. > The total size of fonts used are 19.9MB. Would it be overkill to just add those to webkit svn repository? GIT is not mandatory. One could download the zip file from the location though. > also, out of curiosity, why did it become a problem now? were new tests added? and if those fonts are indeed needed by the gtk and win ports, then maybe they should be moved to a less qt-specific location? No new tests were added. When WEBKIT_TESTFONTS was wrong or not set "run-webkit-tests" shows crashed for every test case. This would mislead without any error message. 257036 11 maheshk 2010-07-27 22:32:29 -0700 (In reply to comment #9) > (From update of attachment 62540 [details]) > Is isCygwin() right? Won't that error for Apple's Win port? Maybe it's right for it to do so? > Apple's win port also uses testfont for layout testing. > If we don't have git, we could fail to auto-pull the fonts, but if we do, we should just pull them. I also suspect it should be possible to pull them w/o git. :) I agree with your comment #7 suggestion, having it those fonts in trunk would make sense. As long as check-out of 20mb more is a problem? 257037 12 dumi 2010-07-27 22:33:17 -0700 > No new tests were added. When WEBKIT_TESTFONTS was wrong or not set "run-webkit-tests" shows crashed for every test case. This would mislead without any error message. i don't think this is true. the win and gtk bots were happy to run all tests without these fonts. 257045 13 maheshk 2010-07-27 23:40:41 -0700 (In reply to comment #12) > > No new tests were added. When WEBKIT_TESTFONTS was wrong or not set "run-webkit-tests" shows crashed for every test case. This would mislead without any error message. > > i don't think this is true. the win and gtk bots were happy to run all tests without these fonts. Actually if you check the patch, elsif (isGtk()) { $platform = "gtk"; - if (!$ENV{"WEBKIT_TESTFONTS"}) { - print "The WEBKIT_TESTFONTS environment variable is not defined.\n"; - print "You must set it before running the tests.\n"; - print "Use git to grab the actual fonts from http://gitorious.org/qtwebkit/testfonts\n"; - exit 1; - } GTK always had this check. This patch just moves it to QT and win port as well. On #webkit IRC, I came to know apple's win port uses this env variable so isCygwin() also added to the check 257117 14 tonikitoo 2010-07-28 05:20:18 -0700 > Actually if you check the patch, > > elsif (isGtk()) { > $platform = "gtk"; > - if (!$ENV{"WEBKIT_TESTFONTS"}) { > - print "The WEBKIT_TESTFONTS environment variable is not defined.\n"; > - print "You must set it before running the tests.\n"; > - print "Use git to grab the actual fonts from http://gitorious.org/qtwebkit/testfonts\n"; > - exit 1; > - } > > GTK always had this check. This patch just moves it to QT and win port as well. > On #webkit IRC, I came to know apple's win port uses this env variable so isCygwin() also added to the check Not sure about cygwin, but for Qt is it a valuable check, yes. 62526 2010-07-25 11:42:34 -0700 2010-07-27 05:04:09 -0700 patch layoutScript.patch text/plain 1856 maheshk SW5kZXg6IFNjcmlwdHMvb2xkLXJ1bi13ZWJraXQtdGVzdHMKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU2NyaXB0 cy9vbGQtcnVuLXdlYmtpdC10ZXN0cwkocmV2aXNpb24gNjQwMjEpCisrKyBTY3JpcHRzL29sZC1y dW4td2Via2l0LXRlc3RzCSh3b3JraW5nIGNvcHkpCkBAIC0yMTQsMTIgKzIxNCw2IEBACiAgICAg fQogfSBlbHNpZiAoaXNHdGsoKSkgewogICAgICRwbGF0Zm9ybSA9ICJndGsiOwotICAgIGlmICgh JEVOVnsiV0VCS0lUX1RFU1RGT05UUyJ9KSB7Ci0gICAgICAgIHByaW50ICJUaGUgV0VCS0lUX1RF U1RGT05UUyBlbnZpcm9ubWVudCB2YXJpYWJsZSBpcyBub3QgZGVmaW5lZC5cbiI7Ci0gICAgICAg IHByaW50ICJZb3UgbXVzdCBzZXQgaXQgYmVmb3JlIHJ1bm5pbmcgdGhlIHRlc3RzLlxuIjsKLSAg ICAgICAgcHJpbnQgIlVzZSBnaXQgdG8gZ3JhYiB0aGUgYWN0dWFsIGZvbnRzIGZyb20gaHR0cDov L2dpdG9yaW91cy5vcmcvcXR3ZWJraXQvdGVzdGZvbnRzXG4iOwotICAgICAgICBleGl0IDE7Ci0g ICAgfQogfSBlbHNpZiAoaXNXeCgpKSB7CiAgICAgJHBsYXRmb3JtID0gInd4IjsKIH0gZWxzaWYg KGlzQ3lnd2luKCkpIHsKQEAgLTIzNCw2ICsyMjgsMTYgQEAKICAgICB9CiB9CiAKK2lmIChpc1F0 KCkgfHwgaXNHdGsoKSB8fCBpc0N5Z3dpbigpKSB7CisgICAgbXkgJHRlc3Rmb250UGF0aCA9ICRF TlZ7IldFQktJVF9URVNURk9OVFMifTsKKyAgICBpZiAoISR0ZXN0Zm9udFBhdGggfHwgIS1kICIk dGVzdGZvbnRQYXRoIikgeworICAgICAgICBwcmludCAiVGhlIFdFQktJVF9URVNURk9OVFMgZW52 aXJvbm1lbnQgdmFyaWFibGUgaXMgbm90IGRlZmluZWQgb3Igbm90IHNldCBwcm9wZXJseVxuIjsK KyAgICAgICAgcHJpbnQgIllvdSBtdXN0IHNldCBpdCBiZWZvcmUgcnVubmluZyB0aGUgdGVzdHMu XG4iOworICAgICAgICBwcmludCAiVXNlIGdpdCB0byBncmFiIHRoZSBhY3R1YWwgZm9udHMgZnJv bSBodHRwOi8vZ2l0b3Jpb3VzLm9yZy9xdHdlYmtpdC90ZXN0Zm9udHNcbiI7CisgICAgICAgIGV4 aXQgMTsKKyAgICB9Cit9CisKIGlmICghZGVmaW5lZCgkcGxhdGZvcm0pKSB7CiAgICAgcHJpbnQg IldBUk5JTkc6IFlvdXIgcGxhdGZvcm0gaXMgbm90IHJlY29nbml6ZWQuIEFueSBwbGF0Zm9ybS1z cGVjaWZpYyByZXN1bHRzIHdpbGwgYmUgZ2VuZXJhdGVkIGluIHBsYXRmb3JtL3VuZGVmaW5lZC5c biI7CiAgICAgJHBsYXRmb3JtID0gInVuZGVmaW5lZCI7CkluZGV4OiBDaGFuZ2VMb2cKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gQ2hhbmdlTG9nCShyZXZpc2lvbiA2NDAyMSkKKysrIENoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDEwLTA3LTI1ICBNYWhlc2ggS3Vsa2FybmkgIDxt YWhlc2gua3Vsa2FybmlAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD00Mjk0NworCUNoZWNrIGZvciBXRUJLSVRfVEVTVEZPTlRTIGZvciBxdCwgZ3RrIGFuZCB3aW5k b3dzIHBvcnQgYW5kIHRocm93IAorICAgICAgICBlcnJvci4gV2l0aG91dCB3aGljaCBkdW1wUmVu ZGVyVHJlZSBjcmFzaGVzLgorCisgICAgICAgICogU2NyaXB0cy9vbGQtcnVuLXdlYmtpdC10ZXN0 czoKKwogMjAxMC0wNy0yNSAgQWxleGV5IFByb3NrdXJ5YWtvdiAgPGFwQGFwcGxlLmNvbT4KIAog ICAgICAgICBSZXZpZXdlZCBieSBTYW0gV2VpbmlnLgo= 62540 2010-07-25 21:11:33 -0700 2010-07-27 13:15:48 -0700 removed tab layoutScript.patch text/plain 1863 maheshk SW5kZXg6IFNjcmlwdHMvb2xkLXJ1bi13ZWJraXQtdGVzdHMKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU2NyaXB0 cy9vbGQtcnVuLXdlYmtpdC10ZXN0cwkocmV2aXNpb24gNjQwMjEpCisrKyBTY3JpcHRzL29sZC1y dW4td2Via2l0LXRlc3RzCSh3b3JraW5nIGNvcHkpCkBAIC0yMTQsMTIgKzIxNCw2IEBACiAgICAg fQogfSBlbHNpZiAoaXNHdGsoKSkgewogICAgICRwbGF0Zm9ybSA9ICJndGsiOwotICAgIGlmICgh JEVOVnsiV0VCS0lUX1RFU1RGT05UUyJ9KSB7Ci0gICAgICAgIHByaW50ICJUaGUgV0VCS0lUX1RF U1RGT05UUyBlbnZpcm9ubWVudCB2YXJpYWJsZSBpcyBub3QgZGVmaW5lZC5cbiI7Ci0gICAgICAg IHByaW50ICJZb3UgbXVzdCBzZXQgaXQgYmVmb3JlIHJ1bm5pbmcgdGhlIHRlc3RzLlxuIjsKLSAg ICAgICAgcHJpbnQgIlVzZSBnaXQgdG8gZ3JhYiB0aGUgYWN0dWFsIGZvbnRzIGZyb20gaHR0cDov L2dpdG9yaW91cy5vcmcvcXR3ZWJraXQvdGVzdGZvbnRzXG4iOwotICAgICAgICBleGl0IDE7Ci0g ICAgfQogfSBlbHNpZiAoaXNXeCgpKSB7CiAgICAgJHBsYXRmb3JtID0gInd4IjsKIH0gZWxzaWYg KGlzQ3lnd2luKCkpIHsKQEAgLTIzNCw2ICsyMjgsMTYgQEAKICAgICB9CiB9CiAKK2lmIChpc1F0 KCkgfHwgaXNHdGsoKSB8fCBpc0N5Z3dpbigpKSB7CisgICAgbXkgJHRlc3Rmb250UGF0aCA9ICRF TlZ7IldFQktJVF9URVNURk9OVFMifTsKKyAgICBpZiAoISR0ZXN0Zm9udFBhdGggfHwgIS1kICIk dGVzdGZvbnRQYXRoIikgeworICAgICAgICBwcmludCAiVGhlIFdFQktJVF9URVNURk9OVFMgZW52 aXJvbm1lbnQgdmFyaWFibGUgaXMgbm90IGRlZmluZWQgb3Igbm90IHNldCBwcm9wZXJseVxuIjsK KyAgICAgICAgcHJpbnQgIllvdSBtdXN0IHNldCBpdCBiZWZvcmUgcnVubmluZyB0aGUgdGVzdHMu XG4iOworICAgICAgICBwcmludCAiVXNlIGdpdCB0byBncmFiIHRoZSBhY3R1YWwgZm9udHMgZnJv bSBodHRwOi8vZ2l0b3Jpb3VzLm9yZy9xdHdlYmtpdC90ZXN0Zm9udHNcbiI7CisgICAgICAgIGV4 aXQgMTsKKyAgICB9Cit9CisKIGlmICghZGVmaW5lZCgkcGxhdGZvcm0pKSB7CiAgICAgcHJpbnQg IldBUk5JTkc6IFlvdXIgcGxhdGZvcm0gaXMgbm90IHJlY29nbml6ZWQuIEFueSBwbGF0Zm9ybS1z cGVjaWZpYyByZXN1bHRzIHdpbGwgYmUgZ2VuZXJhdGVkIGluIHBsYXRmb3JtL3VuZGVmaW5lZC5c biI7CiAgICAgJHBsYXRmb3JtID0gInVuZGVmaW5lZCI7CkluZGV4OiBDaGFuZ2VMb2cKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gQ2hhbmdlTG9nCShyZXZpc2lvbiA2NDAyMSkKKysrIENoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDEwLTA3LTI1ICBNYWhlc2ggS3Vsa2FybmkgIDxt YWhlc2gua3Vsa2FybmlAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD00Mjk0NworICAgICAgICBDaGVjayBmb3IgV0VCS0lUX1RFU1RGT05UUyBmb3IgcXQsIGd0ayBh bmQgd2luZG93cyBwb3J0IGFuZCB0aHJvdyAKKyAgICAgICAgZXJyb3IuIFdpdGhvdXQgd2hpY2gg ZHVtcFJlbmRlclRyZWUgY3Jhc2hlcy4KKworICAgICAgICAqIFNjcmlwdHMvb2xkLXJ1bi13ZWJr aXQtdGVzdHM6CisKIDIwMTAtMDctMjUgIEFsZXhleSBQcm9za3VyeWFrb3YgIDxhcEBhcHBsZS5j b20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgU2FtIFdlaW5pZy4K