Page MenuHomePhabricator

[lit] Fix internal env calling env
ClosedPublic

Authored by jdenny on Aug 3 2019, 7:17 AM.

Details

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.

Diff Detail

Event Timeline

jdenny created this revision.Aug 3 2019, 7:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2019, 7:17 AM
Herald added a subscriber: delcypher. · View Herald Transcript

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.

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 updated this revision to Diff 215390.Aug 15 2019, 7:10 AM

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

jdenny edited the summary of this revision. (Show Details)Aug 15 2019, 7:11 AM
jdenny set the repository for this revision to rG LLVM Github Monorepo.

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

mgorny added inline comments.Aug 19 2019, 7:29 PM
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 marked 3 inline comments as done.Aug 20 2019, 7:01 AM
jdenny added inline comments.
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 updated this revision to Diff 216180.Aug 20 2019, 9:26 AM

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

jdenny marked 3 inline comments as done.Aug 20 2019, 9:31 AM
rnk added inline comments.Aug 20 2019, 10:59 AM
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 marked 2 inline comments as done.Aug 20 2019, 11:43 AM
jdenny added inline comments.
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 added inline comments.Aug 20 2019, 11:53 AM
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 marked an inline comment as done.Aug 20 2019, 12:16 PM
jdenny added inline comments.
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 added a comment.EditedAug 20 2019, 12:46 PM

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).

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.

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 updated this revision to Diff 216252.Aug 20 2019, 2:19 PM
jdenny set the repository for this revision to rG LLVM Github Monorepo.

Replace ValueError with InternalShellError.

jdenny marked an inline comment as done.Aug 20 2019, 2:21 PM
jdenny added inline comments.
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 updated this revision to Diff 216286.Aug 20 2019, 4:29 PM

Rebase onto D66506.

jdenny marked 3 inline comments as done.Aug 20 2019, 4:38 PM
jdenny added inline comments.
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 accepted this revision.Aug 20 2019, 4:49 PM

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.

This revision is now accepted and ready to land.Aug 20 2019, 4:49 PM
jdenny marked an inline comment as done.Aug 20 2019, 5:00 PM
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 updated this revision to Diff 216384.Aug 21 2019, 6:40 AM
jdenny retitled this revision from [lit] Fix internal env calling internal commands to [lit] Fix internal env calling env.
jdenny edited the summary of this revision. (Show Details)
jdenny set the repository for this revision to rG LLVM Github Monorepo.

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

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.

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 accepted this revision.Aug 21 2019, 1:41 PM

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.

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.

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 added a comment.Aug 21 2019, 2:30 PM

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.

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.

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.

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....

This revision was automatically updated to reflect the committed changes.