This is an archive of the discontinued LLVM Phabricator instance.

[test] Avoid unportable echo in Other/lit-quoting.txt
ClosedPublic

Authored by ro on Aug 13 2021, 6:06 AM.

Details

Summary

LLVM :: Other/lit-quoting.txt currently FAILs on Solaris:

llvm/test/Other/lit-quoting.txt:8:9: error: CHECK2: expected string not found in input
CHECK2: {{^a\[b\\c$}}
        ^
<stdin>:1:1: note: scanning from here
a[b
^

This happens because echo with backslashes or special characters is unportable, as
extensively documented in the Autoconf manual. In the case at hand,
echo 'a[b\c' yields a[b\c on Linux, but a[b (no newline) on Solaris.

This patch fixes this by using the portable alternative suggested in the Autoconf manual.

Tested on amd64-pc-solaris2.11, sparcv9-sun-solaris2.11, and x86_64-pc-linux-gnu.

Diff Detail

Event Timeline

ro created this revision.Aug 13 2021, 6:06 AM
ro requested review of this revision.Aug 13 2021, 6:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2021, 6:06 AM

I'm surprised to see that this actually seems to pass in the Windows CI environment (even if there are plenty of other unrelated failures in the CI run). On Windows, these tests aren't executed by a unix shell, but by the python lit internal shell, and echo is one of the commands handled internally there, but printf isn't.

rnk added a comment.Aug 13 2021, 1:00 PM

... as extensively documented in the Autoconf manual ...

I hope I never have the joy of reading it. :)

I'm surprised to see that this actually seems to pass in the Windows CI environment (even if there are plenty of other unrelated failures in the CI run). On Windows, these tests aren't executed by a unix shell, but by the python lit internal shell, and echo is one of the commands handled internally there, but printf isn't.

So, if I understand, the printf variant is actually a *better* test than the echo test because lit will actually call out to an external program. At least, I do believe this version does test the codepath that we care about, right? If you agree, I think this is fine.

I'm surprised to see that this actually seems to pass in the Windows CI environment (even if there are plenty of other unrelated failures in the CI run). On Windows, these tests aren't executed by a unix shell, but by the python lit internal shell, and echo is one of the commands handled internally there, but printf isn't.

So, if I understand, the printf variant is actually a *better* test than the echo test because lit will actually call out to an external program. At least, I do believe this version does test the codepath that we care about, right? If you agree, I think this is fine.

Hmm, I'm pretty sure I tested these to make sure they did test the thing that mattered (invoking external executables - and that the test did fail before applying the fix).

In any case, I checked now, and both GnuWin (which I think some buildbots use?) and Git Bash (which the CI runner uses) contain a printf.exe, and it does seem to work with GnuWin's printf.exe too, so I guess it should be fine then.

mstorsjo accepted this revision.Aug 14 2021, 12:50 PM

I guess I shouldn't be blocking this, so as this seems to be working on Windows, LGTM.

This revision is now accepted and ready to land.Aug 14 2021, 12:50 PM
This revision was landed with ongoing or failed builds.Aug 14 2021, 3:21 PM
This revision was automatically updated to reflect the committed changes.