This is an archive of the discontinued LLVM Phabricator instance.

Quote '?' in llvm-rc test
ClosedPublic

Authored by rnk on Jul 25 2017, 1:14 PM.

Download Raw Diff

Details

Reviewers

mnbvmar
ecbeckmann
jgravelle-google

Commits

rG43c2b131d9a1: Quote '?' in llvm-rc test
rL309133: Quote '?' in llvm-rc test

Summary

Bash interperets the '?' character as matching an arbitrary character.
On systems that have a file or directory with exactly one character in
their root directory, '/?' gets reinterpreted into that pathname, which
fails to match the expected Help text for llvm-rc.
This patch quotes the '/?' to avoid that edge case.

Diff Detail

Build Status

Buildable 8581
Build 8581: arc lint + arc unit

Event Timeline

jgravelle-google created this revision.Jul 25 2017, 1:14 PM

I'm not sure off the top of my head if this works the same on Windows' cmd.exe. If someone can check this on a Windows box that would be useful.

Harbormaster completed remote builds in B8581: Diff 108144.Jul 25 2017, 1:16 PM

In D35852#820587, @jgravelle-google wrote:

I'm not sure off the top of my head if this works the same on Windows' cmd.exe. If someone can check this on a Windows box that would be useful.

I believe this is not the case in cmd.exe, since most commands use /? to display the help text.

Anyways, this explains some of the failures I've seen running help text tests.

It seems that single-quotes fail on Windows ('/h' is considered a whole argument, including the quotes). Double-quotes should work, though.

In D35852#820686, @mnbvmar wrote:

It seems that single-quotes fail on Windows ('/h' is considered a whole argument, including the quotes). Double-quotes should work, though.

We don't actually use cmd to run tests on Windows, so the single quotes should work. We use lit's internal shell, which is effectively a limited implementation of bash.

This revision is now accepted and ready to land.Jul 25 2017, 2:39 PM

ruiu added a subscriber: ruiu.Jul 25 2017, 2:43 PM

ruiu added inline comments.

test/tools/llvm-rc/helpmsg.test
1	nit: you don't need to quote this, but only '?'

We have hit this corner case in our internal Windows bot! I can confirm that your fix, fixes it on our bot, so please submit it when you can!

rnk commandeered this revision.Jul 26 2017, 9:24 AM

rnk edited reviewers, added: jgravelle-google; removed: rnk.

This revision now requires review to proceed.Jul 26 2017, 9:25 AM

rnk marked an inline comment as done.Jul 26 2017, 9:25 AM

Closed by commit rL309133: Quote '?' in llvm-rc test (authored by rnk). · Explain WhyJul 26 2017, 9:28 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents

Path

Size

test/

tools/

llvm-rc/

helpmsg.test

4 lines

Diff 108144

test/tools/llvm-rc/helpmsg.test

	; RUN: llvm-rc /h > %t1			; RUN: llvm-rc '/h' > %t1
				ruiuUnsubmitted Done Reply Inline Actions nit: you don't need to quote this, but only '?' ruiu: nit: you don't need to quote this, but only '?'
	; RUN: llvm-rc /? > %t2			; RUN: llvm-rc '/?' > %t2
	; RUN: diff %t1 %t2			; RUN: diff %t1 %t2
	; RUN: FileCheck -input-file=%t1 %s			; RUN: FileCheck -input-file=%t1 %s

	; CHECK: OVERVIEW: Resource Converter			; CHECK: OVERVIEW: Resource Converter
	; CHECK-DAG: USAGE: rc [options] <inputs>			; CHECK-DAG: USAGE: rc [options] <inputs>
	; CHECK-DAG: OPTIONS:			; CHECK-DAG: OPTIONS:
	; CHECK-NEXT: /? Display this help and exit.			; CHECK-NEXT: /? Display this help and exit.
	; CHECK-NEXT: /D <value> Define a symbol for the C preprocessor.			; CHECK-NEXT: /D <value> Define a symbol for the C preprocessor.
	Show All 10 Lines