Bug 107606 - EWS creates 0 byte sized log files
Summary: EWS creates 0 byte sized log files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 124782 124783
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-22 17:50 PST by Lucas Forschler
Modified: 2013-11-25 07:45 PST (History)
10 users (show)

See Also:


Attachments
Patch (3.45 KB, patch)
2013-11-11 09:16 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (3.99 KB, patch)
2013-11-21 06:47 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (4.61 KB, patch)
2013-11-22 07:52 PST, László Langó
no flags Details | Formatted Diff | Diff
Patch (7.17 KB, patch)
2013-11-25 00:49 PST, László Langó
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Forschler 2013-01-22 17:50:11 PST
Ever since r138264, EWS creates 0 length log files.
The only logs which have content, are those with errors.

It is strange to see a directory full of empty files.  We should not be creating them unless something is to be put in them.  
We might not be able to detect this until the run is over... at which we could check for 0 length files, and prune them.
Comment 1 Eric Seidel (no email) 2013-01-22 17:57:37 PST
They should be smaller, but not zero sized. :(

We can revert the change.
Comment 2 László Langó 2013-11-11 09:16:18 PST
Created attachment 216578 [details]
Patch
Comment 3 Ryosuke Niwa 2013-11-21 03:59:25 PST
Comment on attachment 216578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216578&action=review

> Tools/ChangeLog:13
> +
> +        * Scripts/webkitpy/tool/bot/queueengine.py:
> +        (QueueEngine.run):
> +        (QueueEngine._open_work_log):
> +        (QueueEngine._ensure_work_log_closed):
> +        * Scripts/webkitpy/tool/commands/earlywarningsystem.py:
> +        (AbstractEarlyWarningSystem.review_patch):

I'd like to see some description as to why this bug manifested and how you fixed it.
There is a lot of code change here, and the correctness of code change is not self-evident.
Comment 4 László Langó 2013-11-21 05:53:59 PST
(In reply to comment #3)
> (From update of attachment 216578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216578&action=review
> 
> > Tools/ChangeLog:13
> > +
> > +        * Scripts/webkitpy/tool/bot/queueengine.py:
> > +        (QueueEngine.run):
> > +        (QueueEngine._open_work_log):
> > +        (QueueEngine._ensure_work_log_closed):
> > +        * Scripts/webkitpy/tool/commands/earlywarningsystem.py:
> > +        (AbstractEarlyWarningSystem.review_patch):
> 
> I'd like to see some description as to why this bug manifested and how you fixed it.
> There is a lot of code change here, and the correctness of code change is not self-evident.

There was a modification in r138264, that tried to make less log, because some of the messeges were duplicated. After this the EWS created the log file (with the same name as the bugID) but doesn't write anything into it, even if there were errors during the build. So i managed this. Now it will create the log file only if there is an error, and wont create if dont want to write anything to it.

The main problem was that, the ScriptError was not raised and the QueueEngine never get that. Now it gets the error and handles it.
Comment 5 Ryosuke Niwa 2013-11-21 06:01:12 PST
Please add per-function/code change comment as to how you fixed that problem.
Comment 6 László Langó 2013-11-21 06:47:18 PST
Created attachment 217559 [details]
Patch
Comment 7 Ryosuke Niwa 2013-11-21 19:23:04 PST
Comment on attachment 217559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217559&action=review

> Tools/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the long description but after the bug url.

> Tools/ChangeLog:15
> +        (QueueEngine.run): Open the log file only if a ScriptError was raised.

Why?

> Tools/ChangeLog:16
> +        (QueueEngine._open_work_log): Does not need to tee STDOUT to log file anymore.

Why?

> Tools/ChangeLog:17
> +        (QueueEngine._ensure_work_log_closed): Close the logfile.

Why?

> Tools/ChangeLog:19
> +        (AbstractEarlyWarningSystem.review_patch): Raise again the captured ScriptError.

Why?
Comment 8 Ryosuke Niwa 2013-11-21 19:24:11 PST
The current change log comments simply repeats the code change.  I'm looking for is an explanation as to why each code change is made.
Comment 9 László Langó 2013-11-22 07:52:43 PST
Created attachment 217684 [details]
Patch
Comment 10 Ryosuke Niwa 2013-11-22 07:56:59 PST
Comment on attachment 217684 [details]
Patch

Thanks.
Comment 11 WebKit Commit Bot 2013-11-22 08:24:04 PST
Comment on attachment 217684 [details]
Patch

Clearing flags on attachment: 217684

Committed r159690: <http://trac.webkit.org/changeset/159690>
Comment 12 WebKit Commit Bot 2013-11-22 08:24:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Commit Bot 2013-11-22 09:39:37 PST
Re-opened since this is blocked by bug 124782
Comment 14 Alexey Proskuryakov 2013-11-22 09:41:00 PST
This broke webkitpy tests, rolling out. It's not obvious whether these just need an update of results.

http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20(Tests)/builds/15098/steps/webkitpy-test/logs/stdio
Comment 15 László Langó 2013-11-25 00:49:43 PST
Created attachment 217777 [details]
Patch
Comment 16 László Langó 2013-11-25 00:58:39 PST
(In reply to comment #14)
> This broke webkitpy tests, rolling out. It's not obvious whether these just need an update of results.
> 
> http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20(Tests)/builds/15098/steps/webkitpy-test/logs/stdio

Unfortunately i forgot to update the unit test. It really was not obvious, but this change is needed, because of the patch. I wrote some explanation into the Change log too.
Comment 17 WebKit Commit Bot 2013-11-25 07:45:37 PST
Comment on attachment 217777 [details]
Patch

Clearing flags on attachment: 217777

Committed r159749: <http://trac.webkit.org/changeset/159749>
Comment 18 WebKit Commit Bot 2013-11-25 07:45:41 PST
All reviewed patches have been landed.  Closing bug.