Bug 50475 – Circular dependency in webkitpy.common.checkout.changelog module

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ademar Reis]

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.

[Ademar Reis]

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)]

Why aren't others seeing this?

[Eric Seidel (no email)]

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]

(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]

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)]

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)]

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]

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]

(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]

(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 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 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]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.