[PATCH] D14706: [lit] Implement support of per test timeout in lit.

[Dan Liew via llvm-commits]

Hi,

> The implementation sounds good to me -- I am fine having an initial version
> of this feature that only works with an extra dependency, if it at least
> helps to get the feature in place. It would still be nice to get a
> completely built in version, eventually, but I think it is more important to
> have a start than have it completely finished.

Sure. I'll try to address your minor comments, rebase on trunk, check
everything works then commit to trunk.

Would you like me to add the dependency (or optional dependency) to
the ``setup.py`` file for a PyPi?

> On that note, it might just be a matter of ensuring we spawn processes into
> their own process group and killing the full process group to get rid of the
> dependency, if you are interested in investigating.

That definitely works on non-windows platforms as I have used it my
own scripts. The problem here is windows which doesn't have progress
groups (at least in the POSIX sense). It looks like job objects as
mentioned in [1] might be the best option on Windows.

Right now the trouble is that the creation of a process and killing it
are completely separated from each other which will cause problems if
we need a platform specific way of creating and destroying processes.
I think what we need to do is create a thin-wrapper around
``subprocess.Popen`` that has the methods we need and has an extra
``killProcessAndChildren()`` method which can call on a test timeout.
We can have the implementation of the wrapper depend on the platform.
On non-windows platforms we can just use ``subprocess.Popen`` with
``os.killpg()``. For windows platforms it would be easier initially to
just use psutil to kill the processes. Someone at a later date could
tackle the Windows problem...

I'd happily take a look at this as extra commits.

> My only comment on the implementation is that I would prefer not to have the
> timeout line printed following the header on each invocation. I don't think
> that is important enough info to merit always being output.

Sure. That was for my debugging. I'll remove it.

> I'm also not
> sure it should be a warning if the user overrides the timeout on the command
> line, honoring that seems expected behavior.

Sure I'll downgrade that to a note rather than it being a warning.

[1] http://stackoverflow.com/questions/1230669/subprocess-deleting-child-processes-in-windows

Thanks,
Dan.