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

From: Adam Dinwoodie <adam@dinwoodie.org>
Date: Sep 11, 2017, 10:31 UTC
Message-ID: <20170911103142.GH10378@dinwoodie.org>
In-Reply-To: <7c3db153-2a56-f27d-af71-e4b61f1252a1@ramsayjones.plus.com>

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.

Show 6 quoted lines

> 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 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.

Show 6 quoted lines

> 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.

On Sun, Sep 10, 2017 at 05:58:49PM +0100, Ramsay Jones wrote:

Show 7 quoted lines

> 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'