Re: [PATCH] test-lib: don't use ulimit in test prerequisites on cygwin

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 13.09.2017 21:20:
> Ramsay Jones wrote:
> 
>> On cygwin (and MinGW), the 'ulimit' built-in bash command does not have
>> the desired effect of limiting the resources of new processes, at least
>> for the stack and file descriptors. However, it always returns success
>> and leads to several test prerequisites being erroneously set to true.
>>
>> Add a check for cygwin and MinGW to the prerequisite expressions, using
>> 'uname -s', and return false instead of (indirectly) using 'ulimit'.
>> This affects the prerequisite expressions for the ULIMIT_STACK_SIZE,
>> CMDLINE_LIMIT and ULIMIT_FILE_DESCRIPTORS prerequisites.
>>
>> Signed-off-by: Ramsay Jones 
>> ---
>>  t/t1400-update-ref.sh | 11 ++-
>>  t/t6120-describe.sh   |  1 -
>>  t/t7004-tag.sh|  1 -
>>  t/test-lib.sh | 22 --
>>  4 files changed, 30 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Jonathan Nieder 
> 
> Thanks.
> 
> An alternative would be to do some more explicit feature-based test
> like using "ulimit" to set a limit and then reading back the limit in
> a separate process, but that feels like overkill.

It's still not clear whether these limits would be in effect or just
reported.

Rather than checking uname -s in several places, I think we should just
define ulimit to be false in one place, or rather set the prerequisite
there.

Michael

Re: [PATCH] test-lib: don't use ulimit in test prerequisites on cygwin

[Jonathan Nieder]

Ramsay Jones wrote:

> On cygwin (and MinGW), the 'ulimit' built-in bash command does not have
> the desired effect of limiting the resources of new processes, at least
> for the stack and file descriptors. However, it always returns success
> and leads to several test prerequisites being erroneously set to true.
>
> Add a check for cygwin and MinGW to the prerequisite expressions, using
> 'uname -s', and return false instead of (indirectly) using 'ulimit'.
> This affects the prerequisite expressions for the ULIMIT_STACK_SIZE,
> CMDLINE_LIMIT and ULIMIT_FILE_DESCRIPTORS prerequisites.
>
> Signed-off-by: Ramsay Jones 
> ---
>  t/t1400-update-ref.sh | 11 ++-
>  t/t6120-describe.sh   |  1 -
>  t/t7004-tag.sh|  1 -
>  t/test-lib.sh | 22 --
>  4 files changed, 30 insertions(+), 5 deletions(-)

Reviewed-by: Jonathan Nieder 

Thanks.

An alternative would be to do some more explicit feature-based test
like using "ulimit" to set a limit and then reading back the limit in
a separate process, but that feels like overkill.

Sincerely,
Jonathan

[PATCH] test-lib: don't use ulimit in test prerequisites on cygwin

[Ramsay Jones]

On cygwin (and MinGW), the 'ulimit' built-in bash command does not have
the desired effect of limiting the resources of new processes, at least
for the stack and file descriptors. However, it always returns success
and leads to several test prerequisites being erroneously set to true.

Add a check for cygwin and MinGW to the prerequisite expressions, using
'uname -s', and return false instead of (indirectly) using 'ulimit'.
This affects the prerequisite expressions for the ULIMIT_STACK_SIZE,
CMDLINE_LIMIT and ULIMIT_FILE_DESCRIPTORS prerequisites.

Signed-off-by: Ramsay Jones 
---

Hi Michael,

So, I finally got around to testing ulimit with open files and
found that it does not limit them on cygwin (it actually fails
on the hard limit of 3200). Having seen Johannes email earlier,
I took a chance and added MinGW to this patch as well. (Hopefully
this won't cause any screams!) ;-)

So, this patch was developed on top of the 'pu' branch, but it
also applies cleanly to your 'mg/name-rev-tests-with-short-stack'
branch (ie. on top of commit 31625b34c0).

If you are going to re-roll your branch, could you please add this
on top (after squashing the two comment deletions into your earlier
patches, maybe).

Thanks!

ATB,
Ramsay Jones

 t/t1400-update-ref.sh | 11 ++-
 t/t6120-describe.sh   |  1 -
 t/t7004-tag.sh|  1 -
 t/test-lib.sh | 22 --
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index dc98b4bc6..49415074f 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1253,7 +1253,16 @@ run_with_limited_open_files () {
(ulimit -n 32 && "$@")
 }
 
-test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
+test_lazy_prereq ULIMIT_FILE_DESCRIPTORS '
+   case $(uname -s) in
+   CYGWIN*|MINGW*)
+   false
+   ;;
+   *)
+   run_with_limited_open_files true
+   ;;
+   esac
+'
 
 test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating 
branches does not burst open file limit' '
 (
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index dd6dd9df9..3d45dc295 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -279,7 +279,6 @@ test_expect_success 'describe ignoring a borken submodule' '
grep broken out
 '
 
-# we require ulimit, this excludes Windows
 test_expect_failure ULIMIT_STACK_SIZE 'name-rev works in a deep repo' '
i=1 &&
while test $i -lt 8000
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 5bf5ace56..b545c33f8 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1863,7 +1863,6 @@ test_expect_success 'version sort with very long 
prerelease suffix' '
git tag -l --sort=version:refname
 '
 
-# we require ulimit, this excludes Windows
 test_expect_success ULIMIT_STACK_SIZE '--contains and --no-contains work in a 
deep repo' '
>expect &&
i=1 &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 83f5d3dd2..250ba5e9b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1170,13 +1170,31 @@ run_with_limited_cmdline () {
(ulimit -s 128 && "$@")
 }
 
-test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'
+test_lazy_prereq CMDLINE_LIMIT '
+   case $(uname -s) in
+   CYGWIN*|MINGW*)
+   false
+   ;;
+   *)
+   run_with_limited_cmdline true
+   ;;
+   esac
+'
 
 run_with_limited_stack () {
(ulimit -s 128 && "$@")
 }
 
-test_lazy_prereq ULIMIT_STACK_SIZE 'run_with_limited_stack true'
+test_lazy_prereq ULIMIT_STACK_SIZE '
+   case $(uname -s) in
+   CYGWIN*|MINGW*)
+   false
+   ;;
+   *)
+   run_with_limited_stack true
+   ;;
+   esac
+'
 
 build_option () {
git version --build-options |
-- 
2.14.0