|Summary:||check-webkit-style crashes with OSError exception if SVN is not installed|
|Product:||WebKit||Reporter:||Emanuele Aina <emanuele.aina>|
|Component:||Tools / Tests||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||abarth, commit-queue, dpranke, eric, glenn, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Emanuele Aina 2012-07-19 07:14:50 PDT
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
Comment 1 Emanuele Aina 2012-07-19 07:42:03 PDT
Created attachment 153260 [details] [check-webkit-style] Don't crash if SVN not installed Handle OSError exceptions when the svn command is not installed
Comment 2 Alexey Proskuryakov 2012-07-19 09:05:12 PDT
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.
Comment 4 Adam Barth 2012-07-19 10:56:07 PDT
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.
Comment 5 Emanuele Aina 2012-07-19 11:15:44 PDT
(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.
Comment 6 Emanuele Aina 2012-07-19 11:19:23 PDT
Created attachment 153312 [details] Patch Fix the introduced python variable scoping error
Comment 7 Eric Seidel (no email) 2012-09-15 09:56:48 PDT
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.
Comment 8 Eric Seidel (no email) 2012-09-15 10:01:08 PDT
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 9 WebKit Commit Bot 2013-05-27 16:40:59 PDT
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
Comment 10 Emanuele Aina 2013-07-12 04:11:03 PDT
Created attachment 206523 [details] Patch Rebased on top of the current trunk, simplified a bit and added a testcase
Comment 11 Emanuele Aina 2013-07-12 04:19:28 PDT
(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 12 Darin Adler 2014-08-19 09:17:38 PDT
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.