[lit] Fix discovery test on Windows

[modocache]

Summary

Many lit tests fail when run on Windows, because file path
separators are hard-coded as '/', whereas on Windows they're '\'.

Fix one such test, discovery.py, by using FileCheck variables.
As far as I understood, FileCheck does not contain a built-in variable
that matches '/' or '\', so I do this manually.

An alternative approach would be to add such a variable to FileCheck,
but for now this approach seemed less intrusive.

[modocache]

@ddunbar suggested that this might be better addressed in the lit config logic. I'll try that.

[modocache]

Actually, I'm not sure this can be fixed in LitConfig.py. I could force the lit output to always use Unix path separators, even on Windows, but that would be inconvenient for Windows developers that want to copy and paste the paths.

Still, matching SEP like this is gross. Do any reviewers feel strongly about this? Would it be better if I added a way to match path separators to FileCheck itself?

[delcypher]

In D27701#621835, @modocache wrote:

Actually, I'm not sure this can be fixed in LitConfig.py. I could force the lit output to always use Unix path separators, even on Windows, but that would be inconvenient for Windows developers that want to copy and paste the paths.

Still, matching SEP like this is gross. Do any reviewers feel strongly about this? Would it be better if I added a way to match path separators to FileCheck itself?

If it's possible I would prefer always using Unix path separators. / is supported as a path separator on Windows (in some cases) . E.g. in cmd.exe I can type

C:\> cd c:/Users/foo
c:\Users\foo>

and the command works correctly. It doesn't always work however so we need to test if lit would perform as expected in the "copy and paste test paths" scenario.

[modocache]

That makes sense, thanks for the feedback @delcypher.

Is there a strong reason you'd prefer / in all cases, though?

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 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?

[delcypher]

In 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 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.

[modocache]

@delcypher Sorry for the incredibly late reply to your comment here. You mentioned:

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.

What would you think of, instead of fixing all of the Windows tests first, we mark them as XFAIL: windows? That way, we could un-revert rL257221, which makes it much easier to run the lit tests in the first place, and then we can take our time fixing the Windows tests. This would be my preferred order, since debugging some of these WIndows test failures is a pain.

In any case, your reviews certainly weren't holding back progress! On the contrary, I'm sorry I got sidetracked. I'd love to add a check-lit CMake target, and if getting the tests to pass on Windows is a prerequisite for that, I can devote some time to doing so in the next few weeks.

[beanz]

I think it is better to XFAIL Windows and have test coverage on non-windows systems than to have the tests not running anywhere.

If you XFAIL them though please file a bug on bugzilla, and comment with the bug in the tests near the XFAIL line to make it easier to track.

[modocache]

Awesome, thanks for the suggestion @beanz!

[modocache]

The equivalent of this change was committed by @rnk in rL309194. I sort of prefer this because it verifies the same separator is used throughout, instead of accepting either. But either way works for me!

[rnk]

In D27701#626275, @modocache wrote:

That makes sense, thanks for the feedback @delcypher.

Is there a strong reason you'd prefer / in all cases, though?

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 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'd be in favor of always using forward slashes in lit output, personally. I just went ahead and committed that to try to get the tests green again. There are fewer and fewer utilities that don't understand forward slashes.

utils/lit/tests/discovery.py

 # Check the basic discovery process, including a sub-suite.
 #
 # RUN: %{lit} %{inputs}/discovery \
 # RUN:   -j 1 --debug --show-tests --show-suites \
 # RUN:   -v > %t.out 2> %t.err
 # RUN: FileCheck --check-prefix=CHECK-BASIC-OUT < %t.out %s
 # RUN: FileCheck --check-prefix=CHECK-BASIC-ERR < %t.err %s
 #
-# CHECK-BASIC-ERR: loading suite config '{{.*}}/discovery/lit.cfg'
-# CHECK-BASIC-ERR-DAG: loading suite config '{{.*}}/discovery/subsuite/lit.cfg'
-# CHECK-BASIC-ERR-DAG: loading local config '{{.*}}/discovery/subdir/lit.local.cfg'
+# CHECK-BASIC-ERR: loading suite config '{{.*}}[[SEP:[/\\]+]]discovery[[SEP]]lit.cfg'
+# CHECK-BASIC-ERR-DAG: loading suite config '{{.*}}[[SEP]]discovery[[SEP]]subsuite[[SEP]]lit.cfg'
+# CHECK-BASIC-ERR-DAG: loading local config '{{.*}}[[SEP]]discovery[[SEP]]subdir[[SEP]]lit.local.cfg'
 #
 # CHECK-BASIC-OUT: -- Test Suites --
 # CHECK-BASIC-OUT:   sub-suite - 2 tests
-# CHECK-BASIC-OUT:     Source Root: {{.*/discovery/subsuite$}}
-# CHECK-BASIC-OUT:     Exec Root  : {{.*/discovery/subsuite$}}
+# CHECK-BASIC-OUT:     Source Root: {{.*}}[[SEP:[/\\]+]]discovery[[SEP]]subsuite{{$}}
+# CHECK-BASIC-OUT:     Exec Root  : {{.*}}[[SEP]]discovery[[SEP]]subsuite{{$}}
 # CHECK-BASIC-OUT:   top-level-suite - 3 tests
-# CHECK-BASIC-OUT:     Source Root: {{.*/discovery$}}
-# CHECK-BASIC-OUT:     Exec Root  : {{.*/discovery$}}
+# CHECK-BASIC-OUT:     Source Root: {{.*}}[[SEP]]discovery{{$}}
+# CHECK-BASIC-OUT:     Exec Root  : {{.*}}[[SEP]]discovery{{$}}
 #
 # CHECK-BASIC-OUT: -- Available Tests --
 # CHECK-BASIC-OUT: sub-suite :: test-one
 # CHECK-BASIC-OUT: sub-suite :: test-two
 # CHECK-BASIC-OUT: top-level-suite :: subdir/test-three
 # CHECK-BASIC-OUT: top-level-suite :: test-one
 # CHECK-BASIC-OUT: top-level-suite :: test-two
 

 # Check discovery when using an exec path.
 #
 # RUN: %{lit} %{inputs}/exec-discovery \
 # RUN:   -j 1 --debug --show-tests --show-suites \
 # RUN:   -v > %t.out 2> %t.err
 # RUN: FileCheck --check-prefix=CHECK-ASEXEC-OUT < %t.out %s
 # RUN: FileCheck --check-prefix=CHECK-ASEXEC-ERR < %t.err %s
 #
-# CHECK-ASEXEC-ERR: loading suite config '{{.*}}/exec-discovery/lit.site.cfg'
-# CHECK-ASEXEC-ERR: load_config from '{{.*}}/discovery/lit.cfg'
-# CHECK-ASEXEC-ERR: loaded config '{{.*}}/discovery/lit.cfg'
-# CHECK-ASEXEC-ERR: loaded config '{{.*}}/exec-discovery/lit.site.cfg'
-# CHECK-ASEXEC-ERR-DAG: loading suite config '{{.*}}/discovery/subsuite/lit.cfg'
-# CHECK-ASEXEC-ERR-DAG: loading local config '{{.*}}/discovery/subdir/lit.local.cfg'
+# CHECK-ASEXEC-ERR: loading suite config '{{.*}}[[SEP:[/\\]+]]exec-discovery[[SEP]]lit.site.cfg'
+# CHECK-ASEXEC-ERR: load_config from '{{.*}}[[SEP]]discovery[[SEP]]lit.cfg'
+# CHECK-ASEXEC-ERR: loaded config '{{.*}}[[SEP]]discovery[[SEP]]lit.cfg'
+# CHECK-ASEXEC-ERR: loaded config '{{.*}}[[SEP]]exec-discovery[[SEP]]lit.site.cfg'
+# CHECK-ASEXEC-ERR-DAG: loading suite config '{{.*}}[[SEP]]discovery[[SEP]]subsuite[[SEP]]lit.cfg'
+# CHECK-ASEXEC-ERR-DAG: loading local config '{{.*}}[[SEP]]discovery[[SEP]]subdir[[SEP]]lit.local.cfg'
 #
 # CHECK-ASEXEC-OUT: -- Test Suites --
 # CHECK-ASEXEC-OUT:   sub-suite - 2 tests
-# CHECK-ASEXEC-OUT:     Source Root: {{.*/discovery/subsuite$}}
-# CHECK-ASEXEC-OUT:     Exec Root  : {{.*/discovery/subsuite$}}
+# CHECK-ASEXEC-OUT:     Source Root: {{.*}}[[SEP:[/\\]+]]discovery[[SEP]]subsuite{{$}}
+# CHECK-ASEXEC-OUT:     Exec Root  : {{.*}}[[SEP]]discovery[[SEP]]subsuite{{$}}
 # CHECK-ASEXEC-OUT:   top-level-suite - 3 tests
-# CHECK-ASEXEC-OUT:     Source Root: {{.*/discovery$}}
-# CHECK-ASEXEC-OUT:     Exec Root  : {{.*/exec-discovery$}}
+# CHECK-ASEXEC-OUT:     Source Root: {{.*}}[[SEP]]discovery{{$}}
+# CHECK-ASEXEC-OUT:     Exec Root  : {{.*}}[[SEP]]exec-discovery{{$}}
 #
 # CHECK-ASEXEC-OUT: -- Available Tests --
 # CHECK-ASEXEC-OUT: sub-suite :: test-one
 # CHECK-ASEXEC-OUT: sub-suite :: test-two
 # CHECK-ASEXEC-OUT: top-level-suite :: subdir/test-three
 # CHECK-ASEXEC-OUT: top-level-suite :: test-one
 # CHECK-ASEXEC-OUT: top-level-suite :: test-two
 

 # suite located inside the test source root.
 #
 # RUN: %{lit} \
 # RUN:     %{inputs}/exec-discovery-in-tree/obj/ \
 # RUN:   -j 1 --show-tests --show-suites -v > %t.out
 # RUN: FileCheck --check-prefix=CHECK-ASEXEC-INTREE < %t.out %s
 #
 #      CHECK-ASEXEC-INTREE:   exec-discovery-in-tree-suite - 1 tests
-# CHECK-ASEXEC-INTREE-NEXT:     Source Root: {{.*/exec-discovery-in-tree$}}
-# CHECK-ASEXEC-INTREE-NEXT:     Exec Root  : {{.*/exec-discovery-in-tree/obj$}}
+# CHECK-ASEXEC-INTREE-NEXT:     Source Root: {{.*}}[[SEP:[/\\]+]]exec-discovery-in-tree{{$}}
+# CHECK-ASEXEC-INTREE-NEXT:     Exec Root  : {{.*}}[[SEP]]exec-discovery-in-tree[[SEP]]obj{{$}}
 # CHECK-ASEXEC-INTREE-NEXT: -- Available Tests --
 # CHECK-ASEXEC-INTREE-NEXT: exec-discovery-in-tree-suite :: test-one