On 11/09/17 11:31, Adam Dinwoodie wrote: > On Sat, Sep 09, 2017 at 02:13:32PM +0100, Ramsay Jones wrote: >> I ran the test-suite on the 'pu' branch last night (simply because >> that was what I had built at the time!), which resulted in a PASS, >> but t6120 was showing a 'TODO passed' for #52. > > Confirmed, I also see this unexpected pass. > >> This is a test introduced by Michael's 'mg/name-rev-tests-with-short-stack' >> branch, which uses 'ulimit -s' to try and force a stack overflow. >> Unfortunately, 'ulimit -s' seems to have no effect on cygwin. I created >> a test program (see below) to eat up the stack and tried running it >> with various ulimit values (128, 12, 8), but it always seg-faulted >> at the same stack-frame. (after using approx 2MB stack space). > > Yes, Cygwin does not implement the ulimit API from the Single Unix > Specification, per https://cygwin.com/cygwin-api/std-notimpl.html. I Isn't that ulimit(3) not ulimit the bash built-in - which would more than likely be implemented by {get,set}rlimit(). (I haven't looked to find out, obviously!). > suspect the Bash builtin still exists because (a) nobody has bothered to > remove it, (b) including it avoids breaking scripts that assume it > exists but don't particularly rely on its output being sensical, or > (c) both. (d) it seems to provide useful information on some of the limits anyway. >> So, it looks like all ULIMIT_STACK_SIZE tests need to be disabled >> on cygwin. I also wonder about the ULIMIT_FILE_DESCRIPTORS tests, >> but haven't looked into it. >> >> Given that 'ulimit' is a bash built-in, this may also be a problem >> on MinGW and Git-For-Windows, but I can't test on those platforms. > > I'll leave this to the relevant folks to test; I don't have a useful > test environment for those either. That said, I'll note the comment in > t6120 says the ULIMIT_STACK_SIZE prerequisite excludes running the test > on Windows, so I assume it's not a problem there. I suspect that was a guess. ;-) > On Sun, Sep 10, 2017 at 05:58:49PM +0100, Ramsay Jones wrote: >> On 10/09/17 13:27, Michael J Gruber wrote: >>> So, I guess, short of testing the effect of "ulimit -s" with another >>> expensive test, it's best to simply set these prerequisites based on >>> "uname -s". >> >> Yes, I was going to suggest adding !CYGWIN to the test(s) (or perhaps >> to the 'test_lazy_prereq' expression(s)). > > Given the issue is Cygwin not implementing ulimit at all, but Cygwin > Bash pretending everything's fine, I think handling this as a special > case for Cygwin seems like the correct approach. Something like the > below, based on the existing code in test-lib.sh for the PIPE prereq? > > -- >8 -- > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 83f5d3dd2..376cd91ae 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -1166,14 +1166,32 @@ test_lazy_prereq UNZIP ' > test $? -ne 127 > ' > > +# On Cygwin, ulimit has no effect because the underlying API hasn't been > +# implemented. Just fail if trying to do something with ulimit. > run_with_limited_cmdline () { > - (ulimit -s 128 && "$@") > + case $(uname -s) in > + CYGWIN*) > + false > + ;; > + *) > + (ulimit -s 128 && "$@") > + ;; > + esac > } > > test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true' > > +# On Cygwin, ulimit has no effect because the underlying API hasn't been > +# implemented. Just fail if trying to do something with ulimit. > run_with_limited_stack () { > - (ulimit -s 128 && "$@") > + case $(uname -s) in > + CYGWIN*) > + false > + ;; > + *) > + (ulimit -s 128 && "$@") > + ;; > + esac > } > > test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true' > What about the other uses of ulimit? (No, I still haven't tried ulimit with open file descriptors - I will assume it doesn't work). I was thinking something more like this: $ git diff diff --git a/t/test-lib.sh b/t/test-lib.sh index 83f5d3dd2..6c939708a 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1009,6 +1009,11 @@ case $uname_s in GIT_TEST_CMP=mingw_test_cmp ;; *CYGWIN*) + # the ulimit bash built-in is ineffective + # but always succeeds, so always fail instead + ulimit () { + false + } test_set_prereq POSIXPERM test_set_prereq EXECKEEPSPID test_set_prereq CYGWIN $ This assumes that 'ulimit -n 32' fails to limit the number of open files, of course. ATB, Ramsay Jones
