[PATCH] D113130: [llvm-libtool-darwin] Print a warning if object file names are repeated

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

In D113130#3252504 <https://reviews.llvm.org/D113130#3252504>, @mstorsjo wrote:

> In D113130#3251852 <https://reviews.llvm.org/D113130#3251852>, @smeenai wrote:
>
>> For the test where we specify one file via `-l` and the other via its path, the output on Windows looks like
>>
>>   warning: file 'L-and-l.test.tmp-input1.o' was specified multiple times.
>>   in: C:\Users\smeenai\llvm-project\build\Release\test\tools\llvm-libtool-darwin\Output\L-and-l.test.tmp/copy/L-and-l.test.tmp-input1.o
>>   in: C:\Users\smeenai\llvm-project\build\Release\test\tools\llvm-libtool-darwin\Output\L-and-l.test.tmp/copy\L-and-l.test.tmp-input1.o
>>
>> Note that the last slash is a forward slash in one path and a backslash in the other. The forward slash is coming from our path specification in the test, and the backslash is coming from `sys::path::append` in libtool (to concatenate `-L` directories and `-l` names).
>>
>> One way to fix this would be to introduce a new `%{sep}` substitution in lit (similar to the existing `%{pathsep}`), so we could use the appropriate slashes for the platform in our test automatically. Another would be to use `sys::path::Style::windows_slash` in our `sys::path::append` call, but that could break with `\\?\` paths, which can't use forward slashes.
>>
>> @mstorsjo, I believe you have a bunch of experience dealing with slash differences on Windows. Any suggestions on the best way to proceed here?
>
> Hmm... Overall I'm not a huge fan of tests like `FileCheck -DSOMETHING=%t/suffix` or similar, because as we've noticed, the FileCheck literal checks are quite brittle for such things - especially as we can configure which kind of separator that `sys::path::append` uses too. Although I guess we could mirror that into the lit subtsitutions too. (Also, for such cases, we could maybe make other lit substitutions, like %t, use the same path separator as is configured to be the default for `sys::path` - that would help fix a bunch of test cases that still are broken with forward slash on Windows.) But it's quite unintuitive to write the match patterns for such cases, to try to mimic exactly what combination of slashes you end up with in each case (when considering people writing tests on unix without access to testing thigs on Windows.)
>
> I don't think considering using `sys::path::Style::windows_slash` explicitly is the right way to go either. I don't think we should complicate the code to do something non-obvious just for the sake of making the tools produce the path that the test framework expects.
>
> For cases within code itself, I've considered adding a `sys::path::equals` that accepts either slash direction, and which would be case insensitive on windows (and darwin?). I wonder how hard it would be to add something similar to FileCheck. I don't offhand know exactly how it would work... But I think something like that would be good for easily writing testcases on unix, without spending lots of unnecessary fiddling on making them pass on Windows.

Thanks for the suggestions!

I'm pretty short on time right now, so I just loosened the test conditionals to make it pass. I agree that a slash-agnostic path comparison would be super useful for FileCheck. I imagine you could implement it via regex (which FileCheck already has support for), but I've never looked into FileCheck's code much to say for sure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113130/new/

https://reviews.llvm.org/D113130