33791 2010-01-18 03:21:38 -0800 check-webkit-style: Checking patch does not work when script not run from source root 2010-01-23 14:25:10 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 33775 1 cjerdonek cjerdonek abarth cjerdonek eric hamaji levin oldest_to_newest 181807 0 cjerdonek 2010-01-18 03:21:38 -0800 I get errors like the following when running check-webkit-style from a folder other than the source root -- e.g. WebKitTools/ Skipping input 'WebKitTools/Scripts/webkitpy/style/text_style.py': Can't open for reading Skipping input 'WebKitTools/Scripts/webkitpy/style/checker.py': Can't open for reading This is a one-line fix (adding a call to os.chdir() in check-webkit-style), but I'm going to submit another change in this patch as well. 181809 1 46809 cjerdonek 2010-01-18 03:51:03 -0800 Created attachment 46809 Proposed patch [Serial patch: this patch assumes the patch from bug 33775 has been checked in (awaiting review). As a result, the status bots may not work.] In this patch, in addition to the one-line fix, I'm also experimenting with a new way to manage intra-package references. As a sibling to the style/ folder, I'm adding a style_references.py module to house external dependencies. The reasoning behind this is more fully explained in the code comments at the beginning of style_references.py. As a side benefit, this also allows us to remove the __path__ hack that I added a while back to __init__.py. See, for instance-- https://bugs.webkit.org/show_bug.cgi?id=32971#c3 https://bugs.webkit.org/show_bug.cgi?id=32971#c6 181914 2 46809 abarth 2010-01-18 10:12:13 -0800 Comment on attachment 46809 Proposed patch This looks good, especially because the style module has so few dependencies on the rest of webkitpy. We'll have to see how this approach scales to other modules. Why is style_references outside the style module? I would have expected it to be located at style.references. 181937 3 cjerdonek 2010-01-18 11:31:41 -0800 (In reply to comment #2) > (From update of attachment 46809 [details]) > This looks good, especially because the style module has so few dependencies on > the rest of webkitpy. We'll have to see how this approach scales to other > modules. > > Why is style_references outside the style module? I would have expected it to > be located at style.references. Good question. It's largely a matter of convention, but my reasons were-- (1) This way, if non-style modules (for example) need to be refactored in ways that affect the style references (e.g. moving or renaming the scm module), this can be done without having to modify the contents of the style package at all. (One of the purposes of the references file is to insulate the style package from changes taking place in other modules/packages.) I think this will also make viewing the source hierarchy in trac a bit more convenient: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy The "Last Change" next to style/ will always be the last change to the logic of the style package rather than a change to the workings of one of its dependencies (which are always changing anyways). (2) We don't need to have any "zig-zag" dependencies, e.g.-- from ..diff_parser from ..scm which may be conceptually simpler. The style package just has a single reference up, and then the reference file has a bunch of references down (or across). (3) I think the intra-package references will be more visible this way. Someone will be able to inspect intra-package references without having to navigate into many sub-folders. And it will be easy to tell which packages have intra-package references simply by viewing the top-level webkitpy folder and seeing which ones have an adjacent *_references.py file. As we move forward, I think this setup will allow us to take stock of and manage our intra-package references more easily -- perhaps choosing a different pattern in the process. It's probably not worth thinking too far ahead, but it seems like this approach might provide a natural transition to an approach where we don't have any ".." references at all (think dependency injection). 183801 4 cjerdonek 2010-01-22 14:10:21 -0800 Manually committed (via "git svn dcommit") r53715: http://trac.webkit.org/changeset/53715 There was a slight problem though in that commit-log-editor didn't kick in and include the ChangeLog as the commit message. I'm not sure what I did wrong. I followed the instructions on the wiki and included commit-log-editor in my core.editor config: http://trac.webkit.org/wiki/UsingGitWithWebKit Is this something I should correct (the commit log message for the check-in, I mean)? Thanks. 183804 5 cjerdonek 2010-01-22 14:14:53 -0800 (In reply to comment #4) > Manually committed (via "git svn dcommit") r53715: > > http://trac.webkit.org/changeset/53715 > > There was a slight problem though in that commit-log-editor didn't kick in and > include the ChangeLog as the commit message. I'm not sure what I did wrong. I > followed the instructions on the wiki and included commit-log-editor in my > core.editor config: > > http://trac.webkit.org/wiki/UsingGitWithWebKit > > Is this something I should correct (the commit log message for the check-in, I > mean)? Thanks. Oh, I understand what I did wrong, now. I should simply have ensured that my local commit message was correct (e.g. using commit-log-editor with commit, prior to dcommit). This is simpler than for SVN. :) Still let me know if I should correct the message in the repo. Thanks. 183809 6 eric 2010-01-22 14:37:49 -0800 I too have never mastered svn-commit-editor. webkit-patch has its own logic (sadly), and doesn't require setting up a commit-log-editor when using commands like "land". 46809 2010-01-18 03:51:03 -0800 2010-01-18 10:12:13 -0800 Proposed patch _patch-33791-1.txt text/plain 6213 cjerdonek ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCA5ZTk0ZDE3Li5lMGQzMTcwIDEwMDY0NAotLS0gYS9XZWJLaXRUb29scy9DaGFuZ2VM b2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjQgQEAKKzIwMTAtMDEt MTggIENocmlzIEplcmRvbmVrICA8Y2plcmRvbmVrQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTWFkZSBjaGVjay13ZWJraXQtc3R5 bGUgYWJsZSB0byBjaGVjayBwYXRjaGVzIHdoZW4gc2NyaXB0IG5vdAorICAgICAgICBydW4gZnJv bSBzb3VyY2Ugcm9vdC4gQWxzbyBjb25zb2xpZGF0ZWQgZXh0ZXJuYWwgcmVmZXJlbmNlcworICAg ICAgICB0byBhIHNpbmdsZSBmaWxlLgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0zMzc5MQorCisgICAgICAgICogU2NyaXB0cy9jaGVjay13ZWJraXQt c3R5bGU6CisgICAgICAgICAgLSBDaGFuZ2VkIHRvIGltcG9ydCBzdHlsZV9yZWZlcmVuY2VzLnB5 LgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9zdHlsZS9fX2luaXRfXy5weToKKyAgICAg ICAgICAtIFJlbW92ZWQgX19wYXRoX18gaGFjayB0aGF0IGFsbG93ZWQgc2VhcmNoaW5nIFNjcmlw dHMvIGRpcmVjdG9yeS4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tl ci5weToKKyAgICAgICAgICAtIENoYW5nZWQgdG8gaW1wb3J0IHN0eWxlX3JlZmVyZW5jZXMucHku CisKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L3N0eWxlX3JlZmVyZW5jZXMucHk6IEFkZGVk LgorCiAyMDEwLTAxLTE3ICBDaHJpcyBKZXJkb25layAgPGNqZXJkb25la0B3ZWJraXQub3JnPgog CiAgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgpkaWZmIC0tZ2l0IGEvV2ViS2l0 VG9vbHMvU2NyaXB0cy9jaGVjay13ZWJraXQtc3R5bGUgYi9XZWJLaXRUb29scy9TY3JpcHRzL2No ZWNrLXdlYmtpdC1zdHlsZQppbmRleCBhMmI1ODRmLi4yNDVkODM0IDEwMDc1NQotLS0gYS9XZWJL aXRUb29scy9TY3JpcHRzL2NoZWNrLXdlYmtpdC1zdHlsZQorKysgYi9XZWJLaXRUb29scy9TY3Jp cHRzL2NoZWNrLXdlYmtpdC1zdHlsZQpAQCAtNDgsOCArNDgsNyBAQCBpbXBvcnQgb3MucGF0aAog aW1wb3J0IHN5cwogCiBpbXBvcnQgd2Via2l0cHkuc3R5bGUuY2hlY2tlciBhcyBjaGVja2VyCi1m cm9tIHdlYmtpdHB5LnNjbSBpbXBvcnQgZGV0ZWN0X3NjbV9zeXN0ZW0KLQorZnJvbSB3ZWJraXRw eS5zdHlsZV9yZWZlcmVuY2VzIGltcG9ydCBTaW1wbGVTY20KIAogZGVmIG1haW4oKToKICAgICAj IENoYW5nZSBzdGRlcnIgdG8gd3JpdGUgd2l0aCByZXBsYWNlbWVudCBjaGFyYWN0ZXJzIHNvIHdl IGRvbid0IGRpZQpAQCAtNzEsOCArNzAsOSBAQCBkZWYgbWFpbigpOgogICAgICAgICAgICAgc3R5 bGVfY2hlY2tlci5jaGVja19maWxlKGZpbGVuYW1lKQogCiAgICAgZWxzZToKLSAgICAgICAgY3dk ID0gb3MucGF0aC5hYnNwYXRoKCcuJykKLSAgICAgICAgc2NtID0gZGV0ZWN0X3NjbV9zeXN0ZW0o Y3dkKQorICAgICAgICBzY20gPSBTaW1wbGVTY20oKQorCisgICAgICAgIG9zLmNoZGlyKHNjbS5j aGVja291dF9yb290KCkpCiAKICAgICAgICAgaWYgb3B0aW9ucy5naXRfY29tbWl0OgogICAgICAg ICAgICAgY29tbWl0ID0gb3B0aW9ucy5naXRfY29tbWl0CmRpZmYgLS1naXQgYS9XZWJLaXRUb29s cy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL19faW5pdF9fLnB5IGIvV2ViS2l0VG9vbHMvU2NyaXB0 cy93ZWJraXRweS9zdHlsZS9fX2luaXRfXy5weQppbmRleCAzNWNiZTRjLi5lZjY1YmVlIDEwMDY0 NAotLS0gYS9XZWJLaXRUb29scy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL19faW5pdF9fLnB5Cisr KyBiL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvX19pbml0X18ucHkKQEAgLTEs NiArMSBAQAogIyBSZXF1aXJlZCBmb3IgUHl0aG9uIHRvIHNlYXJjaCB0aGlzIGRpcmVjdG9yeSBm b3IgbW9kdWxlIGZpbGVzCi0KLWltcG9ydCBvcwotCi0jIEFkZCBjb250YWluaW5nICJ3ZWJraXRw eSIgcGFja2FnZSBkaXJlY3RvcnkgdG8gc2VhcmNoIHBhdGguCi1fX3BhdGhfXy5hcHBlbmQob3Mu cGF0aC5qb2luKF9fcGF0aF9fWzBdLCAiLi4iKSkKZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL1Nj cmlwdHMvd2Via2l0cHkvc3R5bGUvY2hlY2tlci5weSBiL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Vi a2l0cHkvc3R5bGUvY2hlY2tlci5weQppbmRleCA2ZGQ0MWZhLi4zYTMwMmRhIDEwMDY0NAotLS0g YS9XZWJLaXRUb29scy9TY3JpcHRzL3dlYmtpdHB5L3N0eWxlL2NoZWNrZXIucHkKKysrIGIvV2Vi S2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9zdHlsZS9jaGVja2VyLnB5CkBAIC0zNCw3ICszNCw3 IEBAIGltcG9ydCBnZXRvcHQKIGltcG9ydCBvcy5wYXRoCiBpbXBvcnQgc3lzCiAKLWZyb20gZGlm Zl9wYXJzZXIgaW1wb3J0IERpZmZQYXJzZXIKK2Zyb20gLi4gc3R5bGVfcmVmZXJlbmNlcyBpbXBv cnQgcGFyc2VfcGF0Y2gKIGZyb20gY3BwX3N0eWxlIGltcG9ydCBDcHBQcm9jZXNzb3IKIGZyb20g dGV4dF9zdHlsZSBpbXBvcnQgVGV4dFByb2Nlc3NvcgogCkBAIC04NTYsOCArODU2LDggQEAgY2xh c3MgU3R5bGVDaGVja2VyKG9iamVjdCk6CiAgICAgICAgICAgcGF0Y2hfc3RyaW5nOiBBIHN0cmlu ZyBvZiBhIHBhdGNoLgogCiAgICAgICAgICIiIgotICAgICAgICBwYXRjaCA9IERpZmZQYXJzZXIo cGF0Y2hfc3RyaW5nLnNwbGl0bGluZXMoKSkKLSAgICAgICAgZm9yIGZpbGVuYW1lLCBkaWZmIGlu IHBhdGNoLmZpbGVzLml0ZXJpdGVtcygpOgorICAgICAgICBwYXRjaF9maWxlcyA9IHBhcnNlX3Bh dGNoKHBhdGNoX3N0cmluZykKKyAgICAgICAgZm9yIGZpbGVuYW1lLCBkaWZmIGluIHBhdGNoX2Zp bGVzLml0ZXJpdGVtcygpOgogICAgICAgICAgICAgbGluZV9udW1iZXJzID0gc2V0KCkKIAogICAg ICAgICAgICAgZGVmIGVycm9yX2Zvcl9wYXRjaChmaWxlbmFtZSwgbGluZV9udW1iZXIsIGNhdGVn b3J5LCBjb25maWRlbmNlLCBtZXNzYWdlKToKZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL1Njcmlw dHMvd2Via2l0cHkvc3R5bGVfcmVmZXJlbmNlcy5weSBiL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Vi a2l0cHkvc3R5bGVfcmVmZXJlbmNlcy5weQpuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAw MDAwLi4yMzg2OTI2Ci0tLSAvZGV2L251bGwKKysrIGIvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJr aXRweS9zdHlsZV9yZWZlcmVuY2VzLnB5CkBAIC0wLDAgKzEsNzUgQEAKKyMgQ29weXJpZ2h0IChD KSAyMDEwIENocmlzIEplcmRvbmVrIChjamVyZG9uZWtAd2Via2l0Lm9yZykKKyMKKyMgUmVkaXN0 cmlidXRpb24gYW5kIHVzZSBpbiBzb3VyY2UgYW5kIGJpbmFyeSBmb3Jtcywgd2l0aCBvciB3aXRo b3V0CisjIG1vZGlmaWNhdGlvbiwgYXJlIHBlcm1pdHRlZCBwcm92aWRlZCB0aGF0IHRoZSBmb2xs b3dpbmcgY29uZGl0aW9ucyBhcmUKKyMgbWV0OgorIworIyAgICAgKiBSZWRpc3RyaWJ1dGlvbnMg b2Ygc291cmNlIGNvZGUgbXVzdCByZXRhaW4gdGhlIGFib3ZlIGNvcHlyaWdodAorIyBub3RpY2Us IHRoaXMgbGlzdCBvZiBjb25kaXRpb25zIGFuZCB0aGUgZm9sbG93aW5nIGRpc2NsYWltZXIuCisj ICAgICAqIFJlZGlzdHJpYnV0aW9ucyBpbiBiaW5hcnkgZm9ybSBtdXN0IHJlcHJvZHVjZSB0aGUg YWJvdmUKKyMgY29weXJpZ2h0IG5vdGljZSwgdGhpcyBsaXN0IG9mIGNvbmRpdGlvbnMgYW5kIHRo ZSBmb2xsb3dpbmcgZGlzY2xhaW1lcgorIyBpbiB0aGUgZG9jdW1lbnRhdGlvbiBhbmQvb3Igb3Ro ZXIgbWF0ZXJpYWxzIHByb3ZpZGVkIHdpdGggdGhlCisjIGRpc3RyaWJ1dGlvbi4KKyMKKyMgVEhJ UyBTT0ZUV0FSRSBJUyBQUk9WSURFRCBCWSBUSEUgQ09QWVJJR0hUIEhPTERFUlMgQU5EIENPTlRS SUJVVE9SUworIyAiQVMgSVMiIEFORCBBTlkgRVhQUkVTUyBPUiBJTVBMSUVEIFdBUlJBTlRJRVMs IElOQ0xVRElORywgQlVUIE5PVAorIyBMSU1JVEVEIFRPLCBUSEUgSU1QTElFRCBXQVJSQU5USUVT IE9GIE1FUkNIQU5UQUJJTElUWSBBTkQgRklUTkVTUyBGT1IKKyMgQSBQQVJUSUNVTEFSIFBVUlBP U0UgQVJFIERJU0NMQUlNRUQuIElOIE5PIEVWRU5UIFNIQUxMIFRIRSBDT1BZUklHSFQKKyMgT1dO RVIgT1IgQ09OVFJJQlVUT1JTIEJFIExJQUJMRSBGT1IgQU5ZIERJUkVDVCwgSU5ESVJFQ1QsIElO Q0lERU5UQUwsCisjIFNQRUNJQUwsIEVYRU1QTEFSWSwgT1IgQ09OU0VRVUVOVElBTCBEQU1BR0VT IChJTkNMVURJTkcsIEJVVCBOT1QKKyMgTElNSVRFRCBUTywgUFJPQ1VSRU1FTlQgT0YgU1VCU1RJ VFVURSBHT09EUyBPUiBTRVJWSUNFUzsgTE9TUyBPRiBVU0UsCisjIERBVEEsIE9SIFBST0ZJVFM7 IE9SIEJVU0lORVNTIElOVEVSUlVQVElPTikgSE9XRVZFUiBDQVVTRUQgQU5EIE9OIEFOWQorIyBU SEVPUlkgT0YgTElBQklMSVRZLCBXSEVUSEVSIElOIENPTlRSQUNULCBTVFJJQ1QgTElBQklMSVRZ LCBPUiBUT1JUCisjIChJTkNMVURJTkcgTkVHTElHRU5DRSBPUiBPVEhFUldJU0UpIEFSSVNJTkcg SU4gQU5ZIFdBWSBPVVQgT0YgVEhFIFVTRQorIyBPRiBUSElTIFNPRlRXQVJFLCBFVkVOIElGIEFE VklTRUQgT0YgVEhFIFBPU1NJQklMSVRZIE9GIFNVQ0ggREFNQUdFLgorCisiIiJSZWZlcmVuY2Vz IHRvIG5vbi1zdHlsZSBtb2R1bGVzIHVzZWQgYnkgdGhlIHN0eWxlIHBhY2thZ2UuIiIiCisKKyMg VGhpcyBtb2R1bGUgaXMgYSBzaW1wbGUgZmFjYWRlIHRvIHRoZSBmdW5jdGlvbmFsaXR5IHVzZWQg YnkgdGhlCisjIHN0eWxlIHBhY2thZ2UgdGhhdCBjb21lcyBmcm9tIFdlYktpdCBtb2R1bGVzIG91 dHNpZGUgdGhlIHN0eWxlCisjIHBhY2thZ2UuCisjCisjIFdpdGggdGhpcyBtb2R1bGUsIHRoZSBv bmx5IGludHJhLXBhY2thZ2UgcmVmZXJlbmNlcyAoaS5lLgorIyByZWZlcmVuY2VzIHRvIHdlYmtp dHB5IG1vZHVsZXMgb3V0c2lkZSB0aGUgc3R5bGUgZm9sZGVyKSB0aGF0CisjIHRoZSBzdHlsZSBw YWNrYWdlIG5lZWRzIHRvIG1ha2UgYXJlIHJlbGF0aXZlIHJlZmVyZW5jZXMgdG8KKyMgdGhpcyBt b2R1bGUuIEZvciBleGFtcGxlLS0KKyMKKyMgPiBmcm9tIC4uIHN0eWxlX3JlZmVyZW5jZXMgaW1w b3J0IHBhcnNlX3BhdGNoCisjCisjIFNpbWlsYXJseSwgcGVvcGxlIG1haW50YWluaW5nIG5vbi1z dHlsZSBjb2RlIGFyZSBub3QgYmVob2xkZW4KKyMgdG8gdGhlIGNvbnRlbnRzIG9mIHRoZSBzdHls ZSBwYWNrYWdlIHdoZW4gcmVmYWN0b3Jpbmcgb3IKKyMgb3RoZXJ3aXNlIGNoYW5naW5nIG5vbi1z dHlsZSBjb2RlLiBUaGV5IG9ubHkgaGF2ZSB0byBiZSBhd2FyZQorIyBvZiB0aGlzIG1vZHVsZS4K KworaW1wb3J0IG9zCisKK2Zyb20gZGlmZl9wYXJzZXIgaW1wb3J0IERpZmZQYXJzZXIKK2Zyb20g c2NtIGltcG9ydCBkZXRlY3Rfc2NtX3N5c3RlbQorCisKK2RlZiBwYXJzZV9wYXRjaChwYXRjaF9z dHJpbmcpOgorCisgICAgIiIiUGFyc2UgYSBwYXRjaCBzdHJpbmcgYW5kIHJldHVybiB0aGUgYWZm ZWN0ZWQgZmlsZXMuIiIiCisKKyAgICBwYXRjaCA9IERpZmZQYXJzZXIocGF0Y2hfc3RyaW5nLnNw bGl0bGluZXMoKSkKKyAgICByZXR1cm4gcGF0Y2guZmlsZXMKKworCitjbGFzcyBTaW1wbGVTY20o b2JqZWN0KToKKworICAgICIiIlNpbXBsZSBmYWNhZGUgdG8gU0NNIGZvciB1c2UgYnkgc3R5bGUg cGFja2FnZS4iIiIKKworICAgIGRlZiBfX2luaXRfXyhzZWxmKToKKyAgICAgICAgY3dkID0gb3Mu cGF0aC5hYnNwYXRoKCcuJykKKyAgICAgICAgc2VsZi5fc2NtID0gZGV0ZWN0X3NjbV9zeXN0ZW0o Y3dkKQorCisgICAgZGVmIGNoZWNrb3V0X3Jvb3Qoc2VsZik6CisgICAgICAgICIiIlJldHVybiB0 aGUgc291cmNlIGNvbnRyb2wgcm9vdCBhcyBhbiBhYnNvbHV0ZSBwYXRoLiIiIgorICAgICAgICBy ZXR1cm4gc2VsZi5fc2NtLmNoZWNrb3V0X3Jvb3QKKworICAgIGRlZiBjcmVhdGVfcGF0Y2goc2Vs Zik6CisgICAgICAgIHJldHVybiBzZWxmLl9zY20uY3JlYXRlX3BhdGNoKCkKKworICAgIGRlZiBj cmVhdGVfcGF0Y2hfc2luY2VfbG9jYWxfY29tbWl0KHNlbGYsIGNvbW1pdCk6CisgICAgICAgIHJl dHVybiBzZWxmLl9zY20uY3JlYXRlX3BhdGNoX3NpbmNlX2xvY2FsX2NvbW1pdChjb21taXQpCisK