Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions llvm/docs/CommandGuide/lit.rst
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,7 @@ TestRunner.py:
Otherwise, %t but with a single leading ``/`` removed.
%:T On Windows, %/T but a ``:`` is removed if its the second character.
Otherwise, %T but with a single leading ``/`` removed.
%{readfile:<filename>} Reads the file specified.
======================= ==============

Other substitutions are provided that are variations on this base set and
Expand Down
1 change: 1 addition & 0 deletions llvm/test/tools/llvm-cgdata/empty.test
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,4 @@ RUN: printf '\000\000\000\000' >> %t_header.cgdata
RUN: printf '\040\000\000\000\000\000\000\000' >> %t_header.cgdata
RUN: printf '\040\000\000\000\000\000\000\000' >> %t_header.cgdata
RUN: diff %t_header.cgdata %t_emptyheader.cgdata
RUN: echo %{readfile:/tmp/test} > /tmp/test
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this change for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debugging. Looks like I didn't review my PR diff closely enough before requesting review. Sorry for the noise here.

20 changes: 20 additions & 0 deletions llvm/utils/lit/lit/TestRunner.py
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,23 @@ def processRedirects(cmd, stdin_source, cmd_shenv, opened_files):
return std_fds


def _expandLateSubstitutions(arguments, cwd):
for i, arg in enumerate(arguments):
if not isinstance(arg, str):
continue

def _replaceReadFile(match):
filePath = match.group(1)
if not os.path.isabs(filePath):
filePath = os.path.join(cwd, filePath)
with open(filePath) as fileHandle:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you verify that the file exists before trying to open & read it?

Copy link
Contributor

Choose a reason for hiding this comment

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

And fail gracefully otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That wouldn't be very pythonic. try/except would be more idiomatic, if we want a more graceful failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we were going to try/catch, we would end up throwing an internal shell error without about the same amount of information. I think this is fine to leave as is, but can change it up if desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whether you use try/catch or something else, I still think you should verify the file exists before trying to open it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We certainly should have a test that covers the case of a missing file. lit should also treat such a case as a FAIL test and not some other failure mode. I don't know whether that requires any particular handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To get this to be marked as a failure instead of unresolved we need to propagate it up as a InternalShellError. I've made the necessary modifications and added a test for this case. Please take a look.

return fileHandle.read()

arguments[i] = re.sub(r"%{readfile:([^}]*)}", _replaceReadFile, arg)

return arguments


def _executeShCmd(cmd, shenv, results, timeoutHelper):
if timeoutHelper.timeoutReached():
# Prevent further recursion if the timeout has been hit
Expand Down Expand Up @@ -834,6 +851,9 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
# Ensure args[0] is hashable.
args[0] = expand_glob(args[0], cmd_shenv.cwd)[0]

# Expand all late substitutions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Expand all late substitutions
# Expand all late substitutions.

args = _expandLateSubstitutions(args, cmd_shenv.cwd)

inproc_builtin = inproc_builtins.get(args[0], None)
if inproc_builtin and (args[0] != "echo" or len(cmd.commands) == 1):
# env calling an in-process builtin is useless, so we take the safe
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
## Tests that readfile works with absolute paths
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Tests that readfile works with absolute paths
## Tests that readfile works with absolute paths.

# RUN: echo -n "hello" > %t
# RUN: echo %{readfile:%t}

## Fail the test so we can assert on the output.
# RUN: not echo return
8 changes: 8 additions & 0 deletions llvm/utils/lit/tests/Inputs/shtest-readfile/lit.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import lit.formats

config.name = "shtest-readfile"
config.suffixes = [".txt"]
config.test_format = lit.formats.ShTest(execute_external=False)
config.test_source_root = None
config.test_exec_root = None
config.substitutions.append(("%{python}", '"%s"' % (sys.executable)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not for these tests. I cargo-culted this from the ulimit tests. Removed.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
## Tests that readfile works with relative paths
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Tests that readfile works with relative paths
## Tests that readfile works with relative paths.

# RUN: mkdir -p rel_path_test_folder
# RUN: echo -n "hello" > rel_path_test_folder/test_file
# RUN: echo %{readfile:rel_path_test_folder/test_file}

## Fail the test so we can assert on the output.
# RUN: not echo return
8 changes: 8 additions & 0 deletions llvm/utils/lit/tests/Inputs/shtest-readfile/two-same-line.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
## Tests that readfile works with two substitutions on the same line to ensure the
## regular expressions are setup correctly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## regular expressions are setup correctly.
## regular expressions are set up correctly.

("setup" -> noun; "set up" -> verb or adjective)
/end grammar pedantry

# RUN: echo -n "hello" > %t.1
# RUN: echo -n "bye" > %t.2
# RUN: echo %{readfile:%t.1} %{readfile:%t.2}

## Fail the test so we can assert on the output.
# RUN: not echo return
17 changes: 17 additions & 0 deletions llvm/utils/lit/tests/shtest-readfile.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
## Tests the readfile substitution
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Tests the readfile substitution
## Tests the readfile substitution.


# RUN: not %{lit} -a -v %{inputs}/shtest-readfile | FileCheck -match-full-lines %s

# CHECK: -- Testing: 3 tests{{.*}}

# CHECK-LABEL: FAIL: shtest-readfile :: absolute-paths.txt ({{[^)]*}})
# CHECK: echo hello
# CHECK: # executed command: echo '%{readfile:{{.*}}}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you use FileCheck's -D option or similar to allow you to more precisely match the file name mentioned here?


# CHECK-LABEL: FAIL: shtest-readfile :: relative-paths.txt ({{[^)]*}})
# CHECK: echo hello
# CHECK: # executed command: echo '%{readfile:rel_path_test_folder/test_file}'

# CHECK-LABEL: FAIL: shtest-readfile :: two-same-line.txt ({{[^)]*}})
# CHECK: echo hello bye
# CHECK: # executed command: echo '%{readfile:{{.*}}.1}' '%{readfile:{{.*}}.2}'
Loading