Bug 63651 - Update kill-old-processes logic
Summary: Update kill-old-processes logic
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: Lucas Forschler
URL:
Keywords:
Depends on:
Blocks: 69976
  Show dependency treegraph
 
Reported: 2011-06-29 13:43 PDT by Lucas Forschler
Modified: 2011-10-12 21:08 PDT (History)
6 users (show)

See Also:


Attachments
Patch to move to new kill-old-processes script (1.87 KB, application/octet-stream)
2011-06-29 13:47 PDT, Lucas Forschler
lforschler: commit-queue+
Details
Patch to move to new kill-old-processes script (1.87 KB, patch)
2011-06-29 15:28 PDT, Lucas Forschler
no flags Details | Formatted Diff | Diff
Updated patch (2.62 KB, patch)
2011-07-01 16:15 PDT, Lucas Forschler
aroben: review-
aroben: commit-queue-
Details | Formatted Diff | Diff
fixed patch. (2.62 KB, patch)
2011-07-05 06:21 PDT, Lucas Forschler
no flags Details | Formatted Diff | Diff
retry. (2.57 KB, patch)
2011-07-05 08:53 PDT, Lucas Forschler
no flags Details | Formatted Diff | Diff
Updated patch with ChangeLog corrections (2.66 KB, patch)
2011-07-11 13:14 PDT, Lucas Forschler
rniwa: review+
rniwa: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Forschler 2011-06-29 13:43:52 PDT
We currently have a kill-old-processes script running on windows bots.  We should run this on mac bots as well.
A platform agnostic (windows/mac) kill-old-processes script has been checked in already.  Start using it.
Comment 1 Lucas Forschler 2011-06-29 13:47:25 PDT
Created attachment 99144 [details]
Patch to move to new kill-old-processes script
Comment 2 Lucas Forschler 2011-06-29 15:28:14 PDT
Created attachment 99160 [details]
Patch to move to new kill-old-processes script

fixed patch.
Comment 3 Adam Roben (:aroben) 2011-07-01 12:54:52 PDT
Comment on attachment 99160 [details]
Patch to move to new kill-old-processes script

Will KillOldProcesses do the right thing on GTK/Qt/EFL/etc.?
Comment 4 Lucas Forschler 2011-07-01 13:00:24 PDT
It will exit out on any non Cygwin/Darwin platform.  Currently, this script is only run on platforms "win", "chromium-win".  This patch removes that check, so it will now run on darwin as well.

It will raise an exception on non Cygwin/Darwin platforms.  Rather than raising an exception, should we just exit quietly and do nothing?  If someone had specifics for the GTK/Qt/EFL targets, it would be good to update them.
Comment 5 Adam Roben (:aroben) 2011-07-01 13:04:00 PDT
(In reply to comment #4)
> It will raise an exception on non Cygwin/Darwin platforms.  Rather than raising an exception, should we just exit quietly and do nothing?

Yes.
Comment 6 Lucas Forschler 2011-07-01 16:15:05 PDT
Created attachment 99535 [details]
Updated patch

Updated patch to exit clean on unsupported platforms rather than raise an exception.
Comment 7 Adam Roben (:aroben) 2011-07-05 06:17:21 PDT
Comment on attachment 99535 [details]
Updated patch

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

> Tools/BuildSlaveSupport/kill-old-processes:80
> +        sys.exit

Don't you need some parentheses here? sys.exit()
Comment 8 Lucas Forschler 2011-07-05 06:21:14 PDT
Created attachment 99710 [details]
fixed patch.

fixed.
Comment 9 WebKit Review Bot 2011-07-05 07:19:44 PDT
Comment on attachment 99710 [details]
fixed patch.

Rejecting attachment 99710 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 1

Last 500 characters of output:
75f1b5384c06c3d0e73f6948134a89e1359e122e
r90396 = 00d69d39ebebfb88c0ed81355a9508de38c8de30
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/8985403
Comment 10 Lucas Forschler 2011-07-05 08:53:40 PDT
Created attachment 99722 [details]
retry.

previous patch failed to apply, trying again.
Comment 11 WebKit Review Bot 2011-07-05 09:44:41 PDT
Comment on attachment 99722 [details]
retry.

Rejecting attachment 99722 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 1

Last 500 characters of output:
d057bd753106a3b2d9055448e662c77866336023
r90399 = 740b27ceddf9032c6928be43100fd7b2314e7228
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/8983684
Comment 12 Adam Barth 2011-07-10 08:52:11 PDT
Comment on attachment 99722 [details]
retry.

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

> Tools/ChangeLog:3
> +        Need a short description and bug URL (OOPS!)

You need to remove this line in order for the patch to land.  Also, you need to not remove the Reviewed by NOBODY (OOPS!) line.
Comment 13 Lucas Forschler 2011-07-11 13:14:18 PDT
Created attachment 100356 [details]
Updated patch with ChangeLog corrections

update collision with eric's change 90412.  Updated to use sys.exit()

As for the comment about why there is no 't' in WebKitPluginAgen..  when I examined the processes list, it didn't show a 't' in the process name.  I don't think it was truncated in my view, but I could be mistaken.
Comment 14 Eric Seidel (no email) 2011-09-06 15:29:19 PDT
Comment on attachment 99710 [details]
fixed patch.

Cleared Adam Roben's review+ from obsolete attachment 99710 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 15 Eric Seidel (no email) 2011-09-06 15:29:22 PDT
Comment on attachment 99722 [details]
retry.

Cleared Adam Roben's review+ from obsolete attachment 99722 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 16 Ryosuke Niwa 2011-10-12 16:55:31 PDT
Comment on attachment 100356 [details]
Updated patch with ChangeLog corrections

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

Looks sane to me.

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=63651
> +        Update kill-old-processes logic.

The order in which bug title and url appear is wrong. Also you need a blank line after these two lines. Please fix that before you land.
Comment 17 Ryosuke Niwa 2011-10-12 16:55:45 PDT
Comment on attachment 100356 [details]
Updated patch with ChangeLog corrections

Oops, I meant cq-.
Comment 18 Ryosuke Niwa 2011-10-12 21:08:43 PDT
Committed r97341: <http://trac.webkit.org/changeset/97341>