Skip to content

[llvm-lit] Add redirection handling for env command without args and write a lit test to check behavior with lit internal shell #106629

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[llvm-lit] Add redirection handling for env command without args and write a lit test to check behavior with lit internal shell #106629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 18 additions & 4 deletions llvm/utils/lit/lit/TestRunner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,11 +746,25 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
env_str = "\n".join(
f"{key}={value}" for key, value in sorted(cmd_shenv.env.items())
)
results.append(
ShellCommandResult(
j, env_str, "", 0, timeoutHelper.timeoutReached(), []
)
# Process redirections.
stdin, stdout, stderr = processRedirects(
j, default_stdin, cmd_shenv, opened_files
)
if stdout != default_stdin:
Copy link
Member

Choose a reason for hiding this comment

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

Is this always true? I admit I didn't try to understand all of processRedirects, but it doesn't look like the input argument is ever returned for stdout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The input argument (default_stdin, which is set to subprocess.PIPE) is not always returned directly as stdout by the processRedirects function. The function's purpose is to handle any redirections specified in the command. If there's a redirection for stdout (like > or >>), processRedirects will open a file and return a file descriptor for that file instead of returning the original subprocess.PIPE.

So, when you pass subprocess.PIPE as the default for stdout, processRedirects might override it with a different file descriptor if redirection is specified. If no redirection is specified, then it could return subprocess.PIPE as it was originally passed. This means the condition if stdout != default_stdin is designed to check whether stdout was redirected to something other than the pipe, and it's not always true—it depends on whether any redirection was applied.

Did that answer your question?

Copy link
Member

Choose a reason for hiding this comment

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

I looked at processRedirects() and could not see the stdin_source parameter (what you are passing as default_stdin) ever being returned as the second tuple element.

As you say the check is actually if stdout != subprocess.PIPE and the way it is spelled now is very confusing since it seems to imply some relationship between stdout and stdin.

As I understand your comment the != default_stdin check isn't actually checking anything related to what was stdin value for the process, but just relies on the fact that default_stdin holds subprocess.PIPE. With the following check the code makes a lot more sense to me:

Suggested change
if stdout != default_stdin:
if stdout != subprocess.PIPE:

# Write directly to the redirected file (stdout).
stdout.write(env_str)
results.append(
ShellCommandResult(
j, "", "", 0, timeoutHelper.timeoutReached(), []
)
)
else:
# Capture the output for cases without redirection.
results.append(
ShellCommandResult(
j, env_str, "", 0, timeoutHelper.timeoutReached(), []
)
)
Comment on lines +754 to +767
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Write directly to the redirected file (stdout).
stdout.write(env_str)
results.append(
ShellCommandResult(
j, "", "", 0, timeoutHelper.timeoutReached(), []
)
)
else:
# Capture the output for cases without redirection.
results.append(
ShellCommandResult(
j, env_str, "", 0, timeoutHelper.timeoutReached(), []
)
)
# Write directly to the redirected file (stdout).
stdout.write(env_str)
captured_stdout = ""
else:
# Capture the output for cases without redirection.
captured_stdout = env_str
results.append(
ShellCommandResult(
j, captured_stdout, "", 0, timeoutHelper.timeoutReached(), []
)
)

This is a bit shorter and avoids all end empty lines with closing parens.

return 0
elif args[0] == "not":
not_args.append(args.pop(0))
Expand Down
7 changes: 7 additions & 0 deletions llvm/utils/lit/tests/Inputs/shtest-env-positive/env-temp-redirect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
## Test the env command with output redirection to a file.
# RUN: rm -f %t
# RUN: env > %t
Copy link
Member

Choose a reason for hiding this comment

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

I can see from the code that the redirect is only consumed if there are no arguments, but could we also check that env VAR=value > %t and env A=B echo "no env output here please" behave as expected?

# RUN: FileCheck %s < %t

# CHECK: BAR=2
# CHECK: FOO=1
12 changes: 9 additions & 3 deletions llvm/utils/lit/tests/shtest-env-positive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

## Test the env command's successful executions.

# CHECK: -- Testing: 9 tests{{.*}}
# CHECK: -- Testing: 10 tests{{.*}}

# CHECK: PASS: shtest-env :: env-args-last-is-assign.txt ({{[^)]*}})
# CHECK: env FOO=1
Expand Down Expand Up @@ -47,6 +47,12 @@
# CHECK-NOT: # error:
# CHECK: --

# CHECK: PASS: shtest-env :: env-temp-redirect.txt ({{[^)]*}})
# CHECK: env {{.*}}/env-temp-redirect.txt.tmp
# CHECK: # executed command: env
# CHECK-NOT: # error:
# CHECK: --

# CHECK: PASS: shtest-env :: env-u.txt ({{[^)]*}})
# CHECK: env -u FOO | {{.*}}
# CHECK: # executed command: env -u FOO
Expand All @@ -65,6 +71,6 @@
# CHECK-NOT: # error:
# CHECK: --

# CHECK: Total Discovered Tests: 9
# CHECK: Passed: 9 {{\([0-9]*\.[0-9]*%\)}}
# CHECK: Total Discovered Tests: 10
# CHECK: Passed: 10 {{\([0-9]*\.[0-9]*%\)}}
# CHECK-NOT: {{.}}
Loading