Re: [PATCH] D39567: [lit] Implement non-pipelined ‘mkdir’, ‘diff’ and ‘rm’ commands internally

[Ying Yi via llvm-commits]

> Did you confirm that it’s not?

Thanks for your explanation. I confirm that diff doesn’t be used as part of
pipelined commands for both LLVM and Clang tests. However, some tests use
diff ‘-u’, ‘-b’ and ‘-w’ options. I need to update the built-in diff
function to support these options.



> Also, did you test this with both Python 2 and Python 3?

I will check this once I update the diff function.

On Tue, Nov 7, 2017 at 2:55 PM, Zachary Turner <zturner at google.com> wrote:

> I think he means that it’s likely diff is already being used as part of a
> pipeline (which would be broken by this). Did you confirm that it’s not?
>
> Also, did you test this with both Python 2 and Python 3?
>
> On Tue, Nov 7, 2017 at 6:30 AM Ying Yi via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> MaggieYi added a comment.
>>
>>
>>
>> > 1. I don't see any tests for the case where the commands *succeed*.
>> Only for the case where they fail.  Shouldn't we have some tests that
>> verify the commands actually work?
>>
>> I think it tests the lit builtin command for the following reasons:
>>
>> 1. Lit tests enables the use of LIT’s internal shell in ShTests
>> (\llvm\utils\lit\tests\lit.cfg) Line 14: config.test_format =
>> lit.formats.ShTest(execute_external=False) Lit tests use internal shell
>> instead of using external shell.
>> 2. The function _executeShCmd  returns once the shell builtins commands
>> finish. (line 571, 578 and 585 in TestRunner.py) They will not execute the
>> OS command.
>> 3. Since all the failed tests print the builtins commands error
>> information, the successful tests should be run using lit builtins shell.
>>
>> What do you think?
>>
>> > 2. Now that we have upwards of 6 builtin commands, I'm starting to
>> think they should be separated into their own file.  For example,
>> `lit/test/ShellBuiltins.py`  We could test them via unittests (e.g. see how
>> `BooleanExpression.py` implements its unit tests).
>>
>> Thanks, I have updated the patch to solve this issue.
>>
>>
>>
>> ================
>> Comment at: lit/lit/TestRunner.py:566-569
>> +    if cmd.commands[0].args[0] == 'mkdir':
>> +        if len(cmd.commands) != 1:
>> +            raise InternalShellError(cmd.commands[0], "Unsupported:
>> 'mkdir' "
>> +                                     "cannot be part of a pipeline")
>> ----------------
>> rnk wrote:
>> > Can you refactor the handling of non-pipelined commands, either in this
>> change, or split before or after? I'm imagining a dict of commands and
>> methods that implement builtin shell commands, which would then be ripe for
>> refactoring into a separate shell module or something.
>> It is a good idea.
>>
>>
>> ================
>> Comment at: lit/lit/TestRunner.py:573-576
>> +    if cmd.commands[0].args[0] == 'diff':
>> +        if len(cmd.commands) != 1:
>> +            raise InternalShellError(cmd.commands[0], "Unsupported:
>> 'diff' "
>> +                                     "cannot be part of a pipeline")
>> ----------------
>> rnk wrote:
>> > I'd be surprised if this isn't used somewhere.
>> I'm sorry, I'm not sure about this. Could you possibly give me an
>> explanation? Thanks.
>>
>>
>> https://reviews.llvm.org/D39567
>>
>>
>>
>>


-- 
Ying Yi
SN Systems - Sony Interactive Entertainment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171108/1cb2b98b/attachment.html>