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

[Joel E. Denny via Phabricator via llvm-commits]

jdenny marked 3 inline comments as done.
jdenny added inline comments.


================
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:
> jdenny wrote:
> > rnk wrote:
> > > 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.
> > > 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.
> > 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?
> 
> 
> 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.


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

https://reviews.llvm.org/D65697