Bug 38718 - test-webkitpy fails under cygwin
Summary: test-webkitpy fails under cygwin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 38716 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-05-06 21:24 PDT by WebKit Review Bot
Modified: 2010-05-10 06:30 PDT (History)
8 users (show)

See Also:


Attachments
work in progress (4.59 KB, patch)
2010-05-06 21:37 PDT, WebKit Review Bot
no flags Details | Formatted Diff | Diff
Patch (8.42 KB, patch)
2010-05-06 23:31 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix default_configuration for Windows (and Qt) (10.41 KB, patch)
2010-05-07 00:29 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Updated patch for testing on windows (10.69 KB, patch)
2010-05-07 09:07 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Updated patch for testing on windows (11.73 KB, patch)
2010-05-07 09:36 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Final patch, addresses Ojan's comments and fixes everything except NRWT --platform dryrun on win (12.17 KB, patch)
2010-05-07 10:07 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Final patch, addresses Ojan's comments and fixes everything except NRWT --platform dryrun on win (12.30 KB, patch)
2010-05-07 10:12 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Final patch, address Ojan's last two comments. (13.12 KB, patch)
2010-05-07 18:42 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix comment for Dirk (13.14 KB, patch)
2010-05-07 22:09 PDT, Eric Seidel (no email)
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2010-05-06 21:24:42 PDT
test-webkitpy fails under cygwin
Comment 1 Eric Seidel (no email) 2010-05-06 21:26:03 PDT
*** Bug 38716 has been marked as a duplicate of this bug. ***
Comment 2 WebKit Review Bot 2010-05-06 21:37:39 PDT
Created attachment 55337 [details]
work in progress
Comment 3 Eric Seidel (no email) 2010-05-06 23:31:05 PDT
Created attachment 55341 [details]
Patch
Comment 4 Eric Seidel (no email) 2010-05-07 00:29:31 PDT
Created attachment 55346 [details]
Fix default_configuration for Windows (and Qt)
Comment 5 Adam Barth 2010-05-07 00:48:27 PDT
Comment on attachment 55346 [details]
Fix default_configuration for Windows (and Qt)

WebKitTools/Scripts/webkitpy/common/system/executive.py:204
 +          if not extension:
foo.bar.exe probably fails here.
Comment 6 Eric Seidel (no email) 2010-05-07 00:51:34 PDT
You mean that we should be adding .exe to "foo.bar" and wouldn't?  I'm not sure that's a case I care to support. ;)
Comment 7 Adam Barth 2010-05-07 01:25:36 PDT
Yes.  I don't know what you mean by "care to support" but it's going to be a strange bug that bites us later.
Comment 8 Eric Seidel (no email) 2010-05-07 08:56:40 PDT
Comment on attachment 55346 [details]
Fix default_configuration for Windows (and Qt)

Will post a new patch.
Comment 9 Eric Seidel (no email) 2010-05-07 09:07:39 PDT
Created attachment 55381 [details]
Updated patch for testing on windows
Comment 10 Eric Seidel (no email) 2010-05-07 09:36:21 PDT
Created attachment 55386 [details]
Updated patch for testing on windows
Comment 11 Ojan Vafai 2010-05-07 09:46:52 PDT
> +++ b/WebKitTools/Scripts/webkitpy/common/system/executive.py
> @@ -33,6 +33,7 @@ try:
> +    # Executive unit tests uses this information to validate that
> +    # kill_process/kill_all work as expected an cause the expected exit codes.
> +    # We store this information in this file to keep it next to the platform ifs.
> +    # taskkill.exe causes processes to exit 0, otherwise processes exit -SIGNAL
> +    _KILL_PROCESS_KILLED_PROCESS_EXIT_CODE = 0 if sys.platform == "windows" else -signal.SIGKILL
> +    _KILL_ALL_KILLED_PROCESS_EXIT_CODE = 0 if sys.platform in ("windows", "cygwin") else -signal.SIGTERM

I don't really buy that it makes sense to put these here. This is just test data really. Anyone messing with this code will need to look at the tests as well. Anyways, not a big deal if you feel strongly.

> +        while retries_left:
> +            try:
> +                retries_left -= 1
> +                os.kill(pid, signal.SIGKILL)
> +            except OSError, e:
> +                if e.errno == errno.EAGAIN:
> +                    continue

Maybe we should raise if we get here and retries_left is 0? We should at least log something.

> +                if e.errno == errno.ESRCH:  # The process does not exist.
> +                    # FIXME: We should make non-silent failure an option.
> +                    return

I'm missing something, why can't we just log here?
Comment 12 Eric Seidel (no email) 2010-05-07 09:59:41 PDT
(In reply to comment #11)
> I don't really buy that it makes sense to put these here. This is just test
> data really. Anyone messing with this code will need to look at the tests as
> well. Anyways, not a big deal if you feel strongly.

I don't feel strongly.  But it's difficult to test those constants because they require testing on multiple platforms.  So I felt like putting them closer to the implementation might make them less likely to be changed to be wrong.

> > +        while retries_left:
> > +            try:
> > +                retries_left -= 1
> > +                os.kill(pid, signal.SIGKILL)
> > +            except OSError, e:
> > +                if e.errno == errno.EAGAIN:
> > +                    continue
> 
> Maybe we should raise if we get here and retries_left is 0? We should at least
> log something.

To prevent us from infinite looping you mean?  I can make it while retries_left > 0.

> > +                if e.errno == errno.ESRCH:  # The process does not exist.
> > +                    # FIXME: We should make non-silent failure an option.
> > +                    return

Sure, we could log.  The code which this function was based on assumed it was silent, but not in a "break it if it isn't silent" kind of way.
Comment 13 Ojan Vafai 2010-05-07 10:06:29 PDT
> > > +        while retries_left:
> > > +            try:
> > > +                retries_left -= 1
> > > +                os.kill(pid, signal.SIGKILL)
> > > +            except OSError, e:
> > > +                if e.errno == errno.EAGAIN:
> > > +                    continue
> > 
> > Maybe we should raise if we get here and retries_left is 0? We should at least
> > log something.
> 
> To prevent us from infinite looping you mean?  I can make it while retries_left
> > 0.

No. Does this mean something went wrong? I'm just saying we should log this as it ought to be useful debugging when stuff goes wrong.

> > > +                if e.errno == errno.ESRCH:  # The process does not exist.
> > > +                    # FIXME: We should make non-silent failure an option.
> > > +                    return
> 
> Sure, we could log.  The code which this function was based on assumed it was
> silent, but not in a "break it if it isn't silent" kind of way.

Yeah, seems like we should log in cases where things go wrong. This is an unexpected case, right?
Comment 14 Eric Seidel (no email) 2010-05-07 10:07:36 PDT
Created attachment 55392 [details]
Final patch, addresses Ojan's comments and fixes everything except NRWT --platform dryrun on win
Comment 15 Eric Seidel (no email) 2010-05-07 10:12:42 PDT
Created attachment 55393 [details]
Final patch, addresses Ojan's comments and fixes everything except NRWT --platform dryrun on win
Comment 16 Ojan Vafai 2010-05-07 10:35:59 PDT
Comment on attachment 55393 [details]
Final patch, addresses Ojan's comments and fixes everything except NRWT --platform dryrun on win

WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:301
 +      # Easy override for unit tests
Did you intend to write tests here? If not, I don't see the benefit of having this in a separate method.

WebKitTools/Scripts/webkitpy/layout_tests/port/win.py:51
 +              self._webkit_baseline_path("mac-snowleopard"),
It's weird that the win port checks mac-snowleopard but not mac-leopard/mac-tiger. I'd expect all of those or none of them. Anyways, this does match old-run-webkit tests. For consistency, do you mind making the same change to port/chromium_win.py? Or just file a bug. It's missing this one line.
Comment 17 Eric Seidel (no email) 2010-05-07 18:35:16 PDT
(In reply to comment #16)
> (From update of attachment 55393 [details])
> WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py:301
>  +      # Easy override for unit tests
> Did you intend to write tests here? If not, I don't see the benefit of having
> this in a separate method.

It's used by every unit test. :)

        port._open_configuration_file = lambda: WithAwareStringIO(file_contents)


> WebKitTools/Scripts/webkitpy/layout_tests/port/win.py:51
>  +              self._webkit_baseline_path("mac-snowleopard"),
> It's weird that the win port checks mac-snowleopard but not
> mac-leopard/mac-tiger. I'd expect all of those or none of them. Anyways, this
> does match old-run-webkit tests. For consistency, do you mind making the same
> change to port/chromium_win.py? Or just file a bug. It's missing this one line.

OK, I can add a FIXME.  I don't dare change chromium_win, as it might cause all sorts of failures with different fallback results.
Comment 18 Eric Seidel (no email) 2010-05-07 18:42:14 PDT
Created attachment 55447 [details]
Final patch, address Ojan's last two comments.
Comment 19 Dirk Pranke 2010-05-07 19:15:16 PDT
Comment on attachment 55447 [details]
Final patch, address Ojan's last two comments.

WebKitTools/Scripts/webkitpy/common/system/executive.py:184
 +              command = ["taskkill.exe", "/f", "/pid", pid]
This comment seems to conflict with the code 

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:60
 +          # FIMXE: This should include mac-snowleopard like win.py does.
No, I don't think it should, although it probably shouldn't fall back to win or mac at all, really. We should check to see if those directories are actually used.

Looks more-or-less right otherwise, although I can't swear to the windows-isms
Comment 20 Eric Seidel (no email) 2010-05-07 19:22:09 PDT
(In reply to comment #19)
> (From update of attachment 55447 [details])
> WebKitTools/Scripts/webkitpy/common/system/executive.py:184
>  +              command = ["taskkill.exe", "/f", "/pid", pid]
> This comment seems to conflict with the code 

I assume you mean this comment?
        if sys.platform == "windows":
            # We can't (easily) use taskkill.exe on cygwin because
            # subprocess.pid is a cygwin pid and taskkill expects a windows pid.
            # Thankfully os.kill on cygwin handles either pid type.

I was trying to explain why that wasn't "sys.platform in ("windows", "cygwin")" like how kill_all is done.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:60
>  +          # FIMXE: This should include mac-snowleopard like win.py does.
> No, I don't think it should, although it probably shouldn't fall back to win or
> mac at all, really. We should check to see if those directories are actually
> used.

I leave you and Ojan to duke it out.  I'm happy to keep or remove the FIXME.  I'll land it either way and someone later can fix/remove.

@abarth, @davelevin or @cjerdonek: care to review?
Comment 21 Dirk Pranke 2010-05-07 19:56:48 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 55447 [details] [details])
> > WebKitTools/Scripts/webkitpy/common/system/executive.py:184
> >  +              command = ["taskkill.exe", "/f", "/pid", pid]
> > This comment seems to conflict with the code 
> 
> I assume you mean this comment?
>         if sys.platform == "windows":
>             # We can't (easily) use taskkill.exe on cygwin because
>             # subprocess.pid is a cygwin pid and taskkill expects a windows
> pid.
>             # Thankfully os.kill on cygwin handles either pid type.
> 
> I was trying to explain why that wasn't "sys.platform in ("windows", "cygwin")"
> like how kill_all is done.
> 

Yeah, I was confused by the fact that "windows" and "cygwin" are not the same thing and was thinking that you meant to be calling os.kill() instead of taskkill.exe. Maybe there's some way you can' rewrite this to say something like "we can only use taskkill.exe on windows and not cygwin", to make it clear that you're talking about the branch you took, and not the branch you didn't take.
Comment 22 Eric Seidel (no email) 2010-05-07 22:09:35 PDT
Created attachment 55459 [details]
Fix comment for Dirk
Comment 23 Eric Seidel (no email) 2010-05-07 22:11:50 PDT
This should be a pretty much no-brainer r+ that this point. :)  Anybody?
Comment 24 Daniel Bates 2010-05-07 23:00:13 PDT
Comment on attachment 55459 [details]
Fix comment for Dirk

> +    # Executive unit tests uses this information to validate that
> +    # kill_process/kill_all work as expected an cause the expected exit codes.

Typo, "an" should be "and".

> +    # FIXME: This list may be incomplete as Apple has some sekret configs.

Looks like you intentionally misspelled "sekret" :-P.

Looks good to me.
Comment 25 Eric Seidel (no email) 2010-05-07 23:11:44 PDT
Committed r59000: <http://trac.webkit.org/changeset/59000>
Comment 26 Eric Seidel (no email) 2010-05-08 00:25:13 PDT
Committed r59006: <http://trac.webkit.org/changeset/59006>
Comment 27 Ojan Vafai 2010-05-08 07:30:47 PDT
> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:60
> >  +          # FIMXE: This should include mac-snowleopard like win.py does.
> > No, I don't think it should, although it probably shouldn't fall back to win or
> > mac at all, really. We should check to see if those directories are actually
> > used.
> 
> I leave you and Ojan to duke it out.  I'm happy to keep or remove the FIXME. 
> I'll land it either way and someone later can fix/remove.

chromium-win and webkit-win should use the same fallback directories. Hopefully we agree on that. :) It's confusing to me that webkit-win checks the mac-snowleopard directory, but given that it does, chromium-win should as well.

A change needs to be made to either chromium-win or webkit-win.

aroben, you have an opinion about the appropriate fallback directories for windows?
Comment 28 Adam Roben (:aroben) 2010-05-10 06:30:18 PDT
(In reply to comment #27)
> aroben, you have an opinion about the appropriate fallback directories for windows?

Apple's Windows port looks in mac-snowleopard because the versions of CG/CF/CFNetwork etc. that we use are based on the versions in SnowLeopard, and so we expect them to have (most of) the same SnowLeopard-specific quirks that they do on Mac.

I'm not sure what the right thing for Chromium is here.