Bug 50372 – nrwt multiprocessing - minor cleanup

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: minor cleanup for multiprocessing work

[Dirk Pranke]

Created attachment 75354 [details]
Patch

[Dirk Pranke]

Created attachment 75356 [details]
yet more cleanup

[Eric Seidel (no email)]

Comment on attachment 75356 [details]
yet more cleanup

I don't feel like line-wrapping patches are very useful.  But then again, I'm in the anti-80c camp. :)

[Ojan Vafai]

Comment on attachment 75356 [details]
yet more cleanup

I agree with Eric. We've decided not to do 80c in webkit python code, so changing a bunch of style to 80c just adds code churn. Can you upload this patch without the line-wrapping changes?

[Dirk Pranke]

Created attachment 75404 [details]
remove whitespace diffs / reformatting

[Dirk Pranke]

(In reply to comment #4)
> (From update of attachment 75356 [details])
> I agree with Eric. We've decided not to do 80c in webkit python code, so changing a bunch of style to 80c just adds code churn. Can you upload this patch without the line-wrapping changes?

Okay. Although I'm obviously in the 80c camp, I only was reformatting this stuff because the rest of the file had already been formatted to 80c, so the mix of < 80 and > 80 made things awkward (I figured consistency one way or another was better).

I've removed the line-wrapping, but I'm curious to your thoughts on the above?

[Eric Seidel (no email)]

I think you didn't mean to upload a squashed diff.

[Dirk Pranke]

Created attachment 75415 [details]
upload the incremental diff, not the whole diff

[Dirk Pranke]

(In reply to comment #7)
> I think you didn't mean to upload a squashed diff.

Whoops, you're right. Sorry! I was rushing out to meet someone for lunch. Fixed now.

[Tony Chang]

Comment on attachment 75415 [details]
upload the incremental diff, not the whole diff

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

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:117
> +    def start_worker(self, worker_number):
> +        thread = super(_MultiThreadedBroker, self).start_worker(worker_number)

thread = _MultiThreadedBroker.start_worker(self, worker_number) for explicitness

[Dirk Pranke]

Created attachment 75434 [details]
add port.default_worker_model, merge in fix from bug 50420, other review feedback

[Dirk Pranke]

(In reply to comment #10)
> (From update of attachment 75415 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75415&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/message_broker.py:117
> > +    def start_worker(self, worker_number):
> > +        thread = super(_MultiThreadedBroker, self).start_worker(worker_number)
> 
> thread = _MultiThreadedBroker.start_worker(self, worker_number) for explicitness

Actually, that would cause an infinite loop :) Correct version added.

[Dirk Pranke]

(In reply to comment #11)
> Created an attachment (id=75434) [details]
> add port.default_worker_model, merge in fix from bug 50420, other review feedback

This patch adds a way for us to do port-specific overrides to --worker-model, which I suspect we'll want as we stabilize this stuff and in case multiprocessing ends up not working that well on windows (or under cygwin).

[Csaba Osztrogonác]

http://trac.webkit.org/changeset/73222 has very strange commit log. (same as 73220) I think it might be a webkit-patch bug. Could you check it?

[Eric Seidel (no email)]

It's a problem when using git rebase.  It sometimes merges your changelog not to the top of the file.

We should have a SVN pre-commit hook, or webkit-patch step to validate that you're only adding to the top of the file (which is where we pull the commit message from).

[Dirk Pranke]

(In reply to comment #15)
> It's a problem when using git rebase.  It sometimes merges your changelog not to the top of the file.
> 
> We should have a SVN pre-commit hook, or webkit-patch step to validate that you're only adding to the top of the file (which is where we pull the commit message from).

In this case I think I was creating the patch using the -g foo..bar option, since I had a bunch of work on branches I was attempting to merge in. So I would bet this was user error, and not a bug in webkit-patch.

That said, I agree a presubmit check might be a good idea.

[Dirk Pranke]

Marking as WONTFIX - will re-split up the patches and re-land a different way.