This is an archive of the discontinued LLVM Phabricator instance.

Differential D56541

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

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

Details

Reviewers
jdenny
probinson
george.karpenkov
Commits
rG4d8c8fe62e8d: [FileCheck] Don't propagate `FILECHECK_DUMP_INPUT_ON_FAILURE` and…
rL350850: [FileCheck] Don't propagate `FILECHECK_DUMP_INPUT_ON_FAILURE` and
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

Event Timeline

delcypher created this revision.Jan 10 2019, 5:42 AM
Harbormaster completed remote builds in B26626: Diff 181037.Jan 10 2019, 5:42 AM
delcypher edited the summary of this revision. (Show Details)Jan 10 2019, 5:51 AM
jdenny added a comment.Jan 10 2019, 7:02 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.
delcypher added a comment.Jan 10 2019, 7:33 AM
In 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.

  • 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.Jan 10 2019, 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..Jan 10 2019, 7:48 AM
delcypher edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B26639: Diff 181065.Jan 10 2019, 7:49 AM
jdenny added a comment.Jan 10 2019, 8:20 AM
In D56541#1352784, @delcypher wrote:
In 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.

george.karpenkov accepted this revision.Jan 10 2019, 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.Jan 10 2019, 9:16 AM
delcypher added a comment.Jan 10 2019, 9:22 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

Closed by commit rL350850: [FileCheck] Don't propagate `FILECHECK_DUMP_INPUT_ON_FAILURE` and (authored by delcypher). · Explain WhyJan 10 2019, 9:27 AM
This revision was automatically updated to reflect the committed changes.
jdenny added a comment.Jan 10 2019, 9:29 AM
In D56541#1352955, @delcypher wrote:

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!

delcypher added a comment.Jan 10 2019, 9:52 AM
In D56541#1352961, @jdenny wrote:
In D56541#1352955, @delcypher wrote:

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.

Revision Contents

PathSize
test/
FileCheck/
8 lines

Diff 181037

test/FileCheck/lit.local.cfg

  • This file was added.
# Remove FILECHECK_DUMP_INPUT_ON_FAILURE from environment
# because the tests expect that environment variable is
# normally unset.
file_check_dump_env_var = 'FILECHECK_DUMP_INPUT_ON_FAILURE'
if file_check_dump_env_var in config.environment:
lit_config.note('Removing {} from environment for FileCheck tests'.format(
file_check_dump_env_var))
config.environment.pop(file_check_dump_env_var)