Bug 50475

Summary: Circular dependency in webkitpy.common.checkout.changelog module
Product: WebKit Reporter: Ademar Reis <ademar>
Component: Tools / TestsAssignee: Ademar Reis <ademar>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dpranke, eric, levin, mihaip, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
fix for circular dependency in checkout.py
eric: review-
fix for circular dependency in checkout.py, v2 none

Ademar Reis
Reported 2010-12-03 12:51:22 PST
There's a circular dependency when using the webkitpy/common/checkout/changelog.py module: changelog.py -> bugzilla.parse_bug_id -> net.Bugzilla -> net.Credentials -> checkout.scm.Git -> [here's the main problem] checkout.api.Checkout -> checkout.changeLog.ChangeLog -> bugzilla.parse_bug_id ... See testcase and backtrace below: (master)[ademar@optimusbook webkitpy]$ python common/checkout/changelog.py Traceback (most recent call last): File "common/checkout/changelog.py", line 39, in <module> from webkitpy.common.net.bugzilla import parse_bug_id File "/opt/projects/webkit/webkit/WebKitTools/Scripts/webkitpy/common/net/bugzilla/__init__.py", line 5, in <module> from .bugzilla import Bugzilla, parse_bug_id File "/opt/projects/webkit/webkit/WebKitTools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py", line 44, in <module> from webkitpy.common.net.credentials import Credentials File "/opt/projects/webkit/webkit/WebKitTools/Scripts/webkitpy/common/net/credentials.py", line 37, in <module> from webkitpy.common.checkout.scm import Git File "/opt/projects/webkit/webkit/WebKitTools/Scripts/webkitpy/common/checkout/__init__.py", line 3, in <module> from api import Checkout File "/opt/projects/webkit/webkit/WebKitTools/Scripts/webkitpy/common/checkout/api.py", line 32, in <module> from webkitpy.common.checkout.changelog import ChangeLog File "/opt/projects/webkit/webkit/WebKitTools/Scripts/webkitpy/common/checkout/changelog.py", line 39, in <module> from webkitpy.common.net.bugzilla import parse_bug_id ImportError: cannot import name parse_bug_id This was introduced by r70820 (bug 48567), which added "from api import Checkout" to common/checkout/__init__.py. I see this import is a common "pattern" inside several modules from webkitpy, but it's a problematic pattern IMO, since __init__.py is run everytime any submodule from the directory is imported. In this case, the problem is that when checkout.scm.Git is imported the api.Checkout module is imported as well. Since there is no code that imports common/checkout directly (only submodules) a patch that removes the offending line is on the way. Other modules with similar __init__.py files are webkitpy/common/net/bugzilla and webkitpy/tool/*, but I'm leaving these alone because: 1. There's code in the repository that imports these modules directly and changing __init__.py would require several refactorings; 2. I couldn't find any broken script due to potential circular deps on these modules. Automating a test for this fix is tricky: due to the way imports work on python, a circular import will fail only when we try to get a symbol from a module which was not completely finished it's import yet (see http://effbot.org/zone/import-confusion.htm). Thus a simple circular import will work, but a circular import of a function will fail. That's why the failure happens around parse_bug_id. Example: This code fails: #!/bin/env python from webkitpy.common.net.bugzilla.bugzilla import parse_bug_id This code works: #!/bin/env python import webkitpy.common.checkout.changelog from webkitpy.common.net.bugzilla.bugzilla import parse_bug_id So the only way to test this is would be by inserting a circular dependency using symbols, which is what currently happens with parse_bug_id. Since the parse_bug_id import itself is unrelated to this particular circular dependency I won't add a test.
Attachments
fix for circular dependency in checkout.py (1.61 KB, patch)
2010-12-03 12:56 PST, Ademar Reis
eric: review-
fix for circular dependency in checkout.py, v2 (1.63 KB, patch)
2010-12-21 11:33 PST, Ademar Reis
no flags
Ademar Reis
Comment 1 2010-12-03 12:56:56 PST
Created attachment 75529 [details] fix for circular dependency in checkout.py See bug description for a comprehensive description of the problem and this fix.
Eric Seidel (no email)
Comment 2 2010-12-09 23:49:20 PST
Why aren't others seeing this?
Eric Seidel (no email)
Comment 3 2010-12-09 23:50:16 PST
Comment on attachment 75529 [details] fix for circular dependency in checkout.py The problem is that Git shouldn't depend on Checkout. We shoudl fix that part instead. I don't see this trouble on my machine.
Ademar Reis
Comment 4 2010-12-13 10:05:28 PST
(In reply to comment #3) > (From update of attachment 75529 [details]) > The problem is that Git shouldn't depend on Checkout. We shoudl fix that part instead. Which is exactly what I pinpointed and what my patch fixes... :-) Look: common/checkout/__init__.py has "from api import Checkout", therefore an import of any checkout submodule (including checkout.scm.git) will import checkout. > > I don't see this trouble on my machine. Just to confirm, you're telling me that the testcases: $ python common/checkout/changelog.py and """ #!/bin/env python from webkitpy.common.net.bugzilla.bugzilla import parse_bug_id """ (with PYTHONPATH set to trunk's webkitpy) both work on your machine?
Ademar Reis
Comment 5 2010-12-21 11:33:10 PST
Created attachment 77140 [details] fix for circular dependency in checkout.py, v2 Adding a new patch, improving the changelog to clarify the problem/solution and also rebasing with current trunk. Please see the testcases and explanation in the bug description and later comments for more details.
Eric Seidel (no email)
Comment 6 2010-12-21 11:36:31 PST
Maybe no one outside of Checkout should import Git directly? It's also possible that the parsing logic should just move/be copied into ChangeLog instead of ChangeLog reaching out to bugzilla.py. Alternatively, the parsing logic could move to common.config.urls
Eric Seidel (no email)
Comment 7 2010-12-21 11:37:25 PST
I'm still under the impresion that it's normal for a module's __init__ to import and expose classes from the module as the api for that module. So I don't think just removing this import from __init__ is correct. I've CC'd a bunch of other folks who speak python.
Dirk Pranke
Comment 8 2010-12-21 12:52:33 PST
Hm. I didn't fully capture the dependencies in my head and I'm not super-familiar with this code so it's hard for me to say specifically that this patch is right. I do agree with Eric however that we should probably fix checkout.scm.Git to not depend on checkout.scm.Checkout. My generic understanding is that one either uses __init__.py and doesn't allow importing of modules in the package, or one has an empty __init__.py and modules in the package are allowed to be accessed directly. It sounds like we're doing neither in Checkout, which is going to lead to problems like the one you've found.
Dirk Pranke
Comment 9 2010-12-21 12:52:49 PST
(In reply to comment #8) > Hm. I didn't fully capture the dependencies in my head and I'm not super-familiar with this code so it's hard for me to say specifically that this patch is right. I do agree with Eric however that we should probably fix checkout.scm.Git to not depend on checkout.scm.Checkout. > > My generic understanding is that one either uses __init__.py and doesn't allow importing of modules in the package, or one has an empty __init__.py and modules in the package are allowed to be accessed directly. > > It sounds like we're doing neither in Checkout, which is going to lead to problems like the one you've found. Er, make that checkout.api.Checkout.
Ademar Reis
Comment 10 2010-12-21 13:04:18 PST
(In reply to comment #8) > [...] I do agree with Eric however that we should probably fix checkout.scm.Git to not depend on checkout.api.Checkout. checkout.scm.Git doesn't depend on checkout.api.Checkout. The only reason why the later is imported is the __init__.py line that does the import automatically whenever a checkout submodule is imported.
Darin Adler
Comment 11 2010-12-29 17:50:10 PST
Comment on attachment 77140 [details] fix for circular dependency in checkout.py, v2 I am assuming this is correct; maybe I should not have been the one to review this!
WebKit Commit Bot
Comment 12 2010-12-29 18:08:51 PST
Comment on attachment 77140 [details] fix for circular dependency in checkout.py, v2 Clearing flags on attachment: 77140 Committed r74774: <http://trac.webkit.org/changeset/74774>
WebKit Commit Bot
Comment 13 2010-12-29 18:08:59 PST
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 14 2010-12-29 18:27:52 PST
Yeah, I'm not sure this is the right way to fix this (I'm not even sure it's a bug). But Adam and I will clean things up. THanks to you both.
Note You need to log in before you can comment on or make changes to this bug.