Re: [PATCH] gnu: Add ninja.

[ludo@gnu.org (Ludovic Courtès)]

宋文武 <iyzsong@gmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> 宋文武 <iyzsong@gmail.com> skribis:
>>
>>> * gnu/packages/ninja.scm: New file.
>>> * gnu-system.am (GNU_SYSTEM_MODULES): Add it.
>>
>> [...]
>>
>>> +          'check
>>> +          (lambda _
>>> +            (and (zero? (system "./configure.py"))
>>> +                 (zero? (system "./ninja ninja_test"))
>>> +                 ;; SubprocessTest.InterruptChild fail when using 'system*'.
>>> +                 ;; SubprocessTest.SetWithLots was skipped.
>>> +                 ;; XXX: Raise [ulimit -n] well above 1025 to make this test go.
>>
>> Does it mean that the test is currently failing?
> Yes, SetWithLots fail with the 'Raise ...' line.

Oh, I see.  Then can you make it clearer in the comment:

  ;; SubprocessTest.SetWithLots fails with:
  ;;
  ;;   Raise [ulimit -n] well above 1025 to make this test go.
  ;;
  ;; Skip it.

>>> +                 (zero? (system (string-append
>>> +                                 "./ninja_test "
>>> +                                 "--gtest_filter="
>>> +                                 "-SubprocessTest.SetWithLots")))))
>>
>> Please use ‘system*’ (with separate arguments) rather than ‘system’.
>> The latter runs “/bin/sh -c ...” whereas the former runs the program
>> directly.
> Use 'system*' to run "./ninja_test" will cause InterruptChild to fail :(
> (as I mentioned in the comment)

Ah right, that’s weird, but could you mention in the comment (1) how it
fails, and (2) that because of this, we use ‘system’ instead of
‘system*’?

It’s important to explain both the problem and the solution/consequence,
otherwise it can be hard by reading the comment to understand if we’re
talking about an unfixed issue, a fixed issue, or a fix.

OK to push with these changes.

Thanks,
Ludo’.