[libcxx][test] Fix empty.gen selftest on windows #69403
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Duo Wang (wdunicornpro) ChangesUsing Full diff: https://github.com/llvm/llvm-project/pull/69403.diff 1 Files Affected:
diff --git a/libcxx/test/libcxx/selftest/gen.cpp/empty.gen.cpp b/libcxx/test/libcxx/selftest/gen.cpp/empty.gen.cpp
index 1915bede471f5e4..21fa2adddfbc460 100644
--- a/libcxx/test/libcxx/selftest/gen.cpp/empty.gen.cpp
+++ b/libcxx/test/libcxx/selftest/gen.cpp/empty.gen.cpp
@@ -8,4 +8,4 @@
// Make sure we can generate no tests at all
-// RUN: true
+// RUN: cd .
|
|
For the record this problem does not show up when running from bash on windows, so feel free to close this PR if it is not of much interest. |
|
@wdunicornpro I think your branch is using an outdated version of |
Oh, and IMO we want to use the builtin Lit shell everywhere as much as possible, so anything that goes against that is worth fixing. |
| @@ -8,4 +8,4 @@ | |||
|
|
|||
| // Make sure the test passes if it succeeds to run | |||
|
|
|||
| // RUN: true | |||
| // RUN: : | |||
There was a problem hiding this comment.
I kinda wish there was a Lit builtin command called true (and why not false) since this would be a lot more readable, but this LGTM.
CC @jdenny-ornl
There was a problem hiding this comment.
Why are true/false not available in the windows environment that motivated this patch? I'm not a windows user, but I don't recall hearing of this issue before.
There was a problem hiding this comment.
I think those are Bash builtins?
There was a problem hiding this comment.
Lit's test suite has tests where RUN lines containing true/false execute with lit's internal shell (not bash), and they appear to succeed in windows bots. That suggests to me that true/false are available as external commands there. Maybe @dyung can shed some light on that setup.
Is the goal of this patch to relax windows installation requirements, or am I just misunderstanding what's happening?
There was a problem hiding this comment.
My goal is just to ensure that we use Lit builtin commands exclusively whenever possible so as to decouple our test suite from the underlying platform as much as possible. true is not a Lit builtin but we have a Lit builtin equivalent (:), so this patch seems desirable. I'll go ahead and merge this but I agree it'd be interesting to figure out how these tests manage to work on Windows.
There was a problem hiding this comment.
Lit's test suite has tests where RUN lines containing true/false execute with lit's internal shell (not bash), and they appear to succeed in windows bots. That suggests to me that true/false are available as external commands there. Maybe @dyung can shed some light on that setup.
Is the goal of this patch to relax windows installation requirements, or am I just misunderstanding what's happening?
I'm not completely sure what is being asked here, but I took a look at our Windows bot that @jdenny-ornl linked to and while there is a true.exe that seems to be installed with git (C:\Program Files\Git\usr\bin), but unless something is adding that directory to the path during testing, it is NOT a part of the regular path. And indeed, trying to run "true" from a command prompt gives the expected error 'true' is not recognized as an internal or external command, operable program or batch file..
There was a problem hiding this comment.
@dyung Thanks. That's exactly what I was looking for. It looks like lit is adding that directory here based on the following output from that windows bot:
llvm-lit.py: Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\utils\lit\lit\llvm\config.py:46: note: using lit tools: C:\Program Files\Git\usr\bin
wdunicornpro
force-pushed
the
libcxx/test/fix_empty_gen_on_win
branch
from
1353759 to
40fe3bc
Compare
|
LGTM, will merge once CI passes. |
Using
trueas a no-op unfortunately does not work on windows, which fails libcxx lit tests on windows. Usingcd .is a good solution that should work across most platforms, but any suggestion is appreciated.