Bug 32896

Summary: [bzt] Bugzilla-tool should not try to get the username and password from Git when we're using SVN
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Trace
none
Patch (Work in progress)
none
Patch (Work in progress)
none
Patch (Work in progress)
none
Patch with unit tests abarth: review+

Description Daniel Bates 2009-12-23 01:36:03 PST
Bugzilla-tool tries to read the username and password from Git to use on bugs.webkit.org. However, if neither Git is installed nor we are using Git as our SCM for WebKit then bugzilla-tool will die (with a trace). For instance, we may be using SVN.

We should make bugzilla-tool handle the case when the external command (i.e. git) cannot be found.
Comment 1 Daniel Bates 2009-12-23 01:37:36 PST
Created attachment 45425 [details]
Trace

Python trace that occurred when I tried to land-patches 32799.
Comment 2 Daniel Bates 2009-12-23 01:39:43 PST
Created attachment 45426 [details]
Patch (Work in progress)

Work in progress.
Comment 3 Adam Barth 2009-12-23 11:56:15 PST
A couple comments:

1) We already have code that detects whether we're in a Git working copy: Git.in_working_directory in SCM.py.

2) We'll need a unittest.  You can look in credentials_unittest to see some examples.
Comment 4 Daniel Bates 2009-12-24 22:37:10 PST
Created attachment 45483 [details]
Patch (Work in progress)

Revised patch to use Git.in_working_directory.

Added argument cwd (current working directory) to Credentials constructor. Added unit test that uses SVNTestRepository to create a temporary SVN repo. before calling credentials.read_credentials.
Comment 5 Daniel Bates 2009-12-24 22:39:55 PST
Created attachment 45484 [details]
Patch (Work in progress)
Comment 6 Daniel Bates 2009-12-27 17:32:09 PST
Created attachment 45538 [details]
Patch with unit tests

Notice, calling Git.in_working_directory can throw an OSError errrno 2 exception if the git command is not found because we call Executive.run_command which does not catch the exception. Hence, this patch modifies Executive.run_command to catch these exceptions and passes them to the specified error handler/returns the error code of the exception/returns the error message of the exception.

For the credentials unit test, I assume SVN is installed as per <http://webkit.org/building/tools.html> at the time of this writing, since SVNTestRepository (imported from modules.scm_unittest) assumes it.

Also, I added a new unit test file, executive_unittest.py, to test the change made to Executive.run_command.
Comment 7 WebKit Review Bot 2009-12-27 17:37:31 PST
style-queue ran check-webkit-style on attachment 45538 [details] without any errors.
Comment 8 Adam Barth 2009-12-27 17:46:35 PST
Comment on attachment 45538 [details]
Patch with unit tests

This looks good.  Thanks Dan!
Comment 9 Daniel Bates 2009-12-27 18:02:55 PST
Committed in <http://trac.webkit.org/changeset/52587>.
Comment 10 Eric Seidel (no email) 2010-01-19 16:18:33 PST
This broke a couple scm.py unit tests:
ERROR: test_error_handlers (webkitpy.scm_unittest.SCMClassTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/scm_unittest.py", line 150, in test_error_handlers
    self.assertRaises(OSError, run_command, command_does_not_exist)
  File "/System/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/unittest.py", line 320, in failUnlessRaises
    callableObj(*args, **kwargs)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/executive.py", line 70, in run_command
    return Executive().run_command(*args, **kwargs)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/executive.py", line 142, in run_command
    (error_handler or self.default_error_handler)(script_error)
  File "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/executive.py", line 113, in default_error_handler
    raise error
ScriptError: Failed to run "['does_not_exist', 'invalid_option']" exit_code: 2

I'm not sure catching OSError here is a good idea.  It turns missing scripts into "exit code 2" which happens to be the same as QueueEngine.handled_error_code = 2

We obviously could change handled_error_code....
Comment 11 Daniel Bates 2010-01-19 16:59:43 PST
Will look into this issue.

(In reply to comment #10)
> This broke a couple scm.py unit tests:
> ERROR: test_error_handlers (webkitpy.scm_unittest.SCMClassTests)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File
> "/Users/eseidel/Projects/WebKit/WebKitTools/Scripts/webkitpy/scm_unittest.py",
> line 150, in test_error_handlers
>     self.assertRaises(OSError, run_command, command_does_not_exist)
>   File
> [...]
> I'm not sure catching OSError here is a good idea.  It turns missing scripts
> into "exit code 2" which happens to be the same as
> QueueEngine.handled_error_code = 2
> 
> We obviously could change handled_error_code....