| Message ID | 442a4c351dea603e226bae89eddc2b3496d93262.1656044659.git.hanxin.hx@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph.c: no lazy fetch in lookup_commit_in_graph() | expand |
Han Xin <hanxin.hx@bytedance.com> writes: > We will use the lazy prerequisite ULIMIT_PROCESSES in a follow-up > commit. > > With run_with_limited_processses() we can limit forking subprocesses and > fail reliably in some test cases. > > Signed-off-by: Han Xin <hanxin.hx@bytedance.com> > --- > t/test-lib.sh | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 8ba5ca1534..f920e3b0ae 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -1816,6 +1816,15 @@ test_lazy_prereq ULIMIT_FILE_DESCRIPTORS ' > run_with_limited_open_files true > ' > > +run_with_limited_processses () { > + (ulimit -u 512 && "$@") The "-u" presumably is a way to say that the current user can have only 512 processes at once that is supported by bash and ksh? dash seems to use "-p" for this but "-p" of course means something completely different to other shells (and is read-only), which is a mess X-<. I suspect that it is OK to make it practically bash-only, but then ... > +} > + > +test_lazy_prereq ULIMIT_PROCESSES ' > + test_have_prereq !HPPA,!MINGW,!CYGWIN && > + run_with_limited_processses true ... as this lazy-prereq makes a trial run that would fail when the system does not allow "ulimit -u 512", do we need the platform specific prereq check? I am wondering if the second line alone is sufficient. Also, 512 is not a number I would exactly call "limit forking". Does it have to be so high, I wonder. Of course it cannot be so low like 3 or 8 or even 32, as per-user limitation counts your window manager and shells running in other windows. What you ideally want is an option that lets you limit the number of processes the shell that issued the ulimit call can spawn simultaneously, but I didn't find it in "man bash/dash/ksh". > +' > + > build_option () { > git version --build-options | > sed -ne "s/^$1: //p"
On Sat, Jun 25, 2022 at 12:03 AM Junio C Hamano <gitster@pobox.com> wrote: > > Han Xin <hanxin.hx@bytedance.com> writes: > > > We will use the lazy prerequisite ULIMIT_PROCESSES in a follow-up > > commit. > > > > With run_with_limited_processses() we can limit forking subprocesses and > > fail reliably in some test cases. > > > > Signed-off-by: Han Xin <hanxin.hx@bytedance.com> > > --- > > t/test-lib.sh | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/t/test-lib.sh b/t/test-lib.sh > > index 8ba5ca1534..f920e3b0ae 100644 > > --- a/t/test-lib.sh > > +++ b/t/test-lib.sh > > @@ -1816,6 +1816,15 @@ test_lazy_prereq ULIMIT_FILE_DESCRIPTORS ' > > run_with_limited_open_files true > > ' > > > > +run_with_limited_processses () { > > + (ulimit -u 512 && "$@") > > The "-u" presumably is a way to say that the current user can have > only 512 processes at once that is supported by bash and ksh? dash > seems to use "-p" for this but "-p" of course means something > completely different to other shells (and is read-only), which is a > mess X-<. > > I suspect that it is OK to make it practically bash-only, but then ... > > > +} > > + > > +test_lazy_prereq ULIMIT_PROCESSES ' > > + test_have_prereq !HPPA,!MINGW,!CYGWIN && > > + run_with_limited_processses true > > ... as this lazy-prereq makes a trial run that would fail when the > system does not allow "ulimit -u 512", do we need the platform > specific prereq check? I am wondering if the second line alone is > sufficient. > Yes，the second line alone is sufficient. > Also, 512 is not a number I would exactly call "limit forking". > Does it have to be so high, I wonder. Of course it cannot be so low > like 3 or 8 or even 32, as per-user limitation counts your window > manager and shells running in other windows. > It's hard to say. I've tried adjusting it to 256, but the test cases in next patch will always fail with the following "err": ./test-lib.sh: fork: Resource temporarily unavailable > What you ideally want is an option that lets you limit the number of > processes the shell that issued the ulimit call can spawn > simultaneously, but I didn't find it in "man bash/dash/ksh". > Maybe I should use "lib-bash.sh" instead of "test-lib.sh" just like t9902 and t9903? The different meanings of "-p" in bash and dash really make this tricky. Thanks. -Han Xin > > +' > > + > > build_option () { > > git version --build-options | > > sed -ne "s/^$1: //p"
Han Xin <hanxin.hx@bytedance.com> writes: > Maybe I should use "lib-bash.sh" instead of "test-lib.sh" just like t9902 > and t9903? > The different meanings of "-p" in bash and dash really make this tricky. I do not think lib-bash.sh is appropriate for this. t9902/9903 are about command line prompt and completion support _FOR_ bash users. By including lib-bash.sh, even if your "usual" shell is not bash, you can run these two scripts under bash, as long as you have it installed. For the purpose of testing these bash specific features, that framework makes quite a lot of sense. Those who are happy to have dash on their system without having to install bash would have no reason to see these two tests to pass, as they do not care about bash at all. What the test under discussion is doing is quite different. Instead of forcing to re-spawn bash when the user's shell is not bash, you'd want to adjust how you invoke "ulimit" if it is not bash, something like run_with_limited_processes () { (ulimit ${ulimit_max_process-"-p"} 512 && "$@") } test_lazy_prereq ULIMIT_PROCESSES ' # bash and ksh use "ulimit -u", dash uses "ulimit -p" if test -n "$BASH_VERSION" then ulimit_max_process="-u" elif test -n "$KSH_VERSION" then ulimit_max_process="-u" fi run_with_limited_processes true ' perhaps?
diff --git a/t/test-lib.sh b/t/test-lib.sh index 8ba5ca1534..f920e3b0ae 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1816,6 +1816,15 @@ test_lazy_prereq ULIMIT_FILE_DESCRIPTORS ' run_with_limited_open_files true ' +run_with_limited_processses () { + (ulimit -u 512 && "$@") +} + +test_lazy_prereq ULIMIT_PROCESSES ' + test_have_prereq !HPPA,!MINGW,!CYGWIN && + run_with_limited_processses true +' + build_option () { git version --build-options | sed -ne "s/^$1: //p"
We will use the lazy prerequisite ULIMIT_PROCESSES in a follow-up commit. With run_with_limited_processses() we can limit forking subprocesses and fail reliably in some test cases. Signed-off-by: Han Xin <hanxin.hx@bytedance.com> --- t/test-lib.sh | 9 +++++++++ 1 file changed, 9 insertions(+)