<div dir="ltr">

<p class="MsoNormal">> Did you confirm that it’s not?</p>

<p class="MsoNormal">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.</p>

<p class="MsoNormal"> </p>

<p class="MsoNormal">> Also, did you test this with both Python 2 and Python
3?</p>

<p class="MsoNormal">I will check this once I update the diff function.</p>

</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 7, 2017 at 2:55 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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?<br><br>Also, did you test this with both Python 2 and Python 3?<div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Tue, Nov 7, 2017 at 6:30 AM Ying Yi via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">MaggieYi added a comment.<br>
<br>
<br>
<br>
> 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?<br>
<br>
I think it tests the lit builtin command for the following reasons:<br>
<br>
1. Lit tests enables the use of LIT’s internal shell in ShTests (\llvm\utils\lit\tests\lit.<wbr>cfg) Line 14: config.test_format = lit.formats.ShTest(execute_<wbr>external=False) Lit tests use internal shell instead of using external shell.<br>
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.<br>
3. Since all the failed tests print the builtins commands error information, the successful tests should be run using lit builtins shell.<br>
<br>
What do you think?<br>
<br>
> 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).<br>
<br>
Thanks, I have updated the patch to solve this issue.<br>
<br>
<br>
<br>
================<br>
Comment at: lit/lit/TestRunner.py:566-569<br>
+    if cmd.commands[0].args[0] == 'mkdir':<br>
+        if len(cmd.commands) != 1:<br>
+            raise InternalShellError(cmd.<wbr>commands[0], "Unsupported: 'mkdir' "<br>
+                                     "cannot be part of a pipeline")<br>
----------------<br>
rnk wrote:<br>
> 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.<br>
It is a good idea.<br>
<br>
<br>
================<br>
Comment at: lit/lit/TestRunner.py:573-576<br>
+    if cmd.commands[0].args[0] == 'diff':<br>
+        if len(cmd.commands) != 1:<br>
+            raise InternalShellError(cmd.<wbr>commands[0], "Unsupported: 'diff' "<br>
+                                     "cannot be part of a pipeline")<br>
----------------<br>
rnk wrote:<br>
> I'd be surprised if this isn't used somewhere.<br>
I'm sorry, I'm not sure about this. Could you possibly give me an explanation? Thanks.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D39567" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D39567</a><br>
<br>
<br>
<br>
</blockquote></div>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><font size="2"><span style="font-family:arial,helvetica,sans-serif"></span></font><font face="Calibri" size="2"><span style="font-size:11pt"><font size="2"><span style="font-size:10pt"><font color="#1F497D" face="Arial">Ying Yi<br>SN Systems - Sony Interactive Entertainment</font></span></font></span></font><br></div></div></div></div></div></div></div></div>
</div>