This is an archive of the discontinued LLVM Phabricator instance.

Fix test Clang :: Driver/cl-response-file.c for Solaris
ClosedPublic

Authored by dyung on Jun 21 2019, 3:45 PM.

Details

Summary

This is a follow up to D63600 that attempts to fix the test to work on all platforms including Windows (which the previous patch broke) and Solaris (which is the original motivation for this patch).

Diff Detail

Repository
rL LLVM

Event Timeline

dyung created this revision.Jun 21 2019, 3:45 PM
rnk added a comment.Jun 21 2019, 4:24 PM

I would almost prefer to XFAIL this on Solaris, I feel like the intent is clearer with echo.

ro added a comment.Jun 21 2019, 4:30 PM
In D63678#1554508, @rnk wrote:

I would almost prefer to XFAIL this on Solaris, I feel like the intent is clearer with echo.

Please have a look at what the autoconf manual has to say on echo vs. printf:

https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/autoconf.html#Limitations-of-Builtins

That's the reason configure has switched to use printf instead, which my testing in https://reviews.llvm.org/D63600 confirms: echo
is a nightmare in cross-platform use. Besides, there are already many uses of printf in the testsuite, so there's no reason to be reluctant
about this one. Besides, there's no reason whatsoever that the test wouldn't fail again on the next target due to issues with echo.

rnk added a comment.Jun 21 2019, 4:43 PM
In D63678#1554513, @ro wrote:

Please have a look at what the autoconf manual has to say on echo vs. printf:

Yikes, fair enough.

I actually really wish lit had some support for the here-document pattern:

cat <<EOF
$foo
EOF

We could replace many uses of echo in our test suite with this, but it's not clear how it interacts when prefixed with # RUN: , and we'd have to implement it in the builtin lit shell for Windows. I proposed a tool to extract such tagged regions from a file before, but some senior contributors opposed it.

rnk accepted this revision.Jun 24 2019, 1:28 PM

lgtm

This revision is now accepted and ready to land.Jun 24 2019, 1:28 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2019, 3:26 PM