Bug 91742 – check-webkit-style crashes with OSError exception if SVN is not installed

• Component: Tools / Tests
• Status: UNCONFIRMED

[Emanuele Aina]

I'm working on a git checkout and I don't have SVN installed.

I get the following OSError exception when running `Tools/Scripts/check-webkit-style -g HEAD':

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/srv/src/c/co/webkit/webkit-clutter/Tools/Scripts/webkitpy/style/main.py", line 127, in main
    host._initialize_scm()
  File "/srv/src/c/co/webkit/webkit-clutter/Tools/Scripts/webkitpy/common/host.py", line 132, in _initialize_scm
    self._scm = detector.default_scm(patch_directories)
  File "/srv/src/c/co/webkit/webkit-clutter/Tools/Scripts/webkitpy/common/checkout/scm/detection.py", line 53, in default_scm
    scm_system = self.detect_scm_system(cwd, patch_directories)
  File "/srv/src/c/co/webkit/webkit-clutter/Tools/Scripts/webkitpy/common/checkout/scm/detection.py", line 69, in detect_scm_system
    if SVN.in_working_directory(absolute_path):
  File "/srv/src/c/co/webkit/webkit-clutter/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 93, in in_working_directory
    exit_code = Executive().run_command(svn_info_args, cwd=path, return_exit_code=True)
  File "/srv/src/c/co/webkit/webkit-clutter/Tools/Scripts/webkitpy/common/system/executive.py", line 397, in run_command
    close_fds=self._should_close_fds())
  File "/srv/src/c/co/webkit/webkit-clutter/Tools/Scripts/webkitpy/common/system/executive.py", line 453, in popen
    return subprocess.Popen(*args, **kwargs)
  File "/usr/lib/python2.7/subprocess.py", line 679, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1259, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

[Emanuele Aina]

Created attachment 153260 [details]
[check-webkit-style] Don't crash if SVN not installed

Handle OSError exceptions when the svn command is not installed

[Alexey Proskuryakov]

Would you be willing to submit a patch for review as described in <http://www.webkit.org/coding/contributing.html>? Notably, a patch should have a ChangeLog, and it should have r? flag to be visible in review queue.

[Emanuele Aina]

Created attachment 153284 [details]
Patch

[Adam Barth]

Comment on attachment 153284 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153284&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/svn.py:97
> +            exit_code = Executive().run_command(svn_info_args, cwd=path, return_exit_code=True)
> +        except OSError:
> +            return False
>          return (exit_code == 0)

Does this work?  The way Python scoping works exit_code will be limited in scope to just the "try" block and won't be available during the return statement.

[Emanuele Aina]

(In reply to comment #4)

> Does this work?  The way Python scoping works exit_code will be
>  limited in scope to just the "try" block and won't be available during
> the return statement.

Yup, you're right, sorry. I'll send an updated patch.

[Emanuele Aina]

Created attachment 153312 [details]
Patch

Fix the introduced python variable scoping error

[Eric Seidel (no email)]

Comment on attachment 153312 [details]
Patch

When is this called?  Maybe we should just ahve a separate SVN.is_svn_installed() method that caller wants to check first?  Also, changes to the python generally unit tests (which should be easy to add here... We may have eto have this method take an optional filesystem/Executive parameter so it can be easily unittested with MockFileSystem, MockExecutive.

I suspect you didn't really want to get this involved in teh python code. :)  But testing it makes it so we don't break you in teh future.

[Eric Seidel (no email)]

The patch is "fine" as is.  But it's not immediately clear to me what other callers might expect.  If you think all callers want this behavior than this is fine (we'd still like a unittest).  If you think that some might not, then it makes more sense to add the is_installed() helper instead.

[WebKit Commit Bot]

Comment on attachment 153312 [details]
Patch

Rejecting attachment 153312 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 153312, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
n Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 2 diffs from patch file(s).
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/webkitpy/common/checkout/scm/svn.py
Hunk #1 FAILED at 90.
1 out of 1 hunk FAILED -- saving rejects to file Tools/Scripts/webkitpy/common/checkout/scm/svn.py.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/672273

[Emanuele Aina]

Created attachment 206523 [details]
Patch

Rebased on top of the current trunk, simplified a bit and added a testcase

[Emanuele Aina]

(In reply to comment #8)
> The patch is "fine" as is.  But it's not immediately clear to me what other callers might expect.  If you think all callers want this behavior than this is fine (we'd still like a unittest).  If you think that some might not, then it makes more sense to add the is_installed() helper instead.

Looking at the callers I don't feel that differentiating the two cases would give any benefit: they all seem to call the check to avoid invoking the SCM outside of a working directory so checking for the SCM availabity in in_working_directory() seems to make sense for all of them.

[Darin Adler]

Comment on attachment 206523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206523&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/svn.py:104
> +            _log.warning("No `svn` executable found in $PATH, assume this is not a Subversion repository.")

Not sure I understand the grammar here. Here’s the sentence:

    No executable found, assume this is not a repository.

It seems like a comma splice, and I don’t understand why the tool is telling me to assume something.