Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon
On Mon, Oct 15, 2018 at 03:12:11AM -0700, Johannes Schindelin via GitGitGadget
wrote:
> From: Johannes Schindelin
>
> This should be more reliable than the current method, and prepares the
> test suite for a consistent way to clean up before re-running the tests
> with different options.
This patch makes the test suite hang in all four Travis CI build jobs
with P4 installed without any of the P4 tests finishing.
Reverting this patch from the whole patch series makes it work again.
I've also tried to revert only this first hunk of the patch below,
because based on the comment I thought it's worth a try, but it didn't
really help. It did make a difference: the 300s watchdog timer
eventually kicked in, and then the test scripts could finish
successfully... but there are a lot of P4 test scripts, and with each
taking 300s the build job still timeouted.
All this may (or may not) be related to and be a different symptom of
the leftover p4d processes Luke mentioned. I couldn't reproduce any
of this on my machine so far.
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> index c27599474c..f4f5d7d296 100644
> --- a/t/lib-git-p4.sh
> +++ b/t/lib-git-p4.sh
> @@ -74,15 +74,6 @@ cli="$TRASH_DIRECTORY/cli"
> git="$TRASH_DIRECTORY/git"
> pidfile="$TRASH_DIRECTORY/p4d.pid"
>
> -# Sometimes "prove" seems to hang on exit because p4d is still running
> -cleanup () {
> - if test -f "$pidfile"
> - then
> - kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
> - fi
> -}
> -trap cleanup EXIT
> -
> # git p4 submit generates a temp file, which will
> # not get cleaned up if the submission fails. Don't
> # clutter up /tmp on the test machine.
Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon
Hi Luke,
On Mon, 15 Oct 2018, Luke Diamand wrote:
> On Mon, 15 Oct 2018 at 16:02, Johannes Schindelin
> wrote:
> >
> > Hi Luke,
> >
> > On Mon, 15 Oct 2018, Luke Diamand wrote:
> >
> > > On Mon, 15 Oct 2018 at 11:12, Johannes Schindelin via GitGitGadget
> > > wrote:
> > > >
> > > > From: Johannes Schindelin
> > > >
> > > > This should be more reliable than the current method, and prepares the
> > > > test suite for a consistent way to clean up before re-running the tests
> > > > with different options.
> > > >
> > >
> > > I'm finding that it's leaving p4d processes lying around.
> >
> > That's a bummer!
> >
> > > e.g.
> > >
> > > $ ./t9820-git-p4-editor-handling.sh
> > >
> > > $ ./t9820-git-p4-editor-handling.sh
> > >
> >
> > Since I do not currently have a setup with p4d installed, can you run that
> > with `sh -x ...` and see whether this code path is hit?
>
> All you need to do is to put p4 and p4d in your PATH.
>
> https://www.perforce.com/downloads/helix-core-p4d
> https://www.perforce.com/downloads/helix-command-line-client-p4
I did download p4d.exe and p4.exe to $HOME/custom/p4/ (similar to the way
ci/install-dependencies.sh does it), from
http://filehost.perforce.com/perforce/r18.1/bin.ntx64/p4d.exe
http://filehost.perforce.com/perforce/r18.1/bin.ntx64/p4.exe
and then prefixed PATH with $HOME/custom/p4, just to run t9820. However,
it does not work here at all:
-- snip --
[...]
++ git p4 clone '--dest=/usr/src/git/azure-pipelines/t/trash
directory.t9820-git-p4-editor-handling/git' //depot
Usage: git-p4 clone [options] //depot/path[@revRange]
[...]
-- snap --
I *think* the reason for that is that the MSYS path mangling kicks in when
`git.exe` is called with the `//depot` argument (the leading `//` can be
used in MSYS and MSYS2 to indicate that the non-MSYS .exe wants an
argument that starts with a single slash, but is not a path that needs to
be translated to a Windows path).
So I did the next best thing I could do: try things in the Windows
Subsystem for Linux (AKA Bash on Ubuntu on Windows).
> The server is free to use for a small number of users, you don't need
> to do anything to make it go.
>
>
> >
> > test_done () {
> > GIT_EXIT_OK=t
> >
> > + test -n "$immediate" || test_atexit_handler
On a slight tangent: I made up my mind on the `test -n "$immediate"` part:
I will skip that. The daemon should be stopped also when `-i` was passed
to the test script (to stop at the first failing test case). There is
very, very little use in keeping the daemon alive in that case.
The commit message of "tests: introduce `test_atexit`" already made the
case for that, but somehow I had not made up my mind yet.
> > +
>
> + test -n
> + test_atexit_handler
> ./t9820-git-p4-editor-handling.sh: 764:
> ./t9820-git-p4-editor-handling.sh: test_atexit_handler: not found
>
> Is that expected?
No, that is not expected. For me, it looks quite a bit different, though:
-- snip --
[...]
+ test_done
+ GIT_EXIT_OK=t
+ test -n
+ test_atexit_handler
+ test : != { kill_p4d
} && (exit "$eval_ret"); eval_ret=$?; :
+ setup_malloc_check
+ MALLOC_CHECK_=3 MALLOC_PERTURB_=165
+ export MALLOC_CHECK_ MALLOC_PERTURB_
+ test_eval_ { kill_p4d
} && (exit "$eval_ret"); eval_ret=$?; :
+ test_eval_inner_ { kill_p4d
} && (exit "$eval_ret"); eval_ret=$?; :
+ eval
want_trace && set -x
{ kill_p4d
} && (exit "$eval_ret"); eval_ret=$?; :
+ want_trace
+ test = t
+ kill_p4d
+ cat /home/wsl/git/wip/t/trash directory.t9820-git-p4-editor-handling/p4d.pid
+ pid=20093
+ retry_until_fail kill 20093
+ time_in_seconds
+ cd /
+ /usr/bin/python -c import time; print(int(time.time()))
+ timeout=1539681637
+ kill 20093
+ time_in_seconds
+ cd /
+ /usr/bin/python -c import time; print(int(time.time()))
+ test 1539681577 -gt 1539681637
+ sleep 1
+ true
+ time_in_seconds
+ cd /
+ /usr/bin/python -c import time; print(int(time.time()))
+ test 1539681578 -gt 1539681871
+ sleep 1
+ kill 20093
+ retry_until_fail kill -9 20093
+ time_in_seconds
+ cd /
+ /usr/bin/python -c import time; print(int(time.time()))
+ timeout=1539681638
+ kill -9 20093
+ test_must_fail kill 20093
+ _test_ok=
+ kill 20093
+ exit_code=1
+ test 1 -eq 0
+ test_match_signal 13 1
+ test 1 = 141
+ test 1 = 269
+ return 1
+ test 1 -gt 129
+ test 1 -eq 127
+ test 1 -eq 126
+ return 0
[...]
-- snap --
As you can see, it works quite well over here.
Maybe I could trouble you to fetch the `vsts-ci` branch from
https://github.com/dscho/git and test things on that one, just in case
that anything important got "lost in the mail"?
> > if test -n "$write_junit_xml" && test -n "$junit_xml_path"
> > then
> >
> > > And also
> > >
> > > $ ./t9800-git-p4-basic.sh
> > >
> > > Ctrl-C
> >
> > Oh, you're right.
So I tested this in WSL, and the relevant part of the output is this:
-- snip --
[...]
+ eval test_prereq_lazily_UTF8_NFD_TO_NFC=$2
+ test_prereq
Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon
Hi Eric,
On Mon, 15 Oct 2018, Eric Sunshine wrote:
> On Mon, Oct 15, 2018 at 6:12 AM Johannes Schindelin via GitGitGadget
> wrote:
> > This should be more reliable than the current method, and prepares the
> > test suite for a consistent way to clean up before re-running the tests
> > with different options.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > diff --git a/t/t-basic.sh b/t/t-basic.sh
> > @@ -134,6 +134,7 @@ check_sub_test_lib_test_err () {
> > +cat >/dev/null <<\DDD
> > test_expect_success 'pretend we have a fully passing test suite' "
> > run_sub_test_lib_test full-pass '3 passing tests' <<-\\EOF &&
> > for i in 1 2 3
> > @@ -820,6 +821,7 @@ test_expect_success 'tests clean up even on failures' "
> > > 1..2
> > EOF
> > "
> > +DDD
>
> Is this "DDD" here-doc leftover debugging goop?
Oy, oy, oy. This is definitely a left-over from debugging (as you can
imagine, it is pretty slow to run t-init.sh on Windows, and if I add a
test case, the development cycle is much faster with the trick you see
aboive). This left-over even made it into Git for Windows' `master`
branch! (Which is the reason I missed it before contributing v2).
Will fix,
Dscho
Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon
On Mon, Oct 15, 2018 at 6:12 AM Johannes Schindelin via GitGitGadget
wrote:
> This should be more reliable than the current method, and prepares the
> test suite for a consistent way to clean up before re-running the tests
> with different options.
>
> Signed-off-by: Johannes Schindelin
> ---
> diff --git a/t/t-basic.sh b/t/t-basic.sh
> @@ -134,6 +134,7 @@ check_sub_test_lib_test_err () {
> +cat >/dev/null <<\DDD
> test_expect_success 'pretend we have a fully passing test suite' "
> run_sub_test_lib_test full-pass '3 passing tests' <<-\\EOF &&
> for i in 1 2 3
> @@ -820,6 +821,7 @@ test_expect_success 'tests clean up even on failures' "
> > 1..2
> EOF
> "
> +DDD
Is this "DDD" here-doc leftover debugging goop?
Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon
On Mon, 15 Oct 2018 at 16:02, Johannes Schindelin
wrote:
>
> Hi Luke,
>
> On Mon, 15 Oct 2018, Luke Diamand wrote:
>
> > On Mon, 15 Oct 2018 at 11:12, Johannes Schindelin via GitGitGadget
> > wrote:
> > >
> > > From: Johannes Schindelin
> > >
> > > This should be more reliable than the current method, and prepares the
> > > test suite for a consistent way to clean up before re-running the tests
> > > with different options.
> > >
> >
> > I'm finding that it's leaving p4d processes lying around.
>
> That's a bummer!
>
> > e.g.
> >
> > $ ./t9820-git-p4-editor-handling.sh
> >
> > $ ./t9820-git-p4-editor-handling.sh
> >
>
> Since I do not currently have a setup with p4d installed, can you run that
> with `sh -x ...` and see whether this code path is hit?
All you need to do is to put p4 and p4d in your PATH.
https://www.perforce.com/downloads/helix-core-p4d
https://www.perforce.com/downloads/helix-command-line-client-p4
The server is free to use for a small number of users, you don't need
to do anything to make it go.
>
> test_done () {
> GIT_EXIT_OK=t
>
> + test -n "$immediate" || test_atexit_handler
> +
+ test -n
+ test_atexit_handler
./t9820-git-p4-editor-handling.sh: 764:
./t9820-git-p4-editor-handling.sh: test_atexit_handler: not found
Is that expected?
> if test -n "$write_junit_xml" && test -n "$junit_xml_path"
> then
>
> > And also
> >
> > $ ./t9800-git-p4-basic.sh
> >
> > Ctrl-C
>
> Oh, you're right. I think I need to do something in this line:
>
> trap 'exit $?' INT
>
> in t/test-lib.sh, something like
>
> trap 'exit_code=$?; test_atexit_handler; exit $exit_code' INT
>
> would you agree? (And: could you test?)
Not sure.
Send me a patch and I can try it out.
Thanks,
Luke
Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon
Hi Luke,
On Mon, 15 Oct 2018, Luke Diamand wrote:
> On Mon, 15 Oct 2018 at 11:12, Johannes Schindelin via GitGitGadget
> wrote:
> >
> > From: Johannes Schindelin
> >
> > This should be more reliable than the current method, and prepares the
> > test suite for a consistent way to clean up before re-running the tests
> > with different options.
> >
>
> I'm finding that it's leaving p4d processes lying around.
That's a bummer!
> e.g.
>
> $ ./t9820-git-p4-editor-handling.sh
>
> $ ./t9820-git-p4-editor-handling.sh
>
Since I do not currently have a setup with p4d installed, can you run that
with `sh -x ...` and see whether this code path is hit?
test_done () {
GIT_EXIT_OK=t
+ test -n "$immediate" || test_atexit_handler
+
if test -n "$write_junit_xml" && test -n "$junit_xml_path"
then
> And also
>
> $ ./t9800-git-p4-basic.sh
>
> Ctrl-C
Oh, you're right. I think I need to do something in this line:
trap 'exit $?' INT
in t/test-lib.sh, something like
trap 'exit_code=$?; test_atexit_handler; exit $exit_code' INT
would you agree? (And: could you test?)
Thanks,
Dscho
> $ ./t9800-git-p4-basic.sh
>
>
> $ ps | grep p4d
> 21392 pts/100:00:00 p4d <
>
>
> Luke
>
Re: [PATCH v2 09/13] git-p4: use `test_atexit` to kill the daemon
On Mon, 15 Oct 2018 at 11:12, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin > > This should be more reliable than the current method, and prepares the > test suite for a consistent way to clean up before re-running the tests > with different options. > I'm finding that it's leaving p4d processes lying around. e.g. $ ./t9820-git-p4-editor-handling.sh $ ./t9820-git-p4-editor-handling.sh And also $ ./t9800-git-p4-basic.sh Ctrl-C $ ./t9800-git-p4-basic.sh $ ps | grep p4d 21392 pts/100:00:00 p4d < Luke
