[PATCH 3 of 3] killdaemons: read pids from file arguments if any

[Patrick Mézard]

Le 20/08/12 00:39, Mads Kiilerich a écrit :
> Patrick Mezard wrote, On 08/19/2012 09:22 PM:
>> If this change is accepted, the plan is to:
> 
> I see that as indication that you expect some feedback ;-)

Worse than that, I expect other people to push my patches if they are good enough.

>> # HG changeset patch
>> # User Patrick Mezard <patrick at mezard.eu>
>> # Date 1345403716 -7200
>> # Node ID f474bf5159f481399e55485769d774183d7cf120
>> # Parent  59dcbbaf61aee7f9b534454820ea00e1bf204c74
>> killdaemons: read pids from file arguments if any
> 
> (What component should be specified for changes like this that obviously shouldn't be mentioned in the release note? Shouldn't it be 'tests' or something starting with 'test'?)

"tests" if you want, but neither is going to be mentioned in any release notes anyway.

>> With no arguments, fallback to the previous behaviour of reading from
>> $DAEMON_PIDS.
> 
> I would rather drop the hardcoded handling of $DAEMON_PIDS and specify it explicitly on the command line where needed. Explicit is better than implicit.

OK, more work but I can do that.

>> On Windows, MinGW kill implementation does not seem to use system
>> process identifiers, and systematically fails when called in tests.
>> killdaemons.py could be a suitable replacement.
> 
> For that reason I think we should move the killdaemons functionality into hg as something like 'hg serve --kill $DAEMON_PIDS'.

The existence of killdaemons.py and hg serve --kill are orthogonal: killdaemons is used to kill more things than hg serve process, see inotify tests or tinyproxy etc. hg serve --kill is meant to terminate an existing hg serve instance, not to be a generic kill utility replacement. We could terminate hg serve instance with HTTP for instance.

So, I agree that hg serve --kill (or hg unserve or whatever), should exist, and this patch queue is a prerequisite, but it will not replace killdaemons.py everywhere.

> (That would however introduce a problem when we want to import this functionality from run-tests - just like we have a problem with importing hghave ... The best solution to both cases might be to do a simple cut'n'paste of the relevant lines of code.)

I will just do the cut and paste in the other direction.

>> test-http-branchmap.t is updated to be run by Windows test suite. The:
>>
>>    listening at http://*:$HGPORT1/
>>
>> line does not appear on Windows because the detached process can no
>> longer write on its parent streams. Grepping hg serve stdout directly
>> causes the parent process to never return and hang the test. This is a
>> bug, but I have no simple solution and prefer to pay this small price
>> and enable hg serve tests on Windows.
> 
> That should be a separate changeset.
> 
> (It seems like the reason 'listening' doesn't appear simply is
> * hg serve output is written to a file and thus doesn't appear immediately when starting hg serve
> * the line is grepped out when the content of that file is displayed.
> 
> It seems like there is a more important story to be told: First why we have to redirect stdout, and second why we have to grep.)

Yes, the problem is we have to redirect stdout because anything else makes the test hang on Windows. So you have to be able to run the test on Windows to illustrate that otherwise it is meaningless. Then you have to grep because the "listening at" line is generated by the server.init() run by the subprocess. On Windows it has no longer access to any standard stream (which is actually a problem too but... too many problems at once), so the line is discarded.

I can move this change before enabling the test on Windows, but that means "you cannot test it, just believe me".

>> - Check test-http-branchmap.t does not fail/hang on the buildbot
> 
>> - Convert all kill utility calls to killdaemons.py calls
>> - Add a rule in check-code.py to forbid kill calls
> 
> 'kill' is in some places used to kill sub processes where the pid come from $!. Shouldn't 'kill' still be used in these cases?

It would be replaced with:

  $ echo $! > stuff.pid
  $ killdaemons.py stuff.pid

I can grep only one instance of this in test-static.http.t.

>> - Possibly drop the 'server' rule from hghave.
> 
> Yes, it should have been named something like 'hg-serve-d-kill' ... and when we have a way to kill 'serve -d' processes on all platforms it can go away.

I propose a new "killdaemons" rule, so I can replace "serve" with it and progressively enable the tests.

--
Patrick Mézard