This is an archive of the discontinued LLVM Phabricator instance.

Fix defines.txt test failure on Windows targets
AbandonedPublic

Authored by thopre on Feb 5 2019, 9:16 AM.

Details

Summary

FileCheck error messages embedding the name of the FileCheck tool vary
in the casing of that name. On Unix it displays 'FileCheck' but on some
Windows system at least it shows 'filecheck.exe' as can be seen in the
defines.txt failure logs available at [1]. This changes the testcase to
only check the error message and leave out the program name.

[1] http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/23393/steps/test/logs/stdio

Diff Detail

Event Timeline

thopre created this revision.Feb 5 2019, 9:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 9:16 AM
jhenderson added inline comments.Feb 5 2019, 9:31 AM
llvm/test/FileCheck/defines.txt
27

Maybe we just don't need to test that "FileCheck" appears in the error message at all? I'm not really convinced it belongs in the error message anyway, but that's a separate issue.

thopre marked an inline comment as done.Feb 5 2019, 9:38 AM
thopre added inline comments.
llvm/test/FileCheck/defines.txt
27

I was wondering about that myself. So what about:

; ERRCLIEQ2: for the -D option: requires a value!

jhenderson accepted this revision.Feb 5 2019, 9:41 AM

I'm about to leave the office for the day, but as long as you remove the portion of the check as suggested, I'm happy to give this an LGTM now.

llvm/test/FileCheck/defines.txt
27

Yes, exactly.

This revision is now accepted and ready to land.Feb 5 2019, 9:41 AM
thopre updated this revision to Diff 185496.Feb 6 2019, 12:28 AM

Remove program name from the string being checked for

thopre edited the summary of this revision. (Show Details)Feb 6 2019, 12:29 AM

Someone beat me to it overnight and went for my initial approach. Is it worth a separate commit to stop testing for FileCheck's name in the error string as suggested or should I abandon this revision?

I guess you might as well abandon it. Up to you though.

thopre abandoned this revision.Feb 6 2019, 2:24 AM

Abandon it as issue has already been fixed.