[PATCH] D27701: [lit] Fix discovery test on Windows

[Dan Liew via Phabricator via llvm-commits]

delcypher added a comment.

In https://reviews.llvm.org/D27701#626275, @modocache wrote:

> That makes sense, thanks for the feedback @delcypher.
>
> Is there a strong reason you'd prefer `/` in all cases, though?


Simplicity. Although I should also note I am incredibly biased (I have an extreme dislike of Windows) so my opinion on how we should handle Lit's behaviour on Windows should be taken with a grain of salt.

> I ask because it seems more correct to output paths using each platform's native path separators. Yes, we could modify the logic in `LitConfig.py` to use `"/".join()` instead of `os.path.join()`, and replace `\` with `/` in the paths we output, but that seems like additional complexity for what is, in my opinion, marginal benefit. True, this particular test is uglier because of the difference in path separators, but this and https://reviews.llvm.org/D27908 are the only two tests that need to be modified for this reason. So personally, I think it makes more sense to change the tests, than it does to change lit's behavior.
> 
> Thoughts?

I agree that is more correct (and expected) to use the platform's native path separator, at least from a user perspective. The problem with the changes here though is the test now allows previously forbidden behaviour: We could emit the wrong slashes for the platform (i.e. `\` on UNIX like systems and `/` on Windows) and the test would still pass. I wonder if there is a way we could use lit's `${path_sep}` substitution to help us here? I can't see an obvious way to do it because it needs to appear in a `RUN:` line and would somehow need to be given to FileCheck.

Anyway I feel like I might be holding back progress here. I think it would be better to get all the tests passing so we can run under Windows so we can turn on the build bots and then try to figure out how to improve the tests. If you feel the same way and are happy to look into fixing up the tests after we get the build bots up then I'll be happy to approve this patch and the others that get the tests working under Windows.


https://reviews.llvm.org/D27701