[llvm-lit] Add redirection handling for env command without args and write a lit test to check behavior with lit internal shell
#106629
Conversation
…n lit Added Redirection Handling: Enhanced TestRunner.py to correctly handle cases where the env command is run without arguments. It now checks if there are no arguments and, when redirection is specified, writes the environment variables directly to the redirected file or a temporary file. Test for Redirection Behavior: Created a new test in lit's internal shell to verify that when the env command is executedwithout arguments and with redirection, it properly outputs the environment variables to the specified file.
Harini0924
force-pushed
the
env_add_redirection
branch
from
aea9055 to
5f5af8c
Compare
|
@llvm/pr-subscribers-testing-tools Author: None (Harini0924) ChangesThe current implementation of the This patch enhances the The patch introduces a check for when the Additionally, this patch adds a corresponding test in the lit's internal shell to verify the correct behavior of the This change is relevant for [RFC] Enabling the Lit Internal Shell by Default Full diff: https://github.com/llvm/llvm-project/pull/106629.diff 3 Files Affected:
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 19f35fc7e212f3..2b3c6b8b9b750f 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -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:
+ # 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(), []
+ )
+ )
return 0
elif args[0] == "not":
not_args.append(args.pop(0))
diff --git a/llvm/utils/lit/tests/Inputs/shtest-env-positive/env-temp-redirect.txt b/llvm/utils/lit/tests/Inputs/shtest-env-positive/env-temp-redirect.txt
new file mode 100755
index 00000000000000..b2ad755e81b0ef
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-env-positive/env-temp-redirect.txt
@@ -0,0 +1,7 @@
+## Test the env command with output redirection to a file.
+# RUN: rm -f %t
+# RUN: env > %t
+# RUN: FileCheck %s < %t
+
+# CHECK: BAR=2
+# CHECK: FOO=1
diff --git a/llvm/utils/lit/tests/shtest-env-positive.py b/llvm/utils/lit/tests/shtest-env-positive.py
index 863fbda8c5b6dc..8f5f8385872a65 100644
--- a/llvm/utils/lit/tests/shtest-env-positive.py
+++ b/llvm/utils/lit/tests/shtest-env-positive.py
@@ -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
@@ -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
@@ -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: {{.}}
|
| @@ -0,0 +1,7 @@ | |||
| ## Test the env command with output redirection to a file. | |||
| # RUN: rm -f %t | |||
| # RUN: env > %t | |||
There was a problem hiding this comment.
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?
| ) | ||
| if stdout != default_stdin: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
| 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(), [] | ||
| ) | ||
| ) |
There was a problem hiding this comment.
| # 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.
The current implementation of the
envcommand in lit's internal shell lacks support for redirection to a temporary file when the command is executed without arguments. This limitation means that when users attempt to redirect the output of theenvcommand to a temporary file, it fails to find the file, leading to unexpected behavior.This patch enhances the
TestRunner.pyto correctly handle cases where theenvcommand is executed without arguments and with redirection. Previously, theenvcommand did not support redirection to a temporary file when no arguments were provided, leading to unexpected behavior where environment variables were not written to the specified file.The patch introduces a check for when the
envcommand is run without arguments. It now calls theprocessRedirectsfunction, which returns the file descriptors (stdin (0), stdout (1) , stderr (2) ). If a redirection is specified (i.e.,stdoutis not the defaultstdin), the environment variables are directly written to the redirected file using thestdout.write(env_str)command. Otherwise, the output is captured for scenarios without redirection.Additionally, this patch adds a corresponding test in the lit's internal shell to verify the correct behavior of the
envcommand with output redirection. The test ensures that when theenvcommand is redirected to a temp file and checks if the environment variables are correctly outputted.This change is relevant for [RFC] Enabling the Lit Internal Shell by Default
fixes: #106627