Web lists-archives.com

Re: Unexpected pass for t6120-describe.sh on cygwin





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