Skip to content

[libcxx][test] Fix empty.gen selftest on windows #69403

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 19, 2023
Merged

[libcxx][test] Fix empty.gen selftest on windows #69403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1798c45
[libcxx][test] Fix empty.gen selftest on windows
wdunicornpro Oct 18, 2023
40fe3bc
refine with Lit builtin command
wdunicornpro Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libcxx/test/libcxx/selftest/gen.cpp/empty.gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@

// Make sure we can generate no tests at all

// RUN: true
// RUN: :
2 changes: 1 addition & 1 deletion libcxx/test/libcxx/selftest/sh.cpp/run-success.sh.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@

// Make sure the test passes if it succeeds to run

// RUN: true
// RUN: :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those are Bash builtins?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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