check-webkit-style starts with '#!/usr/bin/python'. This prevents usage of the script on computers where /usr/bin/python is absent or is too old even if an appropriate python version exists in the PATH.
Created attachment 34686 [details] Patch: uses python from $PATH This solution was suggested by kollivier on the #webkit IRC channel. It worked for me and it may be useful to others.
None of our existing scripts in WebKitTools/Scripts use this approach. If this is a problem I don't see why we would only fix a single script.
Comment on attachment 34686 [details] Patch: uses python from $PATH Clearing review flag for reason mentioned.
(In reply to comment #2) > None of our existing scripts in WebKitTools/Scripts use this approach. If this > is a problem I don't see why we would only fix a single script. Mark, You are right that this would fix only one script and if otherwise the fix is acceptable I am willing to change all scripts in the directory to use the same approach. On the other hand, there is one other script (roll-over-ChangeLogs) which uses the same approach, so it can be argued that since it is ok for that script it should be ok for this script too. I believe that the /usr/bin/env approach does not make sense for standard shells (such as /usr/bin/sh), but may be useful for perl, python and ruby scripts where many incompatible versions may exist in the system.
(In reply to comment #4) > (In reply to comment #2) > > None of our existing scripts in WebKitTools/Scripts use this approach. If this > > is a problem I don't see why we would only fix a single script. > > Mark, > You are right that this would fix only one script and if otherwise the fix is > acceptable I am willing to change all scripts in the directory to use the same > approach. > > On the other hand, there is one other script (roll-over-ChangeLogs) which uses > the same approach, so it can be argued that since it is ok for that script it > should be ok for this script too. > > I believe that the /usr/bin/env approach does not make sense for standard > shells (such as /usr/bin/sh), but may be useful for perl, python and ruby > scripts where many incompatible versions may exist in the system. Perhaps -for now - we should just do this for "top level" python scripts. I did a quick search and I only found 3 scripts: check-webkit-style, bugzilla-tool, run-webkit-unittests
Comment on attachment 34686 [details] Patch: uses python from $PATH Marking r- per the discussion above. Either we should do this for all scripts or none.
Created attachment 38831 [details] This is the second patch attempt for this problem I have applied the fix to all python scripts per lgombos's comments. I was reluctant to fix all scripts in the directory as I am not familiar with many of them and I think that it is preferable that (as the case of style errors in C/C++ code) they are fixed as they cause problems. I am not quite an "if its not broken don't fix it" kind of guy, but I am reluctant to take poorly understood (at least by me) risks when the benefit is small.
Comment on attachment 38831 [details] This is the second patch attempt for this problem Rejecting patch 38831 from commit-queue. This patch will require manual commit. Failed to run "/Users/eseidel/Projects/WebKitSVN/WebKitTools/Scripts/update-webkit" exit_code: 1 cwd: None
Comment on attachment 38831 [details] This is the second patch attempt for this problem svn: Failed to add file 'WebCore/bindings/v8/custom/V8WebSocketCustom.cpp': object of the same name already exists This was a victim of bug 28603. :(
Comment on attachment 38831 [details] This is the second patch attempt for this problem Rejecting patch 38831 from commit-queue. This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment on attachment 38831 [details] This is the second patch attempt for this problem Flakey test. :( compositing/color-matching/image-color-matching.html -> crashed
I believe that's just another instance of bug 28845 biting us. :(
Comment on attachment 38831 [details] This is the second patch attempt for this problem Clearing flags on attachment: 38831 Committed r47955: <http://trac.webkit.org/changeset/47955>
All reviewed patches have been landed. Closing bug.