Bug 72863 - "webkit-patch upload" fails silently if patch is over 2000 KB in size
Summary: "webkit-patch upload" fails silently if patch is over 2000 KB in size
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
: 35208 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-11-21 00:17 PST by epoger
Modified: 2020-05-04 07:47 PDT (History)
4 users (show)

See Also:


Attachments
test case (5.50 MB, application/octet-stream)
2011-11-21 00:28 PST, Eric Seidel (no email)
no flags Details
Patch (2.62 KB, patch)
2012-01-19 12:29 PST, epoger
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description epoger 2011-11-21 00:17:44 PST
if you use “webkit-patch upload” to upload a patch, and the patch is over 2000 KB in size, it pretends to succeed but actually fails to upload the patch to the webkit bug. (If I try to attach the patch via the UI, I get this error message: “The file you are trying to attach is 2651 kilobytes (KB) in size. Patches cannot be more than 2000 KB in size. Try breaking your patch into several pieces.”

We should change "webkit-patch upload" to report failure in this case, rather than pretending to have succeeded.
Comment 1 epoger 2011-11-21 00:25:16 PST
Assigning to Eric, for now, since he made the most recent change I can find in this area:


2011-05-31  Eric Seidel  <eric@webkit.org>

        Reviewed by David Kilzer.

        webkitpy should create zips with zip -9
        https://bugs.webkit.org/show_bug.cgi?id=61789

        Dave Kilzer suggested we add this to make zips uploaded to bugzilla smaller.

        * Scripts/webkitpy/common/system/workspace.py:
        * Scripts/webkitpy/common/system/workspace_unittest.py:
Comment 2 Eric Seidel (no email) 2011-11-21 00:27:50 PST
Very related to bug 31760.
Comment 3 Eric Seidel (no email) 2011-11-21 00:28:57 PST
Created attachment 116044 [details]
test case
Comment 4 Eric Seidel (no email) 2011-11-21 00:29:23 PST
I see, it's only for patches.  I'll try again.
Comment 5 Eric Seidel (no email) 2011-11-21 00:31:05 PST
Yup, only for patches.
Comment 6 epoger 2012-01-19 12:29:54 PST
Created attachment 123175 [details]
Patch
Comment 7 Eric Seidel (no email) 2012-01-19 12:31:25 PST
Comment on attachment 123175 [details]
Patch

Just needs testing. :)  OTherwise looks OK.
Comment 8 epoger 2012-01-19 12:37:41 PST
I take it you are asking for unittests to be added?

I'm not particularly interested in doing that.  (I did do manual tests and it seemed to work fine.)  But I keep bumping into this bug, it's really annoying, and I wanted to save other developers the annoyance of the silent failures documented here.  But there's only so much time I have to devote to this.

I think this change improves the code.  If someone wants to take on the task of adding unittests that's fine, but I would encourage you to accept the improvement anyway.
Comment 9 Adam Barth 2012-01-19 13:54:20 PST
Thanks for the patch, but we follow the general guidelines of the WebKit project, which is not to accept patches that change behavior without tests.  Hopefully you'll either find time to write some tests or someone else will build upon your work.
Comment 10 Eric Seidel (no email) 2012-02-03 11:41:37 PST
*** Bug 35208 has been marked as a duplicate of this bug. ***
Comment 11 Nikolas Zimmermann 2012-02-06 01:44:00 PST
I have an even simpler fix for the problem, how about we always click the 'big file' checkbox?

diff --git a/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py b/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py
index a32e86e..4529bf0 100644
--- a/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py
+++ b/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py
@@ -530,6 +530,7 @@ class Bugzilla(object):
         self.browser['description'] = description
         if is_patch:
             self.browser['ispatch'] = ("1",)
+        self.browser['bigfile'] = ("bigfile",)

This way I could upload my over 4MB patches at bug 77736.
Comment 12 Eric Seidel (no email) 2012-02-06 09:35:49 PST
I don't know what the big-file checkbox does, or if it's a good idea to ever (let alone always) press it.
Comment 13 Eric Seidel (no email) 2012-03-26 13:40:46 PDT
Ideally we'd detect that the patch was over 2MB (but less than 5MB) and automatically check the "big patch" checkbox.