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
Created attachment 153260 [details] [check-webkit-style] Don't crash if SVN not installed Handle OSError exceptions when the svn command is not installed
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.
Created attachment 153284 [details] Patch
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.
(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.
Created attachment 153312 [details] Patch Fix the introduced python variable scoping error
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.
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.
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
Created attachment 206523 [details] Patch Rebased on top of the current trunk, simplified a bit and added a testcase
(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.
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.