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

Wed Aug 21 13:41:11 PDT 2019

rnk accepted this revision.
rnk added a comment.

In D65697#1639524 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D65697#1639463>, @probinson wrote:
> 
>> > Do people know why an internal diff was implemented originally? Is there a portability or performance issue?
>>
>> See D39567 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65697