[lit] Fix internal env calling env

[jdenny]

Summary

Without this patch, when using lit's internal shell, if env on a lit
RUN line calls env, lit accidentally searches for the latter as an
external executable. What's worse is that works fine when a developer
is testing on a platform where env is available and behaves as
expected, but it then breaks on other platforms.

env calling env can make sense if one such env is within a lit
substitution, as in D65156 and D65121. This patch ensures that lit
executes both as internal commands.

[probinson]

Those files under fake-externals... are those symlinks? IIUC that doesn't work so well on Windows. I admit I haven't tried downloading your patch to verify.

[jdenny]

In D65697#1621615, @probinson wrote:

Those files under fake-externals... are those symlinks? IIUC that doesn't work so well on Windows. I admit I haven't tried downloading your patch to verify.

Yes, they are. I don't use Windows enough to know. I could probably just do something like a python import instead.

By the way, would it be better to separate that fake-externals stuff into another patch?

[jdenny]

Rebase. Remove checking for accidental external command calls. I'll put that in a separate patch.

[probinson]

This all seems plausible but I would rather defer approval to someone who actually understood Python.

[mgorny]

llvm/utils/lit/lit/TestRunner.py

609–610

The RHS of this condition is verifying the original command, so I suppose it's never going to trigger with env.

618

You don't need len() here, list will evaluate to whether it's non-empty in boolean context.

622

You are not checking whether it's empty here. I guess this means it's gonna crash if you pass just env.

630

Same about len() here.

[jdenny]

llvm/utils/lit/lit/TestRunner.py

609–610

By RHS, I think you mean len(cmd.commands) == 1. It checks cmd.commands not cmd.commands[0].args, but env is pruned from the latter not the former. The RHS checks if there's a pipeline not how many args there are in the first command from the pipeline, so I think this is right.

618

OK, I thought len was more explicit, but I'm not an experienced python programmer. I'll change it.

Thanks for the review.

622

Good catch. It turns out lit's env had that problem even without this patch, but that bug is later. I suppose there's no reason I shouldn't fix both as part of this patch.

[jdenny]

Rebased onto D66482, and added a new test here for nested env without arguments.

[rnk]

llvm/utils/lit/lit/TestRunner.py

245

Hm, I bet we shouldn't handle -u for export.

610

I don't like the way we have to process env prefixes twice, once for in-process shell builtins and once for out-of-process builtins. I think we should try to make things more uniform by moving the in-process builtins into the loop between 'env' handling and processRedirects. I'd suggest reducing the boilerplate by building a set of in-process builtins (inproc_builtins?), checking against the set, and then calling a helper function that returns a ShellCommandResult or throws. You can do the check for "cannot be part of a pipeline" in the shared code and raise the same InternalShellErrors. Does that seem straightforward?

Speaking of which, I see that diff actually can take one of the files to compare on stdin, so technically we should move it out of process.

[jdenny]

llvm/utils/lit/lit/TestRunner.py

245

Yeah, that seems wrong. Fixing that surely belongs in a separate patch though. Agreed?

Thanks for the review.

610

Does that seem straightforward?

I'll give it a shot.

By the way, a lot of the ValueErrors here really should be InternalShellErrors, right?

Speaking of which, I see that diff actually can take one of the files to compare on stdin, so technically we should move it out of process.

Sorry, I'm not following. Would that be an extension to what's implemented now for lit's internal diff?

[rnk]

llvm/utils/lit/lit/TestRunner.py

245

Yep, definitely separable.

610

By the way, a lot of the ValueErrors here really should be InternalShellErrors, right?

I think so. The difference is that a ValueError results in an UNRESOLVED status and an InternalShellError is a normal test failure.

Sorry, I'm not following. Would that be an extension to what's implemented now for lit's internal diff?

Yes, so it's out of scope for this patch. I was just reviewing the set of in-process non-pipelined builtins to check that they truly cannot be part of pipelines.

Also, can't diff have it's output redirected? Having it be built-in seems fragile, maybe it should be revisited.

Anyway, all that discussion is out of scope.

[jdenny]

llvm/utils/lit/lit/TestRunner.py

610

I think so. The difference is that a ValueError results in an UNRESOLVED status and an InternalShellError is a normal test failure.

Agreed. It might be arguable that UNRESOLVED makes sense because these errors usually represent malformed tests, but an external shell wouldn't produce that status for a command receiving bad arguments. Besides, ValueError produces an unhelpful stack trace that makes it look like lit has a bug.

Yes, so it's out of scope for this patch. I was just reviewing the set of in-process non-pipelined builtins to check that they truly cannot be part of pipelines.

Understood.

Also, can't diff have it's output redirected? Having it be built-in seems fragile, maybe it should be revisited.

I don't believe I've ever used diff in a lit RUN line, and I haven't tried to figure out what use cases people were targeting when they implemented that. I always use FileCheck instead.

[probinson]

Searching llvm and clang tests for uses of diff, mostly they are diffing two generated files (e.g. diff %t1.ll %t2.ll).
There are diffs against canned files in an Inputs directory, and some of those tests pipe the generated output to diff's stdin. There are a number of these in clang/test/Analysis that diff output against canned XML results, so these are not small outputs you can readily verify with FileCheck.
I noticed some invocations passing -aub options to diff.
I found one example of redirecting diff's output (to /dev/null).

[probinson]

In D65697#1638018, @probinson wrote:

I found one example of redirecting diff's output (to /dev/null).

This one is in llvm/test/tools/llvm-cov/llvm-cov.test (line 91): blah | not diff -u somefile - >/dev/null which IMO could use -q and not redirect. But if we make diff always be external, it doesn't matter.

[jdenny]

In D65697#1638038, @probinson wrote:

In D65697#1638018, @probinson wrote:

I found one example of redirecting diff's output (to /dev/null).

This one is in llvm/test/tools/llvm-cov/llvm-cov.test (line 91): blah | not diff -u somefile - >/dev/null which IMO could use -q and not redirect.

Agreed. Interestingly, discarding the output makes -u pointless, but there's a fixme saying the diff failure here isn't actually desirable.

But if we make diff always be external, it doesn't matter.

Do people know why an internal diff was implemented originally? Is there a portability or performance issue?

If it's worthwhile to have an internal diff, is it worthwhile for it to support everything external diffs do? All pipes and redirects could be replaced by temporary files.

[jdenny]

Replace ValueError with InternalShellError.

[jdenny]

llvm/utils/lit/lit/TestRunner.py

610

I don't like the way we have to process env prefixes twice, once for in-process shell builtins and once for out-of-process builtins.

Are you ok with seeing that cleanup in a follow-up patch?

[jdenny]

Rebase onto D66506.

[jdenny]

llvm/utils/lit/lit/TestRunner.py

610

Are you ok with seeing that cleanup in a follow-up patch?

Nevermind. It turns out it was easy to make it a predecessor, in D66506.

[rnk]

lgtm

Now this one is super simple. :)

llvm/utils/lit/lit/TestRunner.py

637–638

I guess this is a nit, but I'd pull the copy out of the loop and guard it with a single if args[0] == 'env': check. That way it's clearer that it only happens once.

[jdenny]

In D65697#1638518, @rnk wrote:

lgtm

Now this one is super simple. :)

Yep, thanks!

llvm/utils/lit/lit/TestRunner.py

637–638

I'll get back to you on that. I'll soon post a patch that handles issues for not here, and then the first env might not be args[0].

[jdenny]

As discussed, moved relevant tests and parts of summary to D66506.

[probinson]

Do people know why an internal diff was implemented originally? Is there a portability or performance issue?

See D39567 and in particular an enthused zturner looked forward to the day when all "shell" commands are internal, eliminating cross-platform issues and most if not all dependencies on additional packages other than Python (e.g. GnuWin32 on Windows). The only external commands would be LLVM-built tools/utilities. I see rnk participated in that review (late 2017).

If it's worthwhile to have an internal diff, is it worthwhile for it to support everything external diffs do? All pipes and redirects could be replaced by temporary files.

Creating temp files is slower than piping, given we have tens of thousands of tests it feels worthwhile to support piping. For diff in particular I found only one (I think unnecessary) redirect so not supporting that seems reasonable.

[jdenny]

In D65697#1639463, @probinson wrote:

Do people know why an internal diff was implemented originally? Is there a portability or performance issue?

See D39567 and in particular an enthused zturner looked forward to the day when all "shell" commands are internal, eliminating cross-platform issues and most if not all dependencies on additional packages other than Python (e.g. GnuWin32 on Windows). The only external commands would be LLVM-built tools/utilities. I see rnk participated in that review (late 2017).

If it's worthwhile to have an internal diff, is it worthwhile for it to support everything external diffs do? All pipes and redirects could be replaced by temporary files.

Creating temp files is slower than piping, given we have tens of thousands of tests it feels worthwhile to support piping. For diff in particular I found only one (I think unnecessary) redirect so not supporting that seems reasonable.

Thanks. Makes sense.

When rnk suggested diff be moved out of process, I originally thought he meant we should depend on external (that is, not provided by lit) implementations of diff. Now I think he meant we should move it to an external python script, similar to llvm/utils/lit/lit/builtin_commands/cat.py.

[rnk]

In D65697#1639524, @jdenny wrote:

When rnk suggested diff be moved out of process, I originally thought he meant we should depend on external (that is, not provided by lit) implementations of diff. Now I think he meant we should move it to an external python script, similar to llvm/utils/lit/lit/builtin_commands/cat.py.

Yep, that's what I had in mind.

In D65697#1639463, @probinson wrote:

Do people know why an internal diff was implemented originally? Is there a portability or performance issue?

See D39567 and in particular an enthused zturner looked forward to the day when all "shell" commands are internal, eliminating cross-platform issues and most if not all dependencies on additional packages other than Python (e.g. GnuWin32 on Windows). The only external commands would be LLVM-built tools/utilities. I see rnk participated in that review (late 2017).

I think these builtins were added over time to address various one off portability or performance issues. But, as we can see from this patch, they have some bugs and some cost. For diff, the motivation was that some external user wanted to eliminate their test suite dependency on a standalone copy of diff. I'm not sure that's the best motivation, and we might want to revisit it if this causes more problems going forward. Based on what Paul said, it seems that we have some clang/LLVM tests that use internal diff and some that use real external diff in a pipeline, and we didn't know it.

---

Anyway, this patch is good to go, IMO.

[jdenny]

In D65697#1639974, @rnk wrote:

Based on what Paul said, it seems that we have some clang/LLVM tests that use internal diff and some that use real external diff in a pipeline, and we didn't know it.

The tests using an external diff in a pipeline are necessarily within test suites that never use lit's internal diff because the test suites are configured not to use lit's internal shell, right? (That is assuming buggy cases like env or not calling diff don't come into play.)

Anyway, this patch is good to go, IMO.

Thanks again. I'll try to push soon.

[rnk]

In D65697#1640071, @jdenny wrote:

The tests using an external diff in a pipeline are necessarily within test suites that never use lit's internal diff because the test suites are configured not to use lit's internal shell, right? (That is assuming buggy cases like env or not calling diff don't come into play.)

Most LLVM test suites are configured to use the internal shell on Windows. I think what's happening is that users happen to have an external copy of diff that gets called when it appears in a pipeline, or they are prefixed by not. The external diff is called, and things "work out" and we never noticed.

[jdenny]

In D65697#1640109, @rnk wrote:

Most LLVM test suites are configured to use the internal shell on Windows. I think what's happening is that users happen to have an external copy of diff that gets called when it appears in a pipeline, or they are prefixed by not. The external diff is called, and things "work out" and we never noticed.

Are these patches going to break those tests on Windows? We now see diff anywhere in a pipeline, and we reject its appearance in a pipeline.

[probinson]

In D65697#1640136, @jdenny wrote:

In D65697#1640109, @rnk wrote:

Most LLVM test suites are configured to use the internal shell on Windows. I think what's happening is that users happen to have an external copy of diff that gets called when it appears in a pipeline, or they are prefixed by not. The external diff is called, and things "work out" and we never noticed.

Are these patches going to break those tests on Windows? We now see diff anywhere in a pipeline, and we reject its appearance in a pipeline.

grep -r -l 'RUN:.* diff ' llvm/test clang/test gets 240 hits. Searching on | diff gets 41, mostly in clang/test/Analysis. So not a super common pattern, but if it's straightforward to build a Python 'diff' that handles pipes, that would be less invasive, and piping does have a minor speed advantage.

[jdenny]

In D65697#1640202, @probinson wrote:

In D65697#1640136, @jdenny wrote:

In D65697#1640109, @rnk wrote:

Most LLVM test suites are configured to use the internal shell on Windows. I think what's happening is that users happen to have an external copy of diff that gets called when it appears in a pipeline, or they are prefixed by not. The external diff is called, and things "work out" and we never noticed.

Are these patches going to break those tests on Windows? We now see diff anywhere in a pipeline, and we reject its appearance in a pipeline.

I've confirmed that, when using lit's internal shell (not on Windows, but that shouldn't matter), echo foo | diff %t - previously called an external diff, but fails after this patch series is applied.

grep -r -l 'RUN:.* diff ' llvm/test clang/test gets 240 hits. Searching on | diff gets 41, mostly in clang/test/Analysis. So not a super common pattern, but if it's straightforward to build a Python 'diff' that handles pipes, that would be less invasive, and piping does have a minor speed advantage.

I guess I have more work to do either way....

llvm/utils/lit/lit/TestRunner.py

for arg in seq:
 
         if needquote:
             result.extend(bs_buf)
             result.append('"')
 
     return ''.join(result)
 
 # args are from 'export' or 'env' command.
-# Returns copy of args without those commands or their arguments.
+# Skips the command, and parses its arguments.
+# Modifies env accordingly.
+# Returns copy of args without the command or its arguments.
 def updateEnv(env, args):
     arg_idx_next = len(args)
     unset_next_env_var = False
     for arg_idx, arg in enumerate(args[1:]):
         # Support for the -u flag (unsetting) for env command
         # e.g., env -u FOO -u BAR will remove both FOO and BAR
         # from the environment.
         if arg == '-u':
             unset_next_env_var = True
             continue
         if unset_next_env_var:
             unset_next_env_var = False
             if arg in env.env:
                 del env.env[arg]
if isinstance(cmd, ShUtil.Seq):
                 return res
 
             if res == 0:
                 res = _executeShCmd(cmd.rhs, shenv, results, timeoutHelper)
             return res
 
         raise ValueError('Unknown shell command: %r' % cmd.op)
     assert isinstance(cmd, ShUtil.Pipeline)
 
     procs = []
     default_stdin = subprocess.PIPE
     stderrTempFiles = []
     opened_files = []
     named_temp_files = []
     builtin_commands = set(['cat', 'diff'])
     builtin_commands_dir = os.path.join(os.path.dirname(os.path.abspath(__file__)), "builtin_commands")
     inproc_builtins = {'cd': executeBuiltinCd,
                        'export': executeBuiltinExport,
                        'echo': executeBuiltinEcho,
                        'mkdir': executeBuiltinMkdir,
                        'rm': executeBuiltinRm,
                        ':': executeBuiltinColon}
     # To avoid deadlock, we use a single stderr stream for piped
     # output. This is null until we have seen some output using
     # stderr.
     for i,j in enumerate(cmd.commands):
         # Reference the global environment by default.
         cmd_shenv = shenv
         args = list(j.args)
-        if j.args[0] == 'env':
+        while args[0] == 'env':
             # Create a copy of the global environment and modify it for this one
-            # command. There might be multiple envs in a pipeline:
+            # command. There might be multiple envs in a pipeline, and
+            # there might be multiple envs in a command (usually when one comes
+            # from a substitution):
             #   env FOO=1 llc < %s | env BAR=2 llvm-mc | FileCheck %s
+            #   env FOO=1 %{another_env_plus_cmd} | FileCheck %s
+            if cmd_shenv is shenv:
                 cmd_shenv = ShellEnvironment(shenv.cwd, shenv.env)
-            args = updateEnv(cmd_shenv, j.args)
+            args = updateEnv(cmd_shenv, args)
             if not args:
                 raise InternalShellError(j,
                                          "Error: 'env' requires a subcommand")
 
         # Handle in-process builtins.
         #
         # Handle "echo" as a builtin if it is not part of a pipeline. This
         # greatly speeds up tests that construct input files by repeatedly

llvm/utils/lit/tests/Inputs/shtest-env/env-args-nested-none.txt

+# RUN: env env env

llvm/utils/lit/tests/Inputs/shtest-env/env-calls-env.txt

+# Check that internal env can call internal env.
+
+# RUN: env env %{python} print_environment.py \
+# RUN: | FileCheck -check-prefix=CHECK-2-EMPTY %s
+#
+# CHECK-2-EMPTY: BAR = 2
+# CHECK-2-EMPTY: FOO = 1
+
+# RUN: env FOO=2 env BAR=1 %{python} print_environment.py \
+# RUN: | FileCheck -check-prefix=CHECK-2-VAL %s
+#
+# CHECK-2-VAL: BAR = 1
+# CHECK-2-VAL: FOO = 2
+
+# RUN: env -u FOO env -u BAR %{python} print_environment.py \
+# RUN: | FileCheck -check-prefix=CHECK-2-U %s
+#
+# CHECK-2-U-NOT: BAR
+# CHECK-2-U-NOT: FOO
+
+# RUN: env -u FOO BAR=1 env -u BAR FOO=2 %{python} print_environment.py \
+# RUN: | FileCheck -check-prefix=CHECK-2-U-VAL %s
+#
+# CHECK-2-U-VAL-NOT: BAR
+# CHECK-2-U-VAL: FOO = 2
+
+# RUN: env -u FOO BAR=1 env -u BAR FOO=2 env BAZ=3 %{python} print_environment.py \
+# RUN: | FileCheck -check-prefix=CHECK-3 %s
+#
+# CHECK-3-NOT: BAR
+# CHECK-3: BAZ = 3
+# CHECK-3: FOO = 2

llvm/utils/lit/tests/shtest-env.py

 # Check the env command
 #
 # RUN: not %{lit} -j 1 -a -v %{inputs}/shtest-env \
 # RUN: | FileCheck -match-full-lines %s
 #
 # END.
 
 # Make sure env commands are included in printed commands.
 
-# CHECK: -- Testing: 13 tests{{.*}}
+# CHECK: -- Testing: 15 tests{{.*}}
 
 # CHECK: FAIL: shtest-env :: env-args-last-is-assign.txt ({{[^)]*}})
+# CHECK: $ "env" "FOO=1"
 # CHECK: Error: 'env' requires a subcommand
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-env :: env-args-last-is-u-arg.txt ({{[^)]*}})
+# CHECK: $ "env" "-u" "FOO"
 # CHECK: Error: 'env' requires a subcommand
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-env :: env-args-last-is-u.txt ({{[^)]*}})
+# CHECK: $ "env" "-u"
+# CHECK: Error: 'env' requires a subcommand
+# CHECK: error: command failed with exit status: {{.*}}
+
+# CHECK: FAIL: shtest-env :: env-args-nested-none.txt ({{[^)]*}})
+# CHECK: $ "env" "env" "env"
 # CHECK: Error: 'env' requires a subcommand
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-env :: env-args-none.txt ({{[^)]*}})
+# CHECK: $ "env"
 # CHECK: Error: 'env' requires a subcommand
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-env :: env-calls-cd.txt ({{[^)]*}})
 # CHECK: $ "env" "-u" "FOO" "BAR=3" "cd" "foobar"
 # CHECK: Error: 'env' cannot call 'cd'
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-env :: env-calls-colon.txt ({{[^)]*}})
 # CHECK: $ "env" "-u" "FOO" "BAR=3" ":"
 # CHECK: Error: 'env' cannot call ':'
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-env :: env-calls-echo.txt ({{[^)]*}})
 # CHECK: $ "env" "-u" "FOO" "BAR=3" "echo" "hello" "world"
 # CHECK: Error: 'env' cannot call 'echo'
 # CHECK: error: command failed with exit status: {{.*}}
 
+# CHECK: PASS: shtest-env :: env-calls-env.txt ({{[^)]*}})
+# CHECK: $ "env" "env" "{{[^"]*}}" "print_environment.py"
+# CHECK: $ "env" "FOO=2" "env" "BAR=1" "{{[^"]*}}" "print_environment.py"
+# CHECK: $ "env" "-u" "FOO" "env" "-u" "BAR" "{{[^"]*}}" "print_environment.py"
+# CHECK: $ "env" "-u" "FOO" "BAR=1" "env" "-u" "BAR" "FOO=2" "{{[^"]*}}" "print_environment.py"
+# CHECK: $ "env" "-u" "FOO" "BAR=1" "env" "-u" "BAR" "FOO=2" "env" "BAZ=3" "{{[^"]*}}" "print_environment.py"
+# CHECK-NOT: ${{.*}}print_environment.py
+
 # CHECK: FAIL: shtest-env :: env-calls-export.txt ({{[^)]*}})
 # CHECK: $ "env" "-u" "FOO" "BAR=3" "export" "BAZ=3"
 # CHECK: Error: 'env' cannot call 'export'
 # CHECK: error: command failed with exit status: {{.*}}
 
 # CHECK: FAIL: shtest-env :: env-calls-mkdir.txt ({{[^)]*}})
 # CHECK: $ "env" "-u" "FOO" "BAR=3" "mkdir" "foobar"
 # CHECK: Error: 'env' cannot call 'mkdir'

 # CHECK: $ "env" "A_FOO=1" "B_BAR=2" "C_OOF=3" "{{[^"]*}}" "print_environment.py"
 # CHECK-NOT: ${{.*}}print_environment.py
 
 # CHECK: PASS: shtest-env :: mixed.txt ({{[^)]*}})
 # CHECK: $ "env" "A_FOO=999" "-u" "FOO" "{{[^"]*}}" "print_environment.py"
 # CHECK: $ "env" "A_FOO=1" "-u" "FOO" "B_BAR=2" "-u" "BAR" "C_OOF=3" "{{[^"]*}}" "print_environment.py"
 # CHECK-NOT: ${{.*}}print_environment.py
 
-# CHECK: Expected Passes : 3
-# CHECK: Unexpected Failures: 10
+# CHECK: Expected Passes : 4
+# CHECK: Unexpected Failures: 11
 # CHECK-NOT: {{.}}