Bug 28225 – check-webkit-style uses python from /usr/bin instead of the PATH

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carol Szabo]

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.

[Carol Szabo]

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.

[Mark Rowe (bdash)]

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 Rowe (bdash)]

Comment on attachment 34686 [details]
Patch: uses python from $PATH

Clearing review flag for reason mentioned.

[Carol Szabo]

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

[Laszlo Gombos]

(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

[Eric Seidel (no email)]

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.

[Carol Szabo]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Comment on attachment 38831 [details]
This is the second patch attempt for this problem

Flakey test. :(
compositing/color-matching/image-color-matching.html -> crashed

[Eric Seidel (no email)]

I believe that's just another instance of bug 28845 biting us. :(

[Eric Seidel (no email)]

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>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.