From: Junio C Hamano <hidden> Date: 2021-11-09 23:34:22
Johannes Schindelin [off-list ref] writes:
I fear that it is a bit late in the -rc cycle to try to get this into the
official v2.34.0. Adam, since you are the maintainer of the Cygwin git
package, would you mind incorporating this patch into Cygwin's version of
Git?
I do not mind taking a Cygwin-only #ifdef block in compat/ like we
see below from folks who have stake in Cygwin, and who are clearly
leading Cygwin users on the list, like Ramsay and Adam are, even
after I tag -rc2.
I cannot give the change any better test than they can, and it is
their platform to improve, or break by accident while trying to do
so.
Thanks.
From: Johannes Schindelin <hidden> Date: 2021-11-10 12:28:16
Hi Junio,
On Tue, 9 Nov 2021, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
I fear that it is a bit late in the -rc cycle to try to get this into the
official v2.34.0. Adam, since you are the maintainer of the Cygwin git
package, would you mind incorporating this patch into Cygwin's version of
Git?
I do not mind taking a Cygwin-only #ifdef block in compat/ like we
see below from folks who have stake in Cygwin, and who are clearly
leading Cygwin users on the list, like Ramsay and Adam are, even
after I tag -rc2.
I cannot give the change any better test than they can, and it is
their platform to improve, or break by accident while trying to do
so.
Right. I tested this as well as I could, via the `--stress` option, and am
fairly confident that it is correct. Since the patch touches only
`simply-ipc` code, the only test that could possibly affected is t0052,
and it passes with `--stress` over here (when it failed without the
patch).
Ciao,
Dscho
P.S.: in case you wondered, no, I did not run the entire test suite. With
the performance characteristics of the POSIX emulation provided by the
Cygwin runtime, this would simply take too long. It's not the first time I
wish our test suite was more efficient, across _all_ supported platforms.
From: Adam Dinwoodie <hidden> Date: 2021-11-12 08:57:22
On Wed, 10 Nov 2021 at 12:28, Johannes Schindelin
[off-list ref] wrote:
Hi Junio,
On Tue, 9 Nov 2021, Junio C Hamano wrote:
quoted
Johannes Schindelin [off-list ref] writes:
quoted
I fear that it is a bit late in the -rc cycle to try to get this into the
official v2.34.0. Adam, since you are the maintainer of the Cygwin git
package, would you mind incorporating this patch into Cygwin's version of
Git?
I do not mind taking a Cygwin-only #ifdef block in compat/ like we
see below from folks who have stake in Cygwin, and who are clearly
leading Cygwin users on the list, like Ramsay and Adam are, even
after I tag -rc2.
I cannot give the change any better test than they can, and it is
their platform to improve, or break by accident while trying to do
so.
Right. I tested this as well as I could, via the `--stress` option, and am
fairly confident that it is correct. Since the patch touches only
`simply-ipc` code, the only test that could possibly affected is t0052,
and it passes with `--stress` over here (when it failed without the
patch).
Ciao,
Dscho
P.S.: in case you wondered, no, I did not run the entire test suite. With
the performance characteristics of the POSIX emulation provided by the
Cygwin runtime, this would simply take too long. It's not the first time I
wish our test suite was more efficient, across _all_ supported platforms.
I have just run the complete test suite on rc2, both with and without
this patch, and I can confirm it resolves this problem and doesn't
cause any other new test failures.
But yes, the (lack of) speed of running the Git test suite on Cygwin
is one of the reasons I run the tests on high-spec Azure VMs rather
than my own systems. Unfortunately the Cygwin compatibility layer plus
the overheads of NTFS mean things are unlikely to get significantly
quicker any time soon, and between WSL and Git for Windows, I expect
interest in improving Cygwin's performance is going to continue to
wane.
From: Ramsay Jones <hidden> Date: 2021-11-13 19:12:53
On 10/11/2021 12:27, Johannes Schindelin wrote:
[snip]
quoted
I cannot give the change any better test than they can, and it is
their platform to improve, or break by accident while trying to do
so.
Right. I tested this as well as I could, via the `--stress` option, and am
fairly confident that it is correct. Since the patch touches only
`simply-ipc` code, the only test that could possibly affected is t0052,
and it passes with `--stress` over here (when it failed without the
patch).
Ciao,
Dscho
P.S.: in case you wondered, no, I did not run the entire test suite. With
the performance characteristics of the POSIX emulation provided by the
Cygwin runtime, this would simply take too long. It's not the first time I
wish our test suite was more efficient, across _all_ supported platforms.
[I seem to have lost Adam's reply about this now being Hunky-Dory, but ...]
I ran the test-suite on -rc2 on thursday night (note _not_ rc2 + this patch)
and it deadlocked on me; I didn't notice for 4 hours, so (in the early hours)
I simply Ctl+C-ed it and went to bed. I haven't had the test-suite deadlock
for many many years - I've been spoilt! ;-)
I tried -rc2 again last night; this time it finished, but I gained another
test failure: t0301-credential-cache.sh. I have _never_ had this test fail
before, so that was unexpected. :(
[Yes, t0052-simple-ipc.sh failed as expected, since this patch was not
applied].
Also, I was half expecting a small speed-up due to the new pipe code in
v3.3.2 of the cygwin dll, but it actually took an hour longer than normal. :(
The only change to my setup, between -rc1 and -rc2, was the cygwin update
to v3.3.2, so this may point to some more fallout from the new pipe code
(maybe?).
Anyway, I haven't even looked at the new failure (see below), which we will
probably not have time to fix before release, so I am just now building
current master (v2.34.0-rc2-16-g5a73c6bdc7) to give that a try. (So, I won't
have anything to report until tomorrow).
Just FYI:
$ ./t0301-credential-cache.sh
...
not ok 13 - socket defaults to ~/.cache/git/credential/socket
#
# test_when_finished "
# git credential-cache exit &&
# rmdir -p .cache/git/credential/
# " &&
# test_path_is_missing "$HOME/.git-credential-cache" &&
# test_path_is_socket "$HOME/.cache/git/credential/socket"
#
...
not ok 26 - use custom XDG_CACHE_HOME if set and default sockets are not created
#
# test_when_finished "git credential-cache exit" &&
# test_path_is_socket "$XDG_CACHE_HOME/git/credential/socket" &&
# test_path_is_missing "$HOME/.git-credential-cache/socket" &&
# test_path_is_missing "$HOME/.cache/git/credential/socket"
#
not ok 27 - credential-cache --socket option overrides default location
#
# test_when_finished "
# git credential-cache exit --socket \"\$HOME/dir/socket\" &&
# rmdir \"\$HOME/dir\"
# " &&
# check approve "cache --socket \"\$HOME/dir/socket\"" <<-\EOF &&
# protocol=https
# host=example.com
# username=store-user
# password=store-pass
# EOF
# test_path_is_socket "$HOME/dir/socket"
#
not ok 28 - use custom XDG_CACHE_HOME even if xdg socket exists
#
# test_when_finished "
# git credential-cache exit &&
# sane_unset XDG_CACHE_HOME
# " &&
# check approve cache <<-\EOF &&
# protocol=https
# host=example.com
# username=store-user
# password=store-pass
# EOF
# test_path_is_socket "$HOME/.cache/git/credential/socket" &&
# XDG_CACHE_HOME="$HOME/xdg" &&
# export XDG_CACHE_HOME &&
# check approve cache <<-\EOF &&
# protocol=https
# host=example.com
# username=store-user
# password=store-pass
# EOF
# test_path_is_socket "$XDG_CACHE_HOME/git/credential/socket"
#
not ok 29 - use user socket if user directory exists
#
# test_when_finished "
# git credential-cache exit &&
# rmdir \"\$HOME/.git-credential-cache/\"
# " &&
# mkdir -p "$HOME/.git-credential-cache/" &&
# chmod 700 "$HOME/.git-credential-cache/" &&
# check approve cache <<-\EOF &&
# protocol=https
# host=example.com
# username=store-user
# password=store-pass
# EOF
# test_path_is_socket "$HOME/.git-credential-cache/socket"
#
not ok 30 - use user socket if user directory is a symlink to a directory
#
# test_when_finished "
# git credential-cache exit &&
# rmdir \"\$HOME/dir/\" &&
# rm \"\$HOME/.git-credential-cache\"
# " &&
# mkdir -p -m 700 "$HOME/dir/" &&
# ln -s "$HOME/dir" "$HOME/.git-credential-cache" &&
# check approve cache <<-\EOF &&
# protocol=https
# host=example.com
# username=store-user
# password=store-pass
# EOF
# test_path_is_socket "$HOME/.git-credential-cache/socket"
#
ok 31 - helper (cache --timeout=1) times out
# failed 6 among 31 test(s)
1..31
$
ATB,
Ramsay Jones
From: Ramsay Jones <hidden> Date: 2021-11-14 19:34:35
On 13/11/2021 19:11, Ramsay Jones wrote:
[snip]
The only change to my setup, between -rc1 and -rc2, was the cygwin update
to v3.3.2, so this may point to some more fallout from the new pipe code
(maybe?).
Anyway, I haven't even looked at the new failure (see below), which we will
probably not have time to fix before release, so I am just now building
current master (v2.34.0-rc2-16-g5a73c6bdc7) to give that a try. (So, I won't
have anything to report until tomorrow).
So, current 'master' fixes t0052-simple-ipc.sh, which is good, but the
t0301-credential-cache.sh test is still failing. Also, I can confirm
that cygwin v3.3.2 adds an additional hour to a test-suite run. :(
[ie it now takes 6 hours rather than 5 hours to run - I remember a time
when it used to only take 2 hours; those were the days!]
ATB,
Ramsay Jones
From: Adam Dinwoodie <hidden> Date: 2021-11-14 20:10:59
On Sat, 13 Nov 2021 at 19:11, Ramsay Jones [off-list ref] wrote:
On 10/11/2021 12:27, Johannes Schindelin wrote:
[snip]
quoted
quoted
I cannot give the change any better test than they can, and it is
their platform to improve, or break by accident while trying to do
so.
Right. I tested this as well as I could, via the `--stress` option, and am
fairly confident that it is correct. Since the patch touches only
`simply-ipc` code, the only test that could possibly affected is t0052,
and it passes with `--stress` over here (when it failed without the
patch).
Ciao,
Dscho
P.S.: in case you wondered, no, I did not run the entire test suite. With
the performance characteristics of the POSIX emulation provided by the
Cygwin runtime, this would simply take too long. It's not the first time I
wish our test suite was more efficient, across _all_ supported platforms.
[I seem to have lost Adam's reply about this now being Hunky-Dory, but ...]
I ran the test-suite on -rc2 on thursday night (note _not_ rc2 + this patch)
and it deadlocked on me; I didn't notice for 4 hours, so (in the early hours)
I simply Ctl+C-ed it and went to bed. I haven't had the test-suite deadlock
for many many years - I've been spoilt! ;-)
I tried -rc2 again last night; this time it finished, but I gained another
test failure: t0301-credential-cache.sh. I have _never_ had this test fail
before, so that was unexpected. :(
[Yes, t0052-simple-ipc.sh failed as expected, since this patch was not
applied].
Also, I was half expecting a small speed-up due to the new pipe code in
v3.3.2 of the cygwin dll, but it actually took an hour longer than normal. :(
The only change to my setup, between -rc1 and -rc2, was the cygwin update
to v3.3.2, so this may point to some more fallout from the new pipe code
(maybe?).
Anyway, I haven't even looked at the new failure (see below), which we will
probably not have time to fix before release, so I am just now building
current master (v2.34.0-rc2-16-g5a73c6bdc7) to give that a try. (So, I won't
have anything to report until tomorrow).
I'm seeing the same failure. It isn't caused by a change in Git --
I've rebuilt and re-run the test on old versions where that test was
passing, and it's now failing -- so this is clearly something in the
Cygwin environment. I've not investigated further, but it's clearly
caused by a Cygwin change rather than a Git change, so I don't think
there's any reason to hold up the Git release.
(I should probably report it on the Cygwin mailing list, but I haven't
got around to that yet...)
From: Johannes Schindelin <hidden> Date: 2021-11-16 11:03:44
Hi Adam,
On Fri, 12 Nov 2021, Adam Dinwoodie wrote:
[...] the (lack of) speed of running the Git test suite on Cygwin
is one of the reasons I run the tests on high-spec Azure VMs rather
than my own systems. Unfortunately the Cygwin compatibility layer plus
the overheads of NTFS mean things are unlikely to get significantly
quicker any time soon, and between WSL and Git for Windows, I expect
interest in improving Cygwin's performance is going to continue to
wane.
Well, at least from the Git point of view, there is still _some_ hope. At
the Git Contributor Summit, we talked (very, very briefly) about moving
parts of Git's test infrastructure from shell code to C.
This would definitely help not only save electricity (and we all _do_ have
to get used to the idea that we cannot continue spending as much energy as
we do right now) when running the CI/PR builds, but also accelerate
running the test suite on Cygwin (or for that matter, I suspect HP NonStop
to be helped tremendously, too).
Ciao,
Dscho