old-run-webkit-tests should use ensure-valid-python to check if it can run the websockets tests Right now the tests are manually skipped for Tiger, which is bad. Would be better to run them automatically when we can find python 2.5+ and not when we can't.
ensure-valid-python will need to have a --non-interactive option or some flag to make it not try to install if missing. Actually it should probably default to not installing if missing since it can't do so w/o a user sitting at the keyboard.
I think that this should be WONTFIX. Skipping tests is bad - if we require developers to make the extra effort of installing custom python, this should be mandatory.
It's just like how we dynamically skip tests on Windows when your machine isn't supported for hardware compositing. This came up because the Tiger bot currently has Python 2.3, and the resolution to that was to skip all the websocket tests (which is bad). I'd rather have the skip only happen if someone is missing the proper python. We can display a huge warning if you like. I'm happy to close this as WONTFIX, as it means less work for me. But I think automatically skipping with a big warning is a better behavior than our current manual skip or than making it impossible to run test at all w/o python 2.5.
> It's just like how we dynamically skip tests on Windows when your machine > isn't supported for hardware compositing. Yes, but Tiger support WebSocket just fine. It's purely a tools issue, unlike the compositing one.
Created attachment 56219 [details] Patch
Comment on attachment 56219 [details] Patch Hi Alexey, this patch would not be the best solution, but seems to make the situation better. If I understand correctly, now we are just skipping all websocket tests by mac-tiger/Skipped even when python 2.5 is manually installed. So, I'd like to put r+ for this patch. Or, we might want to add a flag like --skip-python25-tests-if-we-dont-have-python25 into old-run-webkit-tests and use this flag in our buildbot for now. What do you think? By the way Ukai-san, cannot we remove websocket/tests from mac-tiger/Skipped with this patch?
(In reply to comment #6) > By the way Ukai-san, cannot we remove websocket/tests from mac-tiger/Skipped with this patch? yes, we can.
Created attachment 57738 [details] Patch
Comment on attachment 57738 [details] Patch > print "Your Python version is insuficient to run WebKit's Python code. Please update.\n"; Typo: should be "insufficient". + `./WebKitTools/Scripts/ensure-valid-python --check-only`; We normally use system(), not backticks. But there's precedent for the latter, too.
Committed r60652: <http://trac.webkit.org/changeset/60652>
Can't exec "WebKitTools/Scripts/ensure-valid-python": No such file or directory at ../../Projects/WebKit/WebKitTools/Scripts/old-run-webkit-tests line 1441. WARNING: Your platform does not have Python 2.5+, which is required to run websocket server, so disabling websocket/tests. Running build-dumprendertree I get that when trying to call run-webkit-tests from outside of a webkit checkout. Seems the change didn't use the wkdirs module as it should have.
Yes, this is the wrong way to launch scripts: 1441 `./WebKitTools/Scripts/ensure-valid-python --check-only`; The perl code is lame about how it handles paths. The python code always uses absolute paths. The other callsites make similar relative-path calls and get away with it because they're after the chdirWebKit() call in old-run-webkit-tests. I think you should instead make your path absolute using sourceDir() from webkitdirs. You could also use relativeScriptsDir().
Can't exec "WebKitTools/Scripts/ensure-valid-python": No such file or directory at ../WebKitTools/Scripts/old-run-webkit-tests line 1441. WARNING: Your platform does not have Python 2.5+, which is required to run websocket server, so disabling websocket/tests. I'm getting this all the time. We need to fix or roll this out (please). :)
Created attachment 58125 [details] Patch
Comment on attachment 58125 [details] Patch I think its good practice for all paths to be absolute, regardless of what the current directory is. I wouldn't bother adding the comment, but this looks fine.
Comment on attachment 57738 [details] Patch Marking this patch obsolete since it was landed.
Committed r60875: <http://trac.webkit.org/changeset/60875>