49944 2010-11-22 15:49:26 -0800 speculative fix for upload errors: stop using mechanize to upload to test-results.appspot.com 2010-11-24 11:20:39 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 ojan ojan abarth eric tony victorw oldest_to_newest 312567 0 ojan 2010-11-22 15:49:26 -0800 speculative fix for upload errors: stop using mechanize to upload to test-results.appspot.com 312572 1 74608 ojan 2010-11-22 15:54:00 -0800 Created attachment 74608 Patch 312584 2 74608 abarth 2010-11-22 16:12:58 -0800 Comment on attachment 74608 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74608&action=review Do we really want to be implementing HTTP in Python? Surely there's a library to do that for us. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:44 > +def _encode_multipart_form_data(fields, files): > + """Encode form fields for multipart/form-data. Yuck 312598 3 ojan 2010-11-22 16:40:12 -0800 (In reply to comment #2) > (From update of attachment 74608 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74608&action=review > > Do we really want to be implementing HTTP in Python? Surely there's a library to do that for us. You mean the encoding or using urllib2 directly? If the former, I don't know of a library to do that. If the latter, I'm not really sure what the benefit of a layer of indirection is. > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:44 > > +def _encode_multipart_form_data(fields, files): > > + """Encode form fields for multipart/form-data. > > Yuck Open to suggestions. This is what rietveld's upload code uses, so at least it's well tested. 313007 4 74608 tony 2010-11-23 13:54:20 -0800 Comment on attachment 74608 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74608&action=review Seems OK to try, but I'm skeptical mechanize itself is the problem. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:60 > + for (key, value) in fields: Nit: no ()s > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:68 > + for (key, filename, value) in files: Nit: no ()s > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_results_uploader.py:97 > + for (filename, path) in files: Nit: no ()s 313029 5 ojan 2010-11-23 15:00:15 -0800 Committed r72633: <http://trac.webkit.org/changeset/72633> 313086 6 eric 2010-11-23 16:50:00 -0800 Um... this seems a bad idea. This is mechanizes job. Not ours. Do we actually understand the bug? It seems it should be fixed in mechanize or gae or a wrapper around mechanize. I would have r-ed this. 313360 7 ojan 2010-11-24 10:02:36 -0800 (In reply to comment #6) > Um... this seems a bad idea. This is mechanizes job. Not ours. I don't really understand what mechanize buys us in this case. Here are the downsides of using mechanize: -It uses a GET and a POST, where a POST is sufficient here. -It adds a layer of indirection between the application logic and urllib2. I see the value of using mechanize for bugzilla GETs. It saves us the work of parsing the bugzilla html. I don't see the benefit of using it for POSTs though. To me it just makes for more complicated code that's hard to debug and, in some cases, is slower because you need to do an extra GET. >Do we actually understand the bug? No. What I was seeing from appengine logs is that it would think the uploaded files were empty. From the buildbot side, there was definitely a non-empty file on disk. FWIW, this did seem to fix the issue. There is still a timeout issue on appengine, but the empty files issue seems to have gone away. >It seems it should be fixed in mechanize or gae or a wrapper around mechanize. Maybe. 313406 8 abarth 2010-11-24 11:20:39 -0800 I think the philosophy here is that webkitpy shouldn't know anything about HTTP. That knowledge should be encapsulated in some library or another. If we can't find a library that understands enough about HTTP to do this job, then we should write such a library ourselves, but we shouldn't conflate knowledge of HTTP into an object that knows about uploading LayoutTest results. That's a big layering violation. 74608 2010-11-22 15:54:00 -0800 2010-11-23 13:54:20 -0800 Patch bug-49944-20101122155359.patch text/plain 4774 ojan ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCA1MWM3MDA5NjRkOWQ1MzliMmQ0Y2VkZWU4MjY0MTI5ZWM5YmU2OGNhLi40YTQyNzgz Mjg5ODE4NzNlOTE2OTI2YjgzMWExZjE0NTI2ZDM1ZTljIDEwMDY0NAotLS0gYS9XZWJLaXRUb29s cy9DaGFuZ2VMb2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTIgQEAK KzIwMTAtMTEtMjIgIE9qYW4gVmFmYWkgIDxvamFuQGNocm9taXVtLm9yZz4KKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBzcGVjdWxhdGl2ZSBmaXggZm9y IHVwbG9hZCBlcnJvcnM6IHN0b3AgdXNpbmcgbWVjaGFuaXplIHRvIHVwbG9hZCB0byB0ZXN0LXJl c3VsdHMuYXBwc3BvdC5jb20KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTQ5OTQ0CisKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0 cy9sYXlvdXRfcGFja2FnZS90ZXN0X3Jlc3VsdHNfdXBsb2FkZXIucHk6CisKIDIwMTAtMTEtMjIg IEhheWF0byBJdG8gIDxoYXlhdG9AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5 IFNoaW5pY2hpcm8gSGFtYWppLgpkaWZmIC0tZ2l0IGEvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJr aXRweS9sYXlvdXRfdGVzdHMvbGF5b3V0X3BhY2thZ2UvdGVzdF9yZXN1bHRzX3VwbG9hZGVyLnB5 IGIvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvbGF5b3V0X3BhY2th Z2UvdGVzdF9yZXN1bHRzX3VwbG9hZGVyLnB5CmluZGV4IDY4MGI4NDhjYWNjYWJiODVmNjhlMzk1 ZGRiYTBhYTRkNTExNTM2ODUuLjk0ZTVkZTMwOWU2OWFhNjIxNTk1NTc3MDQzYjRiMmQzYTlkODU2 OTEgMTAwNjQ0Ci0tLSBhL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3Rz L2xheW91dF9wYWNrYWdlL3Rlc3RfcmVzdWx0c191cGxvYWRlci5weQorKysgYi9XZWJLaXRUb29s cy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9sYXlvdXRfcGFja2FnZS90ZXN0X3Jlc3Vs dHNfdXBsb2FkZXIucHkKQEAgLTI3LDQ1ICsyNyw4MSBAQAogIyAoSU5DTFVESU5HIE5FR0xJR0VO Q0UgT1IgT1RIRVJXSVNFKSBBUklTSU5HIElOIEFOWSBXQVkgT1VUIE9GIFRIRSBVU0UKICMgT0Yg VEhJUyBTT0ZUV0FSRSwgRVZFTiBJRiBBRFZJU0VEIE9GIFRIRSBQT1NTSUJJTElUWSBPRiBTVUNI IERBTUFHRS4KIAorZnJvbSBfX2Z1dHVyZV9fIGltcG9ydCB3aXRoX3N0YXRlbWVudAorCitpbXBv cnQgY29kZWNzCiBpbXBvcnQgbWltZXR5cGVzCiBpbXBvcnQgc29ja2V0CitpbXBvcnQgdXJsbGli MgogCiBmcm9tIHdlYmtpdHB5LmNvbW1vbi5uZXQubmV0d29ya3RyYW5zYWN0aW9uIGltcG9ydCBO ZXR3b3JrVHJhbnNhY3Rpb24KLWZyb20gd2Via2l0cHkudGhpcmRwYXJ0eS5hdXRvaW5zdGFsbGVk Lm1lY2hhbml6ZSBpbXBvcnQgQnJvd3NlcgotCiAKIGRlZiBnZXRfbWltZV90eXBlKGZpbGVuYW1l KToKLSAgICByZXR1cm4gbWltZXR5cGVzLmd1ZXNzX3R5cGUoZmlsZW5hbWUpWzBdIG9yICJ0ZXh0 L3BsYWluIgorICAgIHJldHVybiBtaW1ldHlwZXMuZ3Vlc3NfdHlwZShmaWxlbmFtZSlbMF0gb3Ig J2FwcGxpY2F0aW9uL29jdGV0LXN0cmVhbScKKworCitkZWYgX2VuY29kZV9tdWx0aXBhcnRfZm9y bV9kYXRhKGZpZWxkcywgZmlsZXMpOgorICAgICIiIkVuY29kZSBmb3JtIGZpZWxkcyBmb3IgbXVs dGlwYXJ0L2Zvcm0tZGF0YS4KKworICAgIEFyZ3M6CisgICAgICBmaWVsZHM6IEEgc2VxdWVuY2Ug b2YgKG5hbWUsIHZhbHVlKSBlbGVtZW50cyBmb3IgcmVndWxhciBmb3JtIGZpZWxkcy4KKyAgICAg IGZpbGVzOiBBIHNlcXVlbmNlIG9mIChuYW1lLCBmaWxlbmFtZSwgdmFsdWUpIGVsZW1lbnRzIGZv ciBkYXRhIHRvIGJlCisgICAgICAgICAgICAgdXBsb2FkZWQgYXMgZmlsZXMuCisgICAgUmV0dXJu czoKKyAgICAgIChjb250ZW50X3R5cGUsIGJvZHkpIHJlYWR5IGZvciBodHRwbGliLkhUVFAgaW5z dGFuY2UuCisKKyAgICBTb3VyY2U6CisgICAgICBodHRwOi8vY29kZS5nb29nbGUuY29tL3Avcmll dHZlbGQvc291cmNlL2Jyb3dzZS90cnVuay91cGxvYWQucHkKKyAgICAiIiIKKyAgICBCT1VOREFS WSA9ICctTS1BLUctSS1DLS0tQi1PLVUtTi1ELUEtUi1ZLScKKyAgICBDUkxGID0gJ1xyXG4nCisg ICAgbGluZXMgPSBbXQorCisgICAgZm9yIChrZXksIHZhbHVlKSBpbiBmaWVsZHM6CisgICAgICAg IGxpbmVzLmFwcGVuZCgnLS0nICsgQk9VTkRBUlkpCisgICAgICAgIGxpbmVzLmFwcGVuZCgnQ29u dGVudC1EaXNwb3NpdGlvbjogZm9ybS1kYXRhOyBuYW1lPSIlcyInICUga2V5KQorICAgICAgICBs aW5lcy5hcHBlbmQoJycpCisgICAgICAgIGlmIGlzaW5zdGFuY2UodmFsdWUsIHVuaWNvZGUpOgor ICAgICAgICAgICAgdmFsdWUgPSB2YWx1ZS5lbmNvZGUoJ3V0Zi04JykKKyAgICAgICAgbGluZXMu YXBwZW5kKHZhbHVlKQorCisgICAgZm9yIChrZXksIGZpbGVuYW1lLCB2YWx1ZSkgaW4gZmlsZXM6 CisgICAgICAgIGxpbmVzLmFwcGVuZCgnLS0nICsgQk9VTkRBUlkpCisgICAgICAgIGxpbmVzLmFw cGVuZCgnQ29udGVudC1EaXNwb3NpdGlvbjogZm9ybS1kYXRhOyBuYW1lPSIlcyI7IGZpbGVuYW1l PSIlcyInICUgKGtleSwgZmlsZW5hbWUpKQorICAgICAgICBsaW5lcy5hcHBlbmQoJ0NvbnRlbnQt VHlwZTogJXMnICUgZ2V0X21pbWVfdHlwZShmaWxlbmFtZSkpCisgICAgICAgIGxpbmVzLmFwcGVu ZCgnJykKKyAgICAgICAgaWYgaXNpbnN0YW5jZSh2YWx1ZSwgdW5pY29kZSk6CisgICAgICAgICAg ICB2YWx1ZSA9IHZhbHVlLmVuY29kZSgndXRmLTgnKQorICAgICAgICBsaW5lcy5hcHBlbmQodmFs dWUpCisKKyAgICBsaW5lcy5hcHBlbmQoJy0tJyArIEJPVU5EQVJZICsgJy0tJykKKyAgICBsaW5l cy5hcHBlbmQoJycpCisgICAgYm9keSA9IENSTEYuam9pbihsaW5lcykKKyAgICBjb250ZW50X3R5 cGUgPSAnbXVsdGlwYXJ0L2Zvcm0tZGF0YTsgYm91bmRhcnk9JXMnICUgQk9VTkRBUlkKKyAgICBy ZXR1cm4gY29udGVudF90eXBlLCBib2R5CiAKIAogY2xhc3MgVGVzdFJlc3VsdHNVcGxvYWRlcjoK ICAgICBkZWYgX19pbml0X18oc2VsZiwgaG9zdCk6CiAgICAgICAgIHNlbGYuX2hvc3QgPSBob3N0 Ci0gICAgICAgIHNlbGYuX2Jyb3dzZXIgPSBCcm93c2VyKCkKIAogICAgIGRlZiBfdXBsb2FkX2Zp bGVzKHNlbGYsIGF0dHJzLCBmaWxlX29ianMpOgotICAgICAgICBzZWxmLl9icm93c2VyLm9wZW4o Imh0dHA6Ly8lcy90ZXN0ZmlsZS91cGxvYWRmb3JtIiAlIHNlbGYuX2hvc3QpCi0gICAgICAgIHNl bGYuX2Jyb3dzZXIuc2VsZWN0X2Zvcm0oInRlc3RfcmVzdWx0X3VwbG9hZCIpCi0gICAgICAgIGZv ciAobmFtZSwgZGF0YSkgaW4gYXR0cnM6Ci0gICAgICAgICAgICBzZWxmLl9icm93c2VyW25hbWVd ID0gc3RyKGRhdGEpCi0KLSAgICAgICAgZm9yIChmaWxlbmFtZSwgaGFuZGxlKSBpbiBmaWxlX29i anM6Ci0gICAgICAgICAgICBzZWxmLl9icm93c2VyLmFkZF9maWxlKGhhbmRsZSwgZ2V0X21pbWVf dHlwZShmaWxlbmFtZSksIGZpbGVuYW1lLAotICAgICAgICAgICAgICAgICJmaWxlIikKLQotICAg ICAgICBzZWxmLl9icm93c2VyLnN1Ym1pdCgpCisgICAgICAgIHVybCA9ICJodHRwOi8vJXMvdGVz dGZpbGUvdXBsb2FkIiAlIHNlbGYuX2hvc3QKKyAgICAgICAgY29udGVudF90eXBlLCBkYXRhID0g X2VuY29kZV9tdWx0aXBhcnRfZm9ybV9kYXRhKGF0dHJzLCBmaWxlX29ianMpCisgICAgICAgIGhl YWRlcnMgPSB7IkNvbnRlbnQtVHlwZSI6IGNvbnRlbnRfdHlwZX0KKyAgICAgICAgcmVxdWVzdCA9 IHVybGxpYjIuUmVxdWVzdCh1cmwsIGRhdGEsIGhlYWRlcnMpCisgICAgICAgIHVybGxpYjIudXJs b3BlbihyZXF1ZXN0LCB0aW1lb3V0PTYwKQogCiAgICAgZGVmIHVwbG9hZChzZWxmLCBwYXJhbXMs IGZpbGVzLCB0aW1lb3V0X3NlY29uZHMpOgotICAgICAgICBvcmlnX3RpbWVvdXQgPSBzb2NrZXQu Z2V0ZGVmYXVsdHRpbWVvdXQoKQogICAgICAgICBmaWxlX29ianMgPSBbXQotICAgICAgICB0cnk6 Ci0gICAgICAgICAgICBmaWxlX29ianMgPSBbKGZpbGVuYW1lLCBvcGVuKHBhdGgsICJyYiIpKSBm b3IgKGZpbGVuYW1lLCBwYXRoKQotICAgICAgICAgICAgICAgIGluIGZpbGVzXQorICAgICAgICBm b3IgKGZpbGVuYW1lLCBwYXRoKSBpbiBmaWxlczoKKyAgICAgICAgICAgIHdpdGggY29kZWNzLm9w ZW4ocGF0aCwgInJiIikgYXMgZmlsZToKKyAgICAgICAgICAgICAgICBmaWxlX29ianMuYXBwZW5k KCgnZmlsZScsIGZpbGVuYW1lLCBmaWxlLnJlYWQoKSkpCiAKKyAgICAgICAgb3JpZ190aW1lb3V0 ID0gc29ja2V0LmdldGRlZmF1bHR0aW1lb3V0KCkKKyAgICAgICAgdHJ5OgogICAgICAgICAgICAg c29ja2V0LnNldGRlZmF1bHR0aW1lb3V0KHRpbWVvdXRfc2Vjb25kcykKICAgICAgICAgICAgIE5l dHdvcmtUcmFuc2FjdGlvbih0aW1lb3V0X3NlY29uZHM9dGltZW91dF9zZWNvbmRzKS5ydW4oCiAg ICAgICAgICAgICAgICAgbGFtYmRhOiBzZWxmLl91cGxvYWRfZmlsZXMocGFyYW1zLCBmaWxlX29i anMpKQogICAgICAgICBmaW5hbGx5OgogICAgICAgICAgICAgc29ja2V0LnNldGRlZmF1bHR0aW1l b3V0KG9yaWdfdGltZW91dCkKLSAgICAgICAgICAgIGZvciAoZmlsZW5hbWUsIGhhbmRsZSkgaW4g ZmlsZV9vYmpzOgotICAgICAgICAgICAgICAgIGhhbmRsZS5jbG9zZSgpCg==