Test failures on windows

[duncanmmacleod]

In trying to build a Conda package for Comet, I am seeing test failures on Windows with python 3.7.1. Extract from the the build log:

__________________ SpawnCommandProtocolTestCase.test_no_exec __________________
self = <Process pid=None>
reactor = <twisted.internet.selectreactor.SelectReactor object at 0x0000009DB8029E10>
protocol = <comet.handler.spawn.SpawnCommandProtocol object at 0x0000009DB9649A20>
command = '/not/a/real/executable', args = ['/not/a/real/executable']
environment = {b'7ZIP': b'"C:\\Program Files\\7-Zip\\7z.exe"', b'ALLUSERSPROFILE': b'C:\\ProgramData', b'APPDATA': b'C:\\Users\\appveyor\\AppData\\Roaming', b'APPVEYOR': b'True', ...}
path = None
    def __init__(self, reactor, protocol, command, args, environment, path):
        """
        Create a new child process.
        """
        _pollingfile._PollingTimer.__init__(self, reactor)
        BaseProcess.__init__(self, protocol)

        # security attributes for pipes
        sAttrs = win32security.SECURITY_ATTRIBUTES()
        sAttrs.bInheritHandle = 1

        # create the pipes which will connect to the secondary process
        self.hStdoutR, hStdoutW = win32pipe.CreatePipe(sAttrs, 0)
        self.hStderrR, hStderrW = win32pipe.CreatePipe(sAttrs, 0)
        hStdinR, self.hStdinW  = win32pipe.CreatePipe(sAttrs, 0)

        win32pipe.SetNamedPipeHandleState(self.hStdinW,
                                          win32pipe.PIPE_NOWAIT,
                                          None,
                                          None)

        # set the info structure for the new process.
        StartupInfo = win32process.STARTUPINFO()
        StartupInfo.hStdOutput = hStdoutW
        StartupInfo.hStdError  = hStderrW
        StartupInfo.hStdInput  = hStdinR
        StartupInfo.dwFlags = win32process.STARTF_USESTDHANDLES

        # Create new handles whose inheritance property is false
        currentPid = win32api.GetCurrentProcess()

        tmp = win32api.DuplicateHandle(currentPid, self.hStdoutR, currentPid, 0, 0,
                                       win32con.DUPLICATE_SAME_ACCESS)
        win32file.CloseHandle(self.hStdoutR)
        self.hStdoutR = tmp

        tmp = win32api.DuplicateHandle(currentPid, self.hStderrR, currentPid, 0, 0,
                                       win32con.DUPLICATE_SAME_ACCESS)
        win32file.CloseHandle(self.hStderrR)
        self.hStderrR = tmp

        tmp = win32api.DuplicateHandle(currentPid, self.hStdinW, currentPid, 0, 0,
                                       win32con.DUPLICATE_SAME_ACCESS)
        win32file.CloseHandle(self.hStdinW)
        self.hStdinW = tmp

        # Add the specified environment to the current environment - this is
        # necessary because certain operations are only supported on Windows
        # if certain environment variables are present.

        env = os.environ.copy()
        env.update(environment or {})
        newenv = {}
        for key, value in items(env):

            key = _fsdecode(key)
            value = _fsdecode(value)

            newenv[key] = value

        env = newenv

        # Make sure all the arguments are Unicode.
        args = [_fsdecode(x) for x in args]

        cmdline = quoteArguments(args)

        # The command, too, needs to be Unicode, if it is a value.
        command = _fsdecode(command) if command else command
        path = _fsdecode(path) if path else path

        # TODO: error detection here.  See #2787 and #4184.
        def doCreate():
            flags = win32con.CREATE_NO_WINDOW
            self.hProcess, self.hThread, self.pid, dwTid = win32process.CreateProcess(
                command, cmdline, None, None, 1, flags, env, path, StartupInfo)
        try:
>           doCreate()
..\_test_env\lib\site-packages\twisted\internet\_dumbwin32proc.py:227: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
    def doCreate():
        flags = win32con.CREATE_NO_WINDOW
        self.hProcess, self.hThread, self.pid, dwTid = win32process.CreateProcess(
>           command, cmdline, None, None, 1, flags, env, path, StartupInfo)
E       pywintypes.error: (3, 'CreateProcess', 'The system cannot find the path specified.')
..\_test_env\lib\site-packages\twisted\internet\_dumbwin32proc.py:225: error
During handling of the above exception, another exception occurred:
self = <comet.handler.test.test_spawn.SpawnCommandProtocolTestCase testMethod=test_no_exec>
    def test_no_exec(self):
        """
        Fail when the executable doesn't exist.
        """
        spawn = SpawnCommand("/not/a/real/executable")
>       return self.assertFailure(spawn(DummyEvent()), Exception)
..\_test_env\lib\site-packages\comet\handler\test\test_spawn.py:39: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
..\_test_env\lib\site-packages\comet\handler\spawn.py:64: in __call__
    env=os.environ
..\_test_env\lib\site-packages\twisted\internet\posixbase.py:353: in spawnProcess
    return Process(self, processProtocol, executable, args, env, path)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <Process pid=None>
reactor = <twisted.internet.selectreactor.SelectReactor object at 0x0000009DB8029E10>
protocol = <comet.handler.spawn.SpawnCommandProtocol object at 0x0000009DB9649A20>
command = '/not/a/real/executable', args = ['/not/a/real/executable']
environment = {b'7ZIP': b'"C:\\Program Files\\7-Zip\\7z.exe"', b'ALLUSERSPROFILE': b'C:\\ProgramData', b'APPDATA': b'C:\\Users\\appveyor\\AppData\\Roaming', b'APPVEYOR': b'True', ...}
path = None
    def __init__(self, reactor, protocol, command, args, environment, path):
        """
        Create a new child process.
        """
        _pollingfile._PollingTimer.__init__(self, reactor)
        BaseProcess.__init__(self, protocol)

        # security attributes for pipes
        sAttrs = win32security.SECURITY_ATTRIBUTES()
        sAttrs.bInheritHandle = 1

        # create the pipes which will connect to the secondary process
        self.hStdoutR, hStdoutW = win32pipe.CreatePipe(sAttrs, 0)
        self.hStderrR, hStderrW = win32pipe.CreatePipe(sAttrs, 0)
        hStdinR, self.hStdinW  = win32pipe.CreatePipe(sAttrs, 0)

        win32pipe.SetNamedPipeHandleState(self.hStdinW,
                                          win32pipe.PIPE_NOWAIT,
                                          None,
                                          None)

        # set the info structure for the new process.
        StartupInfo = win32process.STARTUPINFO()
        StartupInfo.hStdOutput = hStdoutW
        StartupInfo.hStdError  = hStderrW
        StartupInfo.hStdInput  = hStdinR
        StartupInfo.dwFlags = win32process.STARTF_USESTDHANDLES

        # Create new handles whose inheritance property is false
        currentPid = win32api.GetCurrentProcess()

        tmp = win32api.DuplicateHandle(currentPid, self.hStdoutR, currentPid, 0, 0,
                                       win32con.DUPLICATE_SAME_ACCESS)
        win32file.CloseHandle(self.hStdoutR)
        self.hStdoutR = tmp

        tmp = win32api.DuplicateHandle(currentPid, self.hStderrR, currentPid, 0, 0,
                                       win32con.DUPLICATE_SAME_ACCESS)
        win32file.CloseHandle(self.hStderrR)
        self.hStderrR = tmp

        tmp = win32api.DuplicateHandle(currentPid, self.hStdinW, currentPid, 0, 0,
                                       win32con.DUPLICATE_SAME_ACCESS)
        win32file.CloseHandle(self.hStdinW)
        self.hStdinW = tmp

        # Add the specified environment to the current environment - this is
        # necessary because certain operations are only supported on Windows
        # if certain environment variables are present.

        env = os.environ.copy()
        env.update(environment or {})
        newenv = {}
        for key, value in items(env):

            key = _fsdecode(key)
            value = _fsdecode(value)

            newenv[key] = value

        env = newenv

        # Make sure all the arguments are Unicode.
        args = [_fsdecode(x) for x in args]

        cmdline = quoteArguments(args)

        # The command, too, needs to be Unicode, if it is a value.
        command = _fsdecode(command) if command else command
        path = _fsdecode(path) if path else path

        # TODO: error detection here.  See #2787 and #4184.
        def doCreate():
            flags = win32con.CREATE_NO_WINDOW
            self.hProcess, self.hThread, self.pid, dwTid = win32process.CreateProcess(
                command, cmdline, None, None, 1, flags, env, path, StartupInfo)
        try:
            doCreate()
        except pywintypes.error as pwte:
            if not _invalidWin32App(pwte):
                # This behavior isn't _really_ documented, but let's make it
                # consistent with the behavior that is documented.
>               raise OSError(pwte)
E               OSError: (3, 'CreateProcess', 'The system cannot find the path specified.')
..\_test_env\lib\site-packages\twisted\internet\_dumbwin32proc.py:232: OSError

Have these errors been seen before? If its just that the test suite doesn't run on Windows, but the main library does, that can easily be worked around during conda builds.

[jdswinbank]

As far as I know, nobody has tried to use Comet with Windows before, so I'm not surprised that there are problems. Given that, I can't really comment on whether they are limited to the test suite or are also an issue elsewhere, but the Appveyor log gives me no confidence that they'd be limited to the tests.

I don't have a Windows system to test on, but I'll try spinning up a VM and see if I can work out how to use it...

[duncanmmacleod]

If you would prefer, I can restrict the Conda distribution to Unix-based systems?

[jdswinbank]

I'm happy to spend a Saturday afternoon seeing if I can fix these issues. However, I'm not really familiar with Windows, so I can't give you promises or an ETA. If you want to make progress in the short term, maybe go ahead and push out a Unix-only distribution, and I'll update this ticket (and make a new release) as and when I get fixes in for Windows.

[duncanmmacleod]

@jdswinbank, there's no rush on any of this, so please don't work on the weekend purely on my account. I'll push out a Unix-only release (probably finished on Monday) and will wait on another release for Windows support. My use case is Unix only anyway, its just that conda-forge CI gives you good Windows support with almost no effort.

[jdswinbank]

The current master branch should now pass (or skip, in 6 cases) all of its tests on Windows. Fancy giving that a shot? If that works for you, I'll cut a 3.1 release including these changes.

[duncanmmacleod]

@jdswinbank, I put together an appveyor Ci configuration for Comet in order to test your changes. All of the tests pass for me just run python -m twisted.trial comet on python>=3.5 (there's a problem building twisted for python3.4 on Windows, but that version is not supported any more, so I don't really care). Thanks for taking a look at this in the first place.

If you are interested in adding Appveyor to your CI lineup for Comet, I can easily open a PR to merge the configuration into the main repo.

[duncanmmacleod]

Hmm... it seems you already have an appveyor config that I didn't notice, so nevermind that last bit...

[jdswinbank]

Great — thanks for confirming. I'm travelling over the next couple of days, but I'll push out a new release when I get a moment.

(And yes, I set up Appveyor for Comet yesterday... but thanks for the offer!)

[duncanmmacleod]

Thanks @jdswinbank!

[duncanmmacleod]

@jdswinbank, just a prompt on this, I'm in no rush, but am likely to forget completely before too long, which would be a shame.

[jdswinbank]

Sorry for the delay! I just pushed out a release 3.1 — give it a try & let me know if it works for you.

[jdswinbank]

I've heard no screaming since 3.1 went out, so I'm assuming this is working. :-)