[PATCH] D65697: [lit] Fix internal env calling internal commands

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:247
         # 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.
----------------
jdenny wrote:
> rnk wrote:
> > Hm, I bet we shouldn't handle `-u` for `export`.
> Yeah, that seems wrong.  Fixing that surely belongs in a separate patch though.  Agreed?
> 
> Thanks for the review.
Yep, definitely separable.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:803
+
+    # Extract env commands and their arguments so we don't overlook any shell
+    # builtin.  Don't modify the original args as we'll need to process the
----------------
jdenny wrote:
> rnk wrote:
> > 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.
> > Does that seem straightforward?
> 
> I'll give it a shot.
> 
> By the way, a lot of the `ValueError`s here really should be `InternalShellError`s, 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?
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65697/new/

https://reviews.llvm.org/D65697