[PATCH] D56541: [FileCheck] Don't propagate `FILECHECK_DUMP_INPUT_ON_FAILURE` and `FILECHECK_OPTS` into environment for FileCheck tests.

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

jdenny added a comment.

In D56541#1352784 <https://reviews.llvm.org/D56541#1352784>, @delcypher wrote:

> In D56541#1352721 <https://reviews.llvm.org/D56541#1352721>, @jdenny wrote:
>
> > I've been wanting to fix this too.  Thanks for working on it.  A few issues:
> >
> > - `FILECHECK_OPTS` should be cleared too.
>
>
> Sure I can fix that one.


Thanks.

>> - The last time I checked (but it's been a little while), the lit test suite had problems with these variables too.
> 
> LLVM and lit's test suites pass for me
> 
>   FILECHECK_DUMP_INPUT_ON_FAILURE=1 bin/llvm-lit -vs test/
>   FILECHECK_DUMP_INPUT_ON_FAILURE=1 bin/llvm-lit -v utils/lit/

I checked again (I happen to be at r350441), and the trouble for lit's test suite is with `FILECHECK_OPTS=-v`.  Nevertheless, theoretically either variable could be a problem in the future.

>> - This approach makes these environment variables unhelpful for this test suite, but they don't have to be.  I think a better solution is to create a command, perhaps a lit substitution named FileCheckee, that clears these environment variables and then calls FileCheck.  Throughout this test suite, any FileCheck call under test would be replaced with FileCheckee.  Other FileCheck calls, whose job is to verify correct output from FileCheckee or from other commands, would still be affected by these environment variables.

I should add that, for cases where the test needs to pass these variables to a FileCheck call under test, FileCheckee could copy, for example, `FILECHECKEE_OPTS` to `FILECHECK_OPTS` before calling FileCheck.

> Okay I get the idea. This is a much more invasive change however. Would it be okay to commit this (with `FILECHECK_OPTS` also stripped out) and do your suggestion as a follow up patch?
>  The reason I'd prefer to do this is because this change is intended to fix some internal build bots. Due to where we are in the release cycle the changes I make need to be simple (i.e. the smaller the patch the better) in order to be accepted by internal reviewers.

This patch does improve the situation for everyone, and it's easy to revert, so I'm fine with that.  But could you fix lit's test suite too?  Should be easy, right?  I'm not aware of any other test suites that are sensitive to FileCheck's output in this manner.

Unless you plan to write the followup patch immediately, could you please add a bugzilla and cc me?

Thanks.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56541