Page MenuHomePhabricator

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

Authored by delcypher on Thu, Jan 10, 5:42 AM.

Details

Summary

[FileCheck] Don't propagate FILECHECK_DUMP_INPUT_ON_FAILURE and
FILECHECK_OPTS into environment for FileCheck tests.

This fixes the following FileCheck tests:

  • FileCheck/dump-input-enable.txt
  • FileCheck/match-full-lines.txt

when FILECHECK_DUMP_INPUT_ON_FAILURE is set in the environment.

By default llvm-lit propagates FILECHECK_DUMP_INPUT_ON_FAILURE and
FILECHECK_OPTS from llvm-lit's environment into the test environment.
Unfortunately this can break FileCheck's tests because they expect that
these environment variables not to be set.

rdar://problem/47176262

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher created this revision.Thu, Jan 10, 5:42 AM
delcypher edited the summary of this revision. (Show Details)Thu, Jan 10, 5:51 AM

I've been wanting to fix this too. Thanks for working on it. A few issues:

  • FILECHECK_OPTS should be cleared too.
  • The last time I checked (but it's been a little while), the lit test suite had problems with these variables too.
  • 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'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.

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

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.

delcypher updated this revision to Diff 181065.Thu, Jan 10, 7:47 AM

Also unset FILECHECK_OPTS.

delcypher retitled this revision from Fix FileCheck tests when `FILECHECK_DUMP_INPUT_ON_FAILURE` is set in the environment that llvm-lit is executed in. to [FileCheck] Don't propagate `FILECHECK_DUMP_INPUT_ON_FAILURE` and `FILECHECK_OPTS` into environment for FileCheck tests..Thu, Jan 10, 7:48 AM
delcypher edited the summary of this revision. (Show Details)

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.

george.karpenkov accepted this revision.Thu, Jan 10, 9:16 AM

Thanks! Probably should have thought about that one!

I think this patch is fine as it is: it's small, self-contained, and makes the situation strictly better.

This revision is now accepted and ready to land.Thu, Jan 10, 9:16 AM

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.

If it's straight forward I'll land it in a separate patch.

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

https://bugs.llvm.org/show_bug.cgi?id=40284

This revision was automatically updated to reflect the committed changes.

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.

If it's straight forward I'll land it in a separate patch.

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

https://bugs.llvm.org/show_bug.cgi?id=40284

Great. Thanks!

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.

If it's straight forward I'll land it in a separate patch.

I've landed a straight forward fix in r350854.