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

[Zachary Turner via llvm-commits]

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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171107/f1ad0d66/attachment.html>