Skip to content

Fix tools tests on Windows #184

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.

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 1 commit into from
Dec 4, 2024
Merged

Fix tools tests on Windows #184

merged 1 commit into from
Dec 4, 2024

Conversation

DavidTruby
Copy link
Member

The commit 89fcc5e introduced some tests for the tools in the tools/ subdirectory that are always run, but these tests were not working on Windows due to the executables ending in .exe and one of the tests being written in bash.

This commit fixes those issues by directly using the $<TARGET_FILE> generator expressions to get the executable file names, and rewriting the bash test in python.

The commit 89fcc5e introduced some tests for the tools in the
tools/ subdirectory that are always run, but these tests were not
working on Windows due to the executables ending in .exe and one of the
tests being written in bash.

This commit fixes those issues by directly using the $<TARGET_FILE>
generator expressions to get the executable file names, and rewriting
the bash test in python.
Copy link
Contributor

@tarunprabhu tarunprabhu left a comment

Choose a reason for hiding this comment

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

Thanks for this, David. I'll keep some of these in mind for the future. I am afraid Windows compatibility completely slipped my mind when I added these.

@DavidTruby
Copy link
Member Author

No worries, thanks for the quick review! I'm more alarmed that nobody noticed this was broken on Windows until I ran it manually recently than I am that it broke in the first place; are there no bots running these tests on Windows regularly? 😓

@DavidTruby DavidTruby merged commit eee9587 into llvm:main Dec 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants