50475 2010-12-03 12:51:22 -0800 Circular dependency in webkitpy.common.checkout.changelog module 2010-12-29 18:27:52 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 1 ademar ademar abarth commit-queue dpranke eric levin mihaip tony oldest_to_newest 316976 0 ademar 2010-12-03 12:51:22 -0800 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. 316980 1 75529 ademar 2010-12-03 12:56:56 -0800 Created attachment 75529 fix for circular dependency in checkout.py See bug description for a comprehensive description of the problem and this fix. 319967 2 eric 2010-12-09 23:49:20 -0800 Why aren't others seeing this? 319968 3 75529 eric 2010-12-09 23:50:16 -0800 Comment on attachment 75529 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. 321184 4 ademar 2010-12-13 10:05:28 -0800 (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? 325106 5 77140 ademar 2010-12-21 11:33:10 -0800 Created attachment 77140 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. 325109 6 eric 2010-12-21 11:36:31 -0800 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 325110 7 eric 2010-12-21 11:37:25 -0800 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. 325153 8 dpranke 2010-12-21 12:52:33 -0800 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. 325154 9 dpranke 2010-12-21 12:52:49 -0800 (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. 325161 10 ademar 2010-12-21 13:04:18 -0800 (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. 327771 11 77140 darin 2010-12-29 17:50:10 -0800 Comment on attachment 77140 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! 327777 12 77140 commit-queue 2010-12-29 18:08:51 -0800 Comment on attachment 77140 fix for circular dependency in checkout.py, v2 Clearing flags on attachment: 77140 Committed r74774: <http://trac.webkit.org/changeset/74774> 327778 13 commit-queue 2010-12-29 18:08:59 -0800 All reviewed patches have been landed. Closing bug. 327785 14 eric 2010-12-29 18:27:52 -0800 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. 75529 2010-12-03 12:56:56 -0800 2010-12-21 11:33:10 -0800 fix for circular dependency in checkout.py 0001-Circular-dependency-in-webkitpy.common.checkout.chan.patch text/plain 1647 ademar RnJvbSA5ZTM5MGJiZDZiYTQ2NDVlZGE4MTE3YWY2YTRjMTVmNDczZDBlZTI0IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBZGVtYXIgZGUgU291emEgUmVpcyBKciA8YWRlbWFyLnJlaXNA b3BlbmJvc3NhLm9yZz4KRGF0ZTogRnJpLCAzIERlYyAyMDEwIDE3OjUyOjAwIC0wMzAwClN1Ympl Y3Q6IFtQQVRDSF0gQ2lyY3VsYXIgZGVwZW5kZW5jeSBpbiB3ZWJraXRweS5jb21tb24uY2hlY2tv dXQuY2hhbmdlbG9nIG1vZHVsZQoKUmVtb3ZlIGF1dG9tYXRpYyBpbXBvcnQgb2YgYXBpLkNoZWNr b3V0IG1vZHVsZSB3aGVuIGFueSBjaGVja291dC8Kc3VibW9kdWxlIGlzIGltcG9ydGVkLgoKaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUwNDc1Ci0tLQogV2ViS2l0VG9v bHMvQ2hhbmdlTG9nICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgfCAgIDEyICsrKysrKysr KysrKwogLi4uL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L19faW5pdF9fLnB5ICAg fCAgICAyIC0tCiAyIGZpbGVzIGNoYW5nZWQsIDEyIGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25z KC0pCgpkaWZmIC0tZ2l0IGEvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nIGIvV2ViS2l0VG9vbHMvQ2hh bmdlTG9nCmluZGV4IGNmOTRmNzYuLmY4OWI0NjcgMTAwNjQ0Ci0tLSBhL1dlYktpdFRvb2xzL0No YW5nZUxvZworKysgYi9XZWJLaXRUb29scy9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAx MC0xMi0wMyAgQWRlbWFyIGRlIFNvdXphIFJlaXMgSnIgIDxhZGVtYXIucmVpc0BvcGVuYm9zc2Eu b3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIENp cmN1bGFyIGRlcGVuZGVuY3kgaW4gd2Via2l0cHkuY29tbW9uLmNoZWNrb3V0LmNoYW5nZWxvZyBt b2R1bGUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUw NDc1CisKKyAgICAgICAgUmVtb3ZlIGF1dG9tYXRpYyBpbXBvcnQgb2YgYXBpLkNoZWNrb3V0IG1v ZHVsZSB3aGVuIGFueSBjaGVja291dC8KKyAgICAgICAgc3VibW9kdWxlIGlzIGltcG9ydGVkLgor CisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvX19pbml0X18ucHk6 CisKIDIwMTAtMTItMDMgIFNoZXJpZmYgQm90ICA8d2Via2l0LnJldmlldy5ib3RAZ21haWwuY29t PgogCiAgICAgICAgIFVucmV2aWV3ZWQsIHJvbGxpbmcgb3V0IHI3MzIxMS4KZGlmZiAtLWdpdCBh L1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L19faW5pdF9fLnB5 IGIvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvX19pbml0X18u cHkKaW5kZXggNTk3ZGNiZC4uZWY2NWJlZSAxMDA2NDQKLS0tIGEvV2ViS2l0VG9vbHMvU2NyaXB0 cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvX19pbml0X18ucHkKKysrIGIvV2ViS2l0VG9vbHMv U2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvX19pbml0X18ucHkKQEAgLTEsMyArMSBA QAogIyBSZXF1aXJlZCBmb3IgUHl0aG9uIHRvIHNlYXJjaCB0aGlzIGRpcmVjdG9yeSBmb3IgbW9k dWxlIGZpbGVzCi0KLWZyb20gYXBpIGltcG9ydCBDaGVja291dAotLSAKMS43LjIuMwoK 77140 2010-12-21 11:33:10 -0800 2010-12-29 18:08:51 -0800 fix for circular dependency in checkout.py, v2 0001-Circular-dependency-in-webkitpy.common.checkout.chan.patch text/plain 1672 ademar RnJvbSAzYTUxYzM4MjQ2YmJjYTIyMTc3YTk5NzFhZmZlM2Y4MjBhNTdlNDVkIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBZGVtYXIgZGUgU291emEgUmVpcyBKciA8YWRlbWFyLnJlaXNA b3BlbmJvc3NhLm9yZz4KRGF0ZTogRnJpLCAzIERlYyAyMDEwIDE3OjUyOjAwIC0wMzAwClN1Ympl Y3Q6IFtQQVRDSF0gQ2lyY3VsYXIgZGVwZW5kZW5jeSBpbiB3ZWJraXRweS5jb21tb24uY2hlY2tv dXQuY2hhbmdlbG9nIG1vZHVsZQoKUmVtb3ZlIGF1dG9tYXRpYyBpbXBvcnQgb2YgYXBpLkNoZWNr b3V0IG1vZHVsZSB3aGVuIGFueSBjaGVja291dC8Kc3VibW9kdWxlIGlzIGltcG9ydGVkIChlLmcu IHdoZW4gY2hlY2tvdXQuc2NtLkdpdCBpcyBpbXBvcnRlZCkuCgpodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTA0NzUKLS0tCiBUb29scy9DaGFuZ2VMb2cgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICB8ICAgMTIgKysrKysrKysrKysrCiBUb29scy9TY3Jp cHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9fX2luaXRfXy5weSB8ICAgIDIgLS0KIDIgZmls ZXMgY2hhbmdlZCwgMTIgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQg YS9Ub29scy9DaGFuZ2VMb2cgYi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggOWQ5N2I5Yi4uY2RiMWI5 NiAxMDA2NDQKLS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAt MSwzICsxLDE1IEBACisyMDEwLTEyLTAzICBBZGVtYXIgZGUgU291emEgUmVpcyBKciAgPGFkZW1h ci5yZWlzQG9wZW5ib3NzYS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgQ2lyY3VsYXIgZGVwZW5kZW5jeSBpbiB3ZWJraXRweS5jb21tb24uY2hl Y2tvdXQuY2hhbmdlbG9nIG1vZHVsZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9NTA0NzUKKworICAgICAgICBSZW1vdmUgYXV0b21hdGljIGltcG9ydCBv ZiBhcGkuQ2hlY2tvdXQgbW9kdWxlIHdoZW4gYW55IGNoZWNrb3V0LworICAgICAgICBzdWJtb2R1 bGUgaXMgaW1wb3J0ZWQgKGUuZy46IHdoZW4gY2hlY2tvdXQuc2NtLkdpdCBpcyBpbXBvcnRlZCku CisKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9fX2luaXRfXy5w eToKKwogMjAxMC0xMi0yMSAgQW5kZXJzIENhcmxzc29uICA8YW5kZXJzY2FAYXBwbGUuY29tPgog CiAgICAgICAgIFJldmlld2VkIGJ5IEpvaG4gU3VsbGl2YW4uCmRpZmYgLS1naXQgYS9Ub29scy9T Y3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9fX2luaXRfXy5weSBiL1Rvb2xzL1Njcmlw dHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L19faW5pdF9fLnB5CmluZGV4IDU5N2RjYmQuLmVm NjViZWUgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0 L19faW5pdF9fLnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0 L19faW5pdF9fLnB5CkBAIC0xLDMgKzEgQEAKICMgUmVxdWlyZWQgZm9yIFB5dGhvbiB0byBzZWFy Y2ggdGhpcyBkaXJlY3RvcnkgZm9yIG1vZHVsZSBmaWxlcwotCi1mcm9tIGFwaSBpbXBvcnQgQ2hl Y2tvdXQKLS0gCjEuNy4yLjMKCg==