Bug 38718 – test-webkitpy fails under cygwin

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

[WebKit Review Bot]

test-webkitpy fails under cygwin

[Eric Seidel (no email)]

*** Bug 38716 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Created attachment 55337 [details]
work in progress

[Eric Seidel (no email)]

Created attachment 55341 [details]
Patch

[Eric Seidel (no email)]

Created attachment 55346 [details]
Fix default_configuration for Windows (and Qt)

[Adam Barth]

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.

[Eric Seidel (no email)]

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. ;)

[Adam Barth]

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.

[Eric Seidel (no email)]

Comment on attachment 55346 [details]
Fix default_configuration for Windows (and Qt)

Will post a new patch.

[Eric Seidel (no email)]

Created attachment 55381 [details]
Updated patch for testing on windows

[Eric Seidel (no email)]

Created attachment 55386 [details]
Updated patch for testing on windows

[Ojan Vafai]

> +++ 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?

[Eric Seidel (no email)]

(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.

[Ojan Vafai]

> > > +        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?

[Eric Seidel (no email)]

Created attachment 55392 [details]
Final patch, addresses Ojan's comments and fixes everything except NRWT --platform dryrun on win

[Eric Seidel (no email)]

Created attachment 55393 [details]
Final patch, addresses Ojan's comments and fixes everything except NRWT --platform dryrun on win

[Ojan Vafai]

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.

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

Created attachment 55447 [details]
Final patch, address Ojan's last two comments.

[Dirk Pranke]

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

[Eric Seidel (no email)]

(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?

[Dirk Pranke]

(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.

[Eric Seidel (no email)]

Created attachment 55459 [details]
Fix comment for Dirk

[Eric Seidel (no email)]

This should be a pretty much no-brainer r+ that this point. :)  Anybody?

[Daniel Bates]

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.

[Eric Seidel (no email)]

Committed r59000: <http://trac.webkit.org/changeset/59000>

[Eric Seidel (no email)]

Committed r59006: <http://trac.webkit.org/changeset/59006>

[Ojan Vafai]

> > 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?

[Adam Roben (:aroben)]

(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.