[lit] Echo full RUN lines in case of external shells #66408

jdenny-ornl · 2023-09-14T17:41:55Z

NOTE: The first two commits in this PR (a398f3e1dfbfc83489e4d85fd74e8b3f12d469e9 and e4c20b93fa8d49ac22f4493358a8e3c8102eaf51) should not be reviewed here. They contain D154984 and D156954, which will be re-landed at the same time as this PR. Please review the third commit (d71c72c9f20ed34dc2fe2d0ec849a6e44dbf9d20): "[lit] Echo full RUN lines in case of external shells".

Before https://reviews.llvm.org/D154984 and https://reviews.llvm.org/D156954, lit reported full RUN lines in a Script: section. Now, in the case of lit's internal shell, it's the execution trace that includes them. However, if lit is configured to use an external shell (e.g., bash, windows cmd), they aren't reported at all.

A fix was requested at the following:

This patch does not address the case when the external shell is windows cmd. As discussed at #65242, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.

llvmbot · 2023-09-14T17:43:05Z

@llvm/pr-subscribers-testing-tools

@llvm/pr-subscribers-libcxx

Changes

**NOTE: The first two commits in this PR should not be reviewed. They contain [D154984](https://reviews.llvm.org/D154984) and [D156954](https://reviews.llvm.org/D156954), which will be re-landed at the same time as this commit.**

Before https://reviews.llvm.org/D154984 and https://reviews.llvm.org/D156954, lit reported full RUN lines in a Script: section. Now, in the case of lit's internal shell, it's the execution trace that includes them. However, if lit is configured to use an external shell (e.g., bash, windows cmd), they aren't reported at all.

A fix was requested at the following:

This patch does not address the case when the external shell is windows `cmd`. As discussed at #65242, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.

Patch is 134.19 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66408.diff

42 Files Affected:

(modified) libcxx/utils/libcxx/test/dsl.py (+1-1)
(modified) libcxx/utils/libcxx/test/format.py (+3-46)
(modified) llvm/docs/CommandGuide/lit.rst (+8-10)
(modified) llvm/utils/lit/lit/LitConfig.py (-2)
(modified) llvm/utils/lit/lit/TestRunner.py (+139-42)
(modified) llvm/utils/lit/lit/cl_arguments.py (+6-8)
(modified) llvm/utils/lit/lit/main.py (-1)
(modified) llvm/utils/lit/tests/Inputs/lit-opts/lit.cfg (+1-1)
(modified) llvm/utils/lit/tests/Inputs/shtest-define/examples/param-subst.txt (+4-4)
(modified) llvm/utils/lit/tests/Inputs/shtest-define/expansion-order.txt (+4-4)
(modified) llvm/utils/lit/tests/Inputs/shtest-define/line-number-substitutions.txt (+5-5)
(modified) llvm/utils/lit/tests/Inputs/shtest-define/name-chars.txt (+6-6)
(modified) llvm/utils/lit/tests/Inputs/shtest-define/recursiveExpansionLimit.txt (+2-2)
(modified) llvm/utils/lit/tests/Inputs/shtest-define/value-equals.txt (+4-4)
(modified) llvm/utils/lit/tests/Inputs/shtest-define/value-escaped.txt (+2-2)
(modified) llvm/utils/lit/tests/Inputs/shtest-define/ws-and-continuations.txt (+17-17)
(added) llvm/utils/lit/tests/Inputs/shtest-external-shell-kill/lit.cfg (+5)
(added) llvm/utils/lit/tests/Inputs/shtest-external-shell-kill/test.txt (+5)
(modified) llvm/utils/lit/tests/Inputs/shtest-if-else/test.txt (+40-24)
(modified) llvm/utils/lit/tests/Inputs/shtest-output-printing/basic.txt (+2-1)
(modified) llvm/utils/lit/tests/Inputs/shtest-output-printing/lit.cfg (+1)
(added) llvm/utils/lit/tests/Inputs/shtest-output-printing/write-a-lot.py (+6)
(added) llvm/utils/lit/tests/Inputs/shtest-run-at-line/external-shell/empty-run-line.txt (+3)
(added) llvm/utils/lit/tests/Inputs/shtest-run-at-line/internal-shell/empty-run-line.txt (+3)
(added) llvm/utils/lit/tests/Inputs/shtest-shell/echo-at-redirect-stderr.txt (+1)
(added) llvm/utils/lit/tests/Inputs/shtest-shell/echo-at-redirect-stdin.txt (+2)
(added) llvm/utils/lit/tests/Inputs/shtest-shell/echo-redirect-stderr.txt (+1)
(added) llvm/utils/lit/tests/Inputs/shtest-shell/echo-redirect-stdin.txt (+2)
(modified) llvm/utils/lit/tests/allow-retries.py (+24-5)
(modified) llvm/utils/lit/tests/lit-opts.py (+2-2)
(modified) llvm/utils/lit/tests/shtest-define.py (+8-8)
(modified) llvm/utils/lit/tests/shtest-env.py (+79-52)
(added) llvm/utils/lit/tests/shtest-external-shell-kill.py (+36)
(modified) llvm/utils/lit/tests/shtest-format.py (+43-29)
(modified) llvm/utils/lit/tests/shtest-if-else.py (+2-1)
(modified) llvm/utils/lit/tests/shtest-inject.py (+20-25)
(modified) llvm/utils/lit/tests/shtest-not.py (+132-85)
(modified) llvm/utils/lit/tests/shtest-output-printing.py (+39-25)
(modified) llvm/utils/lit/tests/shtest-pushd-popd.py (+6-6)
(modified) llvm/utils/lit/tests/shtest-recursive-substitution.py (+3-3)
(modified) llvm/utils/lit/tests/shtest-run-at-line.py (+62-44)
(modified) llvm/utils/lit/tests/shtest-shell.py (+453-403)

diff --git a/libcxx/utils/libcxx/test/dsl.py b/libcxx/utils/libcxx/test/dsl.py
index 847cebf5962f6aa..4c2217ec31428b2 100644
--- a/libcxx/utils/libcxx/test/dsl.py
+++ b/libcxx/utils/libcxx/test/dsl.py
@@ -180,7 +180,7 @@ def programOutput(config, program, args=None):
                 &quot;Failed to run program, cmd:\n{}\nstderr is:\n{}&quot;.format(runcmd, err)
             )
 
-        return libcxx.test.format._parseLitOutput(out)
+        return out
 
 
 @_memoizeExpensiveOperation(
diff --git a/libcxx/utils/libcxx/test/format.py b/libcxx/utils/libcxx/test/format.py
index ddd88f25646eaa8..c7c0bad681ddc8b 100644
--- a/libcxx/utils/libcxx/test/format.py
+++ b/libcxx/utils/libcxx/test/format.py
@@ -35,39 +35,6 @@ def _checkBaseSubstitutions(substitutions):
     for s in [&quot;%{cxx}&quot;, &quot;%{compile_flags}&quot;, &quot;%{link_flags}&quot;, &quot;%{flags}&quot;, &quot;%{exec}&quot;]:
         assert s in substitutions, &quot;Required substitution {} was not provided&quot;.format(s)
 
-def _parseLitOutput(fullOutput):
-    &quot;&quot;&quot;
-    Parse output of a Lit ShTest to extract the actual output of the contained commands.
-
-    This takes output of the form
-
-        $ &quot;:&quot; &quot;RUN: at line 11&quot;
-        $ &quot;echo&quot; &quot;OUTPUT1&quot;
-        # command output:
-        OUTPUT1
-
-        $ &quot;:&quot; &quot;RUN: at line 12&quot;
-        $ &quot;echo&quot; &quot;OUTPUT2&quot;
-        # command output:
-        OUTPUT2
-
-    and returns a string containing
-
-        OUTPUT1
-        OUTPUT2
-
-    as-if the commands had been run directly. This is a workaround for the fact
-    that Lit doesn&#x27;t let us execute ShTest and retrieve the raw output without
-    injecting additional Lit output around it.
-    &quot;&quot;&quot;
-    parsed = &#x27;&#x27;
-    for output in re.split(&#x27;[$]\s*&quot;:&quot;\s*&quot;RUN: at line \d+&quot;&#x27;, fullOutput):
-        if output: # skip blank lines
-            commandOutput = re.search(&quot;# command output:\n(.+)\n$&quot;, output, flags=re.DOTALL)
-            if commandOutput:
-                parsed += commandOutput.group(1)
-    return parsed
-
 def _executeScriptInternal(test, litConfig, commands):
     &quot;&quot;&quot;
     Returns (stdout, stderr, exitCode, timeoutInfo, parsedCommands)
@@ -79,21 +46,12 @@ def _executeScriptInternal(test, litConfig, commands):
     _, tmpBase = _getTempPaths(test)
     execDir = os.path.dirname(test.getExecPath())
     res = lit.TestRunner.executeScriptInternal(
-        test, litConfig, tmpBase, parsedCommands, execDir
+        test, litConfig, tmpBase, parsedCommands, execDir, debug=False
     )
     if isinstance(res, lit.Test.Result):  # Handle failure to parse the Lit test
         res = (&quot;&quot;, res.output, 127, None)
     (out, err, exitCode, timeoutInfo) = res
 
-    # TODO: As a temporary workaround until https://reviews.llvm.org/D81892 lands, manually
-    #       split any stderr output that is included in stdout. It shouldn&#x27;t be there, but
-    #       the Lit internal shell conflates stderr and stdout.
-    conflatedErrorOutput = re.search(&quot;(# command stderr:.+$)&quot;, out, flags=re.DOTALL)
-    if conflatedErrorOutput:
-        conflatedErrorOutput = conflatedErrorOutput.group(0)
-        out = out[: -len(conflatedErrorOutput)]
-        err += conflatedErrorOutput
-
     return (out, err, exitCode, timeoutInfo, parsedCommands)
 
 
@@ -400,9 +358,8 @@ def _generateGenTest(self, testSuite, pathInSuite, litConfig, localConfig):
             raise RuntimeError(f&quot;Error while trying to generate gen test\nstdout:\n{out}\n\nstderr:\n{err}&quot;)
 
         # Split the generated output into multiple files and generate one test for each file
-        parsed = _parseLitOutput(out)
-        for (subfile, content) in self._splitFile(parsed):
-            generatedFile = testSuite.getExecPath(pathInSuite + (subfile, ))
+        for subfile, content in self._splitFile(out):
+            generatedFile = testSuite.getExecPath(pathInSuite + (subfile,))
             os.makedirs(os.path.dirname(generatedFile), exist_ok=True)
             with open(generatedFile, &#x27;w&#x27;) as f:
                 f.write(content)
diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst
index 28319660d69c7c8..5e2443cb612a48d 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -94,21 +94,19 @@ OUTPUT OPTIONS
  Show more information on test failures, for example the entire test output
  instead of just the test result.
 
+ Each command is printed before it is executed. This can be valuable for
+ debugging test failures, as the last printed command is the one that failed.
+ Moreover, :program:`lit` inserts ``&#x27;RUN: at line N&#x27;`` before each
+ command pipeline in the output to help you locate the source line of
+ the failed command.
+
 .. option:: -vv, --echo-all-commands
 
- On test failure, echo all commands to stdout as they are being executed.
- This can be valuable for debugging test failures, as the last echoed command
- will be the one which has failed.
- :program:`lit` normally inserts a no-op command (``:`` in the case of bash)
- with argument ``&#x27;RUN: at line N&#x27;`` before each command pipeline, and this
- option also causes those no-op commands to be echoed to stdout to help you
- locate the source line of the failed command.
- This option implies ``--verbose``.
+ Deprecated alias for -v.
 
 .. option:: -a, --show-all
 
- Show more information about all tests, for example the entire test
- commandline and output.
+ Enable -v, but for all tests not just failed tests.
 
 .. option:: --no-progress-bar
 
diff --git a/llvm/utils/lit/lit/LitConfig.py b/llvm/utils/lit/lit/LitConfig.py
index 331c21d7b9dfcf2..d7e79b60f385bd3 100644
--- a/llvm/utils/lit/lit/LitConfig.py
+++ b/llvm/utils/lit/lit/LitConfig.py
@@ -36,7 +36,6 @@ def __init__(
         config_prefix=None,
         maxIndividualTestTime=0,
         parallelism_groups={},
-        echo_all_commands=False,
         per_test_coverage=False,
     ):
         # The name of the test runner.
@@ -87,7 +86,6 @@ def __init__(
 
         self.maxIndividualTestTime = maxIndividualTestTime
         self.parallelism_groups = parallelism_groups
-        self.echo_all_commands = echo_all_commands
         self.per_test_coverage = per_test_coverage
 
     @property
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index b2c50f563696fe2..528694b07f03300 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -8,6 +8,7 @@
 import stat
 import pathlib
 import platform
+import shlex
 import shutil
 import tempfile
 import threading
@@ -348,12 +349,12 @@ def executeBuiltinExport(cmd, shenv):
 
 
 def executeBuiltinEcho(cmd, shenv):
-    &quot;&quot;&quot;Interpret a redirected echo command&quot;&quot;&quot;
+    &quot;&quot;&quot;Interpret a redirected echo or @echo command&quot;&quot;&quot;
     opened_files = []
     stdin, stdout, stderr = processRedirects(cmd, subprocess.PIPE, shenv, opened_files)
     if stdin != subprocess.PIPE or stderr != subprocess.PIPE:
         raise InternalShellError(
-            cmd, &quot;stdin and stderr redirects not supported for echo&quot;
+            cmd, f&quot;stdin and stderr redirects not supported for {cmd.args[0]}&quot;
         )
 
     # Some tests have un-redirected echo commands to help debug test failures.
@@ -700,6 +701,7 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
         &quot;cd&quot;: executeBuiltinCd,
         &quot;export&quot;: executeBuiltinExport,
         &quot;echo&quot;: executeBuiltinEcho,
+        &quot;@echo&quot;: executeBuiltinEcho,
         &quot;mkdir&quot;: executeBuiltinMkdir,
         &quot;popd&quot;: executeBuiltinPopd,
         &quot;pushd&quot;: executeBuiltinPushd,
@@ -927,7 +929,7 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
         if res == -signal.SIGINT:
             raise KeyboardInterrupt
         if proc_not_counts[i] % 2:
-            res = not res
+            res = 1 if res == 0 else 0
         elif proc_not_counts[i] &gt; 1:
             res = 1 if res != 0 else 0
 
@@ -990,19 +992,60 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
     return exitCode
 
 
-def executeScriptInternal(test, litConfig, tmpBase, commands, cwd):
+def formatOutput(title, data, limit=None):
+    if not data.strip():
+        return &quot;&quot;
+    if not limit is None and len(data) &gt; limit:
+        data = data[:limit] + &quot;\n...\n&quot;
+        msg = &quot;data was truncated&quot;
+    else:
+        msg = &quot;&quot;
+    ndashes = 30
+    # fmt: off
+    out =  f&quot;# .---{title}{&#x27;-&#x27; * (ndashes - 4 - len(title))}\n&quot;
+    out += f&quot;# | &quot; + &quot;\n# | &quot;.join(data.splitlines()) + &quot;\n&quot;
+    out += f&quot;# `---{msg}{&#x27;-&#x27; * (ndashes - 4 - len(msg))}\n&quot;
+    # fmt: on
+    return out
+
+
+# Normally returns out, err, exitCode, timeoutInfo.
+#
+# If debug is True (the normal lit behavior), err is empty, and out contains an
+# execution trace, including stdout and stderr shown per command executed.
+#
+# If debug is False (set by some custom lit test formats that call this
+# function), out contains only stdout from the script, err contains only stderr
+# from the script, and there is no execution trace.
+def executeScriptInternal(test, litConfig, tmpBase, commands, cwd, debug=True):
     cmds = []
     for i, ln in enumerate(commands):
+        # Within lit, we try to always add &#x27;%dbg(...)&#x27; to command lines in order
+        # to maximize debuggability.  However, custom lit test formats might not
+        # always add it, so add a generic debug message in that case.
         match = re.fullmatch(kPdbgRegex, ln)
         if match:
+            dbg = match.group(1)
             command = match.group(2)
-            ln = commands[i] = match.expand(&quot;: &#x27;\\1&#x27;; \\2&quot; if command else &quot;: &#x27;\\1&#x27;&quot;)
+        else:
+            dbg = &quot;command line&quot;
+            command = ln
+        if debug:
+            ln = f&quot;@echo &#x27;# {dbg}&#x27; &quot;
+            if command:
+                ln += f&quot;&amp;&amp; @echo {shlex.quote(command.lstrip())} &amp;&amp; {command}&quot;
+            else:
+                ln += &quot;has no command after substitutions&quot;
+        else:
+            ln = command
         try:
             cmds.append(
                 ShUtil.ShParser(ln, litConfig.isWindows, test.config.pipefail).parse()
             )
         except:
-            return lit.Test.Result(Test.FAIL, &quot;shell parser error on: %r&quot; % ln)
+            return lit.Test.Result(
+                Test.FAIL, f&quot;shell parser error on {dbg}: {command.lstrip()}\n&quot;
+            )
 
     cmd = cmds[0]
     for c in cmds[1:]:
@@ -1022,8 +1065,42 @@ def executeScriptInternal(test, litConfig, tmpBase, commands, cwd):
 
     out = err = &quot;&quot;
     for i, result in enumerate(results):
-        # Write the command line run.
-        out += &quot;$ %s\n&quot; % (&quot; &quot;.join(&#x27;&quot;%s&quot;&#x27; % s for s in result.command.args),)
+        if not debug:
+            out += result.stdout
+            err += result.stderr
+            continue
+
+        # The purpose of an &quot;@echo&quot; command is merely to add a debugging message
+        # directly to lit&#x27;s output.  It is used internally by lit&#x27;s internal
+        # shell and is not currently documented for use in lit tests.  However,
+        # if someone misuses it (e.g., both &quot;echo&quot; and &quot;@echo&quot; complain about
+        # stdin redirection), produce the normal execution trace to facilitate
+        # debugging.
+        if (
+            result.command.args[0] == &quot;@echo&quot;
+            and result.exitCode == 0
+            and not result.stderr
+            and not result.outputFiles
+            and not result.timeoutReached
+        ):
+            out += result.stdout
+            continue
+
+        # Write the command line that was run.  Properly quote it.  Leading
+        # &quot;!&quot; commands should not be quoted as that would indicate they are not
+        # the builtins.
+        out += &quot;# executed command: &quot;
+        nLeadingBangs = next(
+            (i for i, cmd in enumerate(result.command.args) if cmd != &quot;!&quot;),
+            len(result.command.args),
+        )
+        out += &quot;! &quot; * nLeadingBangs
+        out += &quot; &quot;.join(
+            shlex.quote(str(s))
+            for i, s in enumerate(result.command.args)
+            if i &gt;= nLeadingBangs
+        )
+        out += &quot;\n&quot;
 
         # If nothing interesting happened, move on.
         if (
@@ -1038,22 +1115,14 @@ def executeScriptInternal(test, litConfig, tmpBase, commands, cwd):
 
         # Add the command output, if redirected.
         for (name, path, data) in result.outputFiles:
-            if data.strip():
-                out += &quot;# redirected output from %r:\n&quot; % (name,)
-                data = to_string(data.decode(&quot;utf-8&quot;, errors=&quot;replace&quot;))
-                if len(data) &gt; 1024:
-                    out += data[:1024] + &quot;\n...\n&quot;
-                    out += &quot;note: data was truncated\n&quot;
-                else:
-                    out += data
-                out += &quot;\n&quot;
-
+            data = to_string(data.decode(&quot;utf-8&quot;, errors=&quot;replace&quot;))
+            out += formatOutput(f&quot;redirected output from &#x27;{name}&#x27;&quot;, data, limit=1024)
         if result.stdout.strip():
-            out += &quot;# command output:\n%s\n&quot; % (result.stdout,)
+            out += formatOutput(&quot;command stdout&quot;, result.stdout)
         if result.stderr.strip():
-            out += &quot;# command stderr:\n%s\n&quot; % (result.stderr,)
+            out += formatOutput(&quot;command stderr&quot;, result.stderr)
         if not result.stdout.strip() and not result.stderr.strip():
-            out += &quot;note: command had no output on stdout or stderr\n&quot;
+            out += &quot;# note: command had no output on stdout or stderr\n&quot;
 
         # Show the error conditions:
         if result.exitCode != 0:
@@ -1063,9 +1132,9 @@ def executeScriptInternal(test, litConfig, tmpBase, commands, cwd):
                 codeStr = hex(int(result.exitCode &amp; 0xFFFFFFFF)).rstrip(&quot;L&quot;)
             else:
                 codeStr = str(result.exitCode)
-            out += &quot;error: command failed with exit status: %s\n&quot; % (codeStr,)
+            out += &quot;# error: command failed with exit status: %s\n&quot; % (codeStr,)
         if litConfig.maxIndividualTestTime &gt; 0 and result.timeoutReached:
-            out += &quot;error: command reached timeout: %s\n&quot; % (
+            out += &quot;# error: command reached timeout: %s\n&quot; % (
                 str(result.timeoutReached),
             )
 
@@ -1095,21 +1164,49 @@ def executeScript(test, litConfig, tmpBase, commands, cwd):
                 commands[i] = match.expand(
                     &quot;echo &#x27;\\1&#x27; &gt; nul &amp;&amp; &quot; if command else &quot;echo &#x27;\\1&#x27; &gt; nul&quot;
                 )
-        if litConfig.echo_all_commands:
-            f.write(&quot;@echo on\n&quot;)
-        else:
-            f.write(&quot;@echo off\n&quot;)
+        f.write(&quot;@echo on\n&quot;)
         f.write(&quot;\n@if %ERRORLEVEL% NEQ 0 EXIT\n&quot;.join(commands))
     else:
         for i, ln in enumerate(commands):
             match = re.fullmatch(kPdbgRegex, ln)
             if match:
+                dbg = match.group(1)
                 command = match.group(2)
-                commands[i] = match.expand(&quot;: &#x27;\\1&#x27;; \\2&quot; if command else &quot;: &#x27;\\1&#x27;&quot;)
+                # Echo the debugging diagnostic to stderr.
+                #
+                # For that echo command, use &#x27;set&#x27; commands to suppress the
+                # shell&#x27;s execution trace, which would just add noise.  Suppress
+                # the shell&#x27;s execution trace for the &#x27;set&#x27; commands by
+                # redirecting their stderr to /dev/null.
+ ...

jdenny-ornl · 2023-09-14T17:48:03Z

The current PR replaces PR #65267 but has some notable differences:

I have rebased it onto the most recent versions of D154984 and D156954, which are based on recent main.
I have folded in the fix for the bot failures that required me to revert the original PR. I also added a lit test for that case.
I added a solution for suppressing duplicate printing of RUN lines (once for tracing the associated echo command, and once for the output of that echo command). It's proposal 3 in the RFC comment that addresses this issue. I decided against proposal 1 because later comments expressed concern about how much info would be lost under it.
I applied python formatting suggestions from darker.

RoboTux

LGTM. Note: I've only looked at third commit as instructed, that is: a398f3e

jdenny-ornl · 2023-09-15T14:12:41Z

LGTM.

Thanks for the quick review!

Note: I've only looked at third commit as instructed, that is: a398f3e

You mean d71c72c9f20ed34dc2fe2d0ec849a6e44dbf9d20, right? a398f3e1dfbfc83489e4d85fd74e8b3f12d469e9 is the first commit.

RoboTux · 2023-09-15T14:16:56Z

LGTM.

Thanks for the quick review!

Note: I've only looked at third commit as instructed, that is: a398f3e

You mean d71c72c, right? a398f3e is the first commit.

Sadly not. I never remember if top is older or newer. Sorry, I'll review it now then. Please ignore my approval for now.

RoboTux

LGTM

jdenny-ornl · 2023-09-15T14:27:52Z

LGTM.

Thanks for the quick review!

Note: I've only looked at third commit as instructed, that is: a398f3e

You mean d71c72c, right? a398f3e is the first commit.

Sadly not. I never remember if top is older or newer. Sorry, I'll review it now then. Please ignore my approval for now.

Sorry for the confusion, and thanks again for your help! My takeaway is that, for situations like this, I should link to the actual commits to avoid the ambiguity.

RoboTux · 2023-09-15T14:30:49Z

LGTM.

Thanks for the quick review!

Note: I've only looked at third commit as instructed, that is: a398f3e

You mean d71c72c, right? a398f3e is the first commit.

Sadly not. I never remember if top is older or newer. Sorry, I'll review it now then. Please ignore my approval for now.

Sorry for the confusion, and thanks again for your help! My takeaway is that, for situations like this, I should link to the actual commits to avoid the ambiguity.

Or just give the commit title. And only temporarily, I'm sure we'll all be well acquainted with github interface eventually.

jdenny-ornl · 2023-09-15T14:55:14Z

Or just give the commit title. And only temporarily, I'm sure we'll all be well acquainted with github interface eventually.

OK, did that too, in case any one else wants to take a look. I'll pull the note out when landing as it won't mean much in the final commit log.

Instead of landing on a Friday, I think it will be best to wait until at least early next week. That will give more time for others to comment, and it will make it easier to respond to any immediate issues. That seems important given the trouble this series had when I tried to land it last time.

Thanks again for your help in moving this along.

Before <https://reviews.llvm.org/D154984> and <https://reviews.llvm.org/D156954>, lit reported full RUN lines in a `Script:` section. Now, in the case of lit's internal shell, it's the execution trace that includes them. However, if lit is configured to use an external shell (e.g., bash, windows `cmd`), they aren't reported at all. A fix was requested at the following: * <https://reviews.llvm.org/D154984#4627605> * <https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839/35?u=jdenny-ornl> This patch does not address the case when the external shell is windows `cmd`. As discussed at <llvm#65242>, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.

dyung · 2023-09-19T20:50:48Z

There seems to be a race condition of sorts which has caused the test shtest-run-at-line.py to be a bit unreliable on a Windows bot:
https://lab.llvm.org/buildbot/#/builders/216

It seems like there is some extra output that is not expected that is causing the CHECK-NEXT to fail on several lines. For example:

             6: Command Output (stderr): 
 check:17       ^~~~~~~~~~~~~~~~~~~~~~~
             7: -- 
 next:18        ^~
             8: bash.exe: warning: could not find /tmp, please create! 
             9: RUN: at line 4: true 
 next:19        !~~~~~~~~~~~~~~~~~~~  error: match on wrong line

The extra line bash.exe: warning: could not find /tmp, please create! seems to sometimes cause the test to fail. Can the test be restructured such that it can handle this?

jdenny-ornl · 2023-09-19T21:12:19Z

There seems to be a race condition of sorts which has caused the test shtest-run-at-line.py to be a bit unreliable on a Windows bot: https://lab.llvm.org/buildbot/#/builders/216

Thanks for pointing that out. (And thanks for enabling FileCheck verbosity on this bot!)

         8: bash.exe: warning: could not find /tmp, please create!

Do you know why this is being printed at all? Is it reasonable to just create that directory on that bot?

Can the test be restructured such that it can handle this?

I'm concerned about weakening the test and missing other problems. I might use grep -v to throw away that line. Is that portable to windows?

dyung · 2023-09-19T21:36:10Z

There seems to be a race condition of sorts which has caused the test shtest-run-at-line.py to be a bit unreliable on a Windows bot: https://lab.llvm.org/buildbot/#/builders/216

Thanks for pointing that out. (And thanks for enabling FileCheck verbosity on this bot!)

Thanks for telling me how to, it is much more useful now!

         8: bash.exe: warning: could not find /tmp, please create! 
Do you know why this is being printed at all? Is it reasonable to just create that directory on that bot?

I could, although I vaguely recall having similar problems in the past trying to get rid of this error, but I can try. I would prefer that the test can handle it just in case though.

Can the test be restructured such that it can handle this?

I'm concerned about weakening the test and missing other problems. I might use grep -v to throw away that line. Is that portable to windows?

I'm not sure. I think there are some tests that still do use grep, but I think the usage of it is highly discouraged these days.

jdenny-ornl · 2023-09-19T21:49:24Z

Thanks for telling me how to, it is much more useful now!

Great!

         8: bash.exe: warning: could not find /tmp, please create! 
Do you know why this is being printed at all? Is it reasonable to just create that directory on that bot?
I could, although I vaguely recall having similar problems in the past trying to get rid of this error, but I can try. I would prefer that the test can handle it just in case though.

Understood.

Can the test be restructured such that it can handle this?

I'm concerned about weakening the test and missing other problems. I might use grep -v to throw away that line. Is that portable to windows?

I'm not sure. I think there are some tests that still do use grep, but I think the usage of it is highly discouraged these days.

I grepped for grep and found a number of tests still using it. I'll push a commit with this solution for now so that (hopefully) the bot is green again. We can try to find a better solution afterward. Does that seem reasonable?

dyung · 2023-09-19T21:56:49Z

Can the test be restructured such that it can handle this?

I'm concerned about weakening the test and missing other problems. I might use grep -v to throw away that line. Is that portable to windows?

I'm not sure. I think there are some tests that still do use grep, but I think the usage of it is highly discouraged these days.

I grepped for grep and found a number of tests still using it. I'll push a commit with this solution for now so that (hopefully) the bot is green again. We can try to find a better solution afterward. Does that seem reasonable?

Sure, that sounds reasonable to me.

mstorsjo · 2023-09-19T21:58:02Z

I'm concerned about weakening the test and missing other problems. I might use grep -v to throw away that line. Is that portable to windows?

I'm not sure. I think there are some tests that still do use grep, but I think the usage of it is highly discouraged these days.

I guess it depends on which testsuite it is. The main llvm testsuite (and the other ones that use the same base setup) rely on a handful of unix-like tools, and will implicitly try to find them from an installation of Git for Windows, unless otherwise available: https://github.com/llvm/llvm-project/blob/llvmorg-17.0.1/llvm/utils/lit/lit/llvm/config.py#L29-L34

jdenny-ornl · 2023-09-19T22:04:55Z

This is lit's own test suite, which invokes that same LLVMConfig constructor.

Seen at <https://lab.llvm.org/buildbot/#/builders/216/builds/27531/steps/7/logs/FAIL__lit___shtest-run-at-line_py>. Discussed starting at <#66408 (comment)>.

jdenny-ornl · 2023-09-19T22:07:36Z

Can the test be restructured such that it can handle this?

I'm concerned about weakening the test and missing other problems. I might use grep -v to throw away that line. Is that portable to windows?

I'm not sure. I think there are some tests that still do use grep, but I think the usage of it is highly discouraged these days.

I grepped for grep and found a number of tests still using it. I'll push a commit with this solution for now so that (hopefully) the bot is green again. We can try to find a better solution afterward. Does that seem reasonable?

Sure, that sounds reasonable to me.

Pushed that: aa71680. I'll keep an eye on the bots for any additional issues.

jdenny-ornl · 2023-09-19T22:42:07Z

Fixed another instance of that: 30d77fb

dyung · 2023-09-19T22:43:09Z

Fixed another instance of that: 30d77fb

Thanks for the quick fix, was just about to comment on that! Hopefully that's the last of them!

jdenny-ornl · 2023-09-19T23:02:16Z

I think we're past that problem now. The remaining failure appears to be a / vs. \ issue in a new test introduced by D150987. (It also happens to be related to /tmp, but I think that's a coincidence.)

dyung · 2023-09-19T23:04:51Z

I think we're past that problem now. The remaining failure appears to be a / vs. \ issue in a new test introduced by D150987. (It also happens to be related to /tmp, but I think that's a coincidence.)

Yeah, I think so as well. I'm testing a fix for that issue.

Seen at <https://lab.llvm.org/buildbot/#/builders/216/builds/27531/steps/7/logs/FAIL__lit___shtest-run-at-line_py>. Discussed starting at <llvm/llvm-project#66408 (comment)>.

jdenny-ornl requested review from rnk, mstorsjo, RoboTux, MaskRay, yln, ldionne, banach-space, asavonic, AaronBallman, Endilll, bjope, jh7370 and mikaelholmen September 14, 2023 17:41

llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. llvm-lit testing-tools labels Sep 14, 2023

jdenny-ornl mentioned this pull request Sep 14, 2023

[lit] Echo full RUN lines in case of external shells #65267

Merged

RoboTux approved these changes Sep 15, 2023

View reviewed changes

jdenny-ornl force-pushed the lit-external-shell-echo-run-lines branch from d71c72c to a76ca4c Compare September 19, 2023 16:45

jdenny-ornl merged commit fba457d into llvm:main Sep 19, 2023

srpande mentioned this pull request Sep 19, 2023

[SelectionDAG] [NFC] Add pre-commit test for PR66701. srpande/llvm-project#2

Closed

srpande mentioned this pull request Sep 19, 2023

[SelectionDAG] [NFC] Add pre-commit test for PR66701. srpande/llvm-project#3

Closed

jdenny-ornl added a commit that referenced this pull request Sep 19, 2023

[lit] Fix a test fail under windows

aa71680

Seen at <https://lab.llvm.org/buildbot/#/builders/216/builds/27531/steps/7/logs/FAIL__lit___shtest-run-at-line_py>. Discussed starting at <#66408 (comment)>.

This was referenced Sep 20, 2023

sve gather scatter offset sinking #66932

Closed

sve gather scatter offset sinking #66994

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lit] Echo full RUN lines in case of external shells #66408

[lit] Echo full RUN lines in case of external shells #66408

jdenny-ornl commented Sep 14, 2023 •

edited

Loading

llvmbot commented Sep 14, 2023 •

edited

Loading

This patch does not address the case when the external shell is windows `cmd`. As discussed at #65242, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.

jdenny-ornl commented Sep 14, 2023

RoboTux left a comment •

edited

Loading

jdenny-ornl commented Sep 15, 2023

RoboTux commented Sep 15, 2023

RoboTux left a comment

jdenny-ornl commented Sep 15, 2023

RoboTux commented Sep 15, 2023

jdenny-ornl commented Sep 15, 2023

dyung commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

dyung commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

dyung commented Sep 19, 2023

mstorsjo commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

dyung commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

dyung commented Sep 19, 2023

[lit] Echo full RUN lines in case of external shells #66408

[lit] Echo full RUN lines in case of external shells #66408

Conversation

jdenny-ornl commented Sep 14, 2023 • edited Loading

llvmbot commented Sep 14, 2023 • edited Loading

This patch does not address the case when the external shell is windows cmd. As discussed at #65242, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.

jdenny-ornl commented Sep 14, 2023

RoboTux left a comment • edited Loading

Choose a reason for hiding this comment

jdenny-ornl commented Sep 15, 2023

RoboTux commented Sep 15, 2023

RoboTux left a comment

Choose a reason for hiding this comment

jdenny-ornl commented Sep 15, 2023

RoboTux commented Sep 15, 2023

jdenny-ornl commented Sep 15, 2023

dyung commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

dyung commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

dyung commented Sep 19, 2023

mstorsjo commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

dyung commented Sep 19, 2023

jdenny-ornl commented Sep 19, 2023

dyung commented Sep 19, 2023

jdenny-ornl commented Sep 14, 2023 •

edited

Loading

llvmbot commented Sep 14, 2023 •

edited

Loading

This patch does not address the case when the external shell is windows `cmd`. As discussed at #65242, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.

RoboTux left a comment •

edited

Loading