Skip to content

[lit] Clean up internal shell parse errors with ScriptFatal #68496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 20, 2023
Merged

[lit] Clean up internal shell parse errors with ScriptFatal #68496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
909a48a
[lit] Clean up internal shell parse errors with ScriptFatal
jdenny-ornl Oct 7, 2023
7954ad5
Add type annotations on functions documented in this PR
jdenny-ornl Oct 10, 2023
e8a93f5
from typing import Optional, Tuple
jdenny-ornl Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions libcxx/utils/libcxx/test/format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,12 @@ def _executeScriptInternal(test, litConfig, commands):

_, tmpBase = _getTempPaths(test)
execDir = os.path.dirname(test.getExecPath())
res = lit.TestRunner.executeScriptInternal(
test, litConfig, tmpBase, parsedCommands, execDir, debug=False
)
if isinstance(res, lit.Test.Result): # Handle failure to parse the Lit test
res = ("", res.output, 127, None)
try:
res = lit.TestRunner.executeScriptInternal(
test, litConfig, tmpBase, parsedCommands, execDir, debug=False
)
except lit.TestRunner.ScriptFatal as e:
res = ("", str(e), 127, None)
(out, err, exitCode, timeoutInfo) = res

return (out, err, exitCode, timeoutInfo, parsedCommands)
Expand Down
52 changes: 36 additions & 16 deletions llvm/utils/lit/lit/TestRunner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import shutil
import tempfile
import threading
import typing
from typing import Optional, Tuple

import io

Expand All @@ -34,6 +36,17 @@ def __init__(self, command, message):
self.message = message


class ScriptFatal(Exception):
"""
A script had a fatal error such that there's no point in retrying. The
message has not been emitted on stdout or stderr but is instead included in
this exception.
"""

def __init__(self, message):
super().__init__(message)


kIsWindows = platform.system() == "Windows"

# Don't use close_fds on Windows.
Expand Down Expand Up @@ -1009,15 +1022,18 @@ def formatOutput(title, data, limit=None):
return out


# Normally returns out, err, exitCode, timeoutInfo.
# Always either returns the tuple (out, err, exitCode, timeoutInfo) or raises a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using a type annotation for the function signature instead of this comment? Then it can be checked by type checker tools as well as IDEs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. Are you thinking of something like the following?

def executeScriptInternal(
    test, litConfig, tmpBase, commands, cwd, debug=True
) -> typing.Tuple[str, str, int, str | None]:

I tried running mypy to check this, and it complained:

llvm/utils/lit/lit/TestRunner.py:1035: error: X | Y syntax for unions requires Python 3.10  [syntax]

However, LLVM requires only python 3.6 currently.

Also, is there a way to provide names/descriptions for the tuple members? Is there a way to specify exceptions? I couldn't find a syntax for either.

Assuming the above limitations, it looks like the comment would be useful anyway. I'm not experienced with type annotations in python. Please let me know if I'm doing something wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how you do it in Python 3.6:

def executeScriptInternal(
    test, litConfig, tmpBase, commands, cwd, debug=True
) -> typing.Optional[typing.Tuple[str, str, int, str]]:

Also, is there a way to provide names/descriptions for the tuple members?

https://docs.python.org/3/library/typing.html#typing.NamedTuple
Dataclasses might be relevant here as well.

Is there a way to specify exceptions?

It was an explicit design choice to not include exceptions in type annotations: https://stackoverflow.com/a/44282299/4182606

I'm not experienced with type annotations in python. Please let me know if I'm doing something wrong.

I have experience with type annotations in Python, specifically in 3.5 and 3.6 versions, so I'll be happy to review them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is there a way to provide names/descriptions for the tuple members?

https://docs.python.org/3/library/typing.html#typing.NamedTuple Dataclasses might be relevant here as well.

If I understand that correctly, I'd be changing the return type rather than just documenting/annotating it. Maybe the type annotation plus a comment naming the fields is enough?

I have experience with type annotations in Python, specifically in 3.5 and 3.6 versions, so I'll be happy to review them.

Thanks for the suggestions. I pushed a commit. Let me know if you think something more would be worthwhile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand that correctly, I'd be changing the return type rather than just documenting/annotating it. Maybe the type annotation plus a comment naming the fields is enough?

If you intend to add names to typing.Tuple[str, str, int, typing.Optional[str]], I'm not sure it's possible in a way that won't scare type checkers away, but I'm sure it's not conventional for Python. If you want to name tuple members, you should create a type with namedtuple(), and return it.
https://github.com/llvm/llvm-project/pull/68496/files#diff-06d85faa2a7a6970de33ccb78e3e004456eb51ab7303330ec52a5e35db92d415R53 would definitely benefit from named arguments.

# ScriptFatal exception.
#
# 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):
def executeScriptInternal(
test, litConfig, tmpBase, commands, cwd, debug=True
) -> Tuple[str, str, int, Optional[str]]:
cmds = []
for i, ln in enumerate(commands):
# Within lit, we try to always add '%dbg(...)' to command lines in order
Expand All @@ -1043,9 +1059,9 @@ def executeScriptInternal(test, litConfig, tmpBase, commands, cwd, debug=True):
ShUtil.ShParser(ln, litConfig.isWindows, test.config.pipefail).parse()
)
except:
return lit.Test.Result(
Test.FAIL, f"shell parser error on {dbg}: {command.lstrip()}\n"
)
raise ScriptFatal(
f"shell parser error on {dbg}: {command.lstrip()}\n"
) from None

cmd = cmds[0]
for c in cmds[1:]:
Expand Down Expand Up @@ -2130,8 +2146,11 @@ def parseIntegratedTestScript(test, additional_parsers=[], require_script=True):
return script


def _runShTest(test, litConfig, useExternalSh, script, tmpBase):
def runOnce(execdir):
def _runShTest(test, litConfig, useExternalSh, script, tmpBase) -> lit.Test.Result:
# Always returns the tuple (out, err, exitCode, timeoutInfo, status).
def runOnce(
execdir,
) -> Tuple[str, str, int, Optional[str], Test.ResultCode]:
# script is modified below (for litConfig.per_test_coverage, and for
# %dbg expansions). runOnce can be called multiple times, but applying
# the modifications multiple times can corrupt script, so always modify
Expand All @@ -2158,12 +2177,16 @@ def runOnce(execdir):
command = buildPdbgCommand(dbg, command)
scriptCopy[i] = command

if useExternalSh:
res = executeScript(test, litConfig, tmpBase, scriptCopy, execdir)
else:
res = executeScriptInternal(test, litConfig, tmpBase, scriptCopy, execdir)
if isinstance(res, lit.Test.Result):
return res
try:
if useExternalSh:
res = executeScript(test, litConfig, tmpBase, scriptCopy, execdir)
else:
res = executeScriptInternal(
test, litConfig, tmpBase, scriptCopy, execdir
)
except ScriptFatal as e:
out = f"# " + "\n# ".join(str(e).splitlines()) + "\n"
return out, "", 1, None, Test.UNRESOLVED

out, err, exitCode, timeoutInfo = res
if exitCode == 0:
Expand All @@ -2183,9 +2206,6 @@ def runOnce(execdir):
attempts = test.allowed_retries + 1
for i in range(attempts):
res = runOnce(execdir)
if isinstance(res, lit.Test.Result):
return res

out, err, exitCode, timeoutInfo, status = res
if status != Test.FAIL:
break
Expand Down
18 changes: 13 additions & 5 deletions llvm/utils/lit/tests/shtest-shell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,10 +561,17 @@

# FIXME: The output here sucks.
#
# CHECK: FAIL: shtest-shell :: error-1.txt
# CHECK: *** TEST 'shtest-shell :: error-1.txt' FAILED ***
# CHECK: shell parser error on RUN: at line 3: echo "missing quote
# CHECK: ***
# CHECK: UNRESOLVED: shtest-shell :: error-1.txt
# CHECK-NEXT: *** TEST 'shtest-shell :: error-1.txt' FAILED ***
# CHECK-NEXT: Exit Code: 1
# CHECK-EMPTY:
# CHECK-NEXT: Command Output (stdout):
# CHECK-NEXT: --
# CHECK-NEXT: # shell parser error on RUN: at line 3: echo "missing quote
# CHECK-EMPTY:
# CHECK-NEXT: --
# CHECK-EMPTY:
# CHECK-NEXT: ***

# CHECK: FAIL: shtest-shell :: error-2.txt
# CHECK: *** TEST 'shtest-shell :: error-2.txt' FAILED ***
Expand Down Expand Up @@ -643,4 +650,5 @@
# CHECK: ***

# CHECK: PASS: shtest-shell :: valid-shell.txt
# CHECK: Failed Tests (39)
# CHECK: Unresolved Tests (1)
# CHECK: Failed Tests (38)