Skip to content
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

GitHub Actions: Prevent ctest invocation error on macOS #2479

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

mcprat
Copy link
Contributor

@mcprat mcprat commented Aug 19, 2024

ping @jhasse @nico

It seems that new versions of CTest now require case-sensitive arguments.

To prevent errors related to case-sensitivity of arguments, use the long option form for verbosity, and fallback to printing out the help message and running normally. get rid of CTest.

While at it, print the version in all cases for reference.

@mcprat
Copy link
Contributor Author

mcprat commented Aug 21, 2024

@nico @jhasse @digit-google I know you all must be very busy, sorry to be impatient on this one, but this is just 1 line that fixes the CI test. Can we do it?

My other PR's CI test states:

Run ctest -C Release -vv
CMake Error: Unknown argument: -vv
CMake Error: Run 'ctest --help' for all supported options.

@mcprat
Copy link
Contributor Author

mcprat commented Aug 22, 2024

a similar fix is part of #2041

@scivision
Copy link
Contributor

Correct--for most of CMake's history, CTest would ignore invalid flags, which was quite confusing. As in this case, where "-vv" did nothing, but now to help avoid confusion, CTest errors on invalid command line flags.

@jhasse
Copy link
Collaborator

jhasse commented Aug 23, 2024

I would prefer a version which avoids ctest all together. Not sure what the advantages of it are besides issue likes this.

There were other issues with ctest in the past and my general opinion of it is, that it should be avoided at all costs and nuked from existence.

@mcprat mcprat force-pushed the workflow-ctest-verbose branch from aad76e5 to c0f1521 Compare August 24, 2024 04:28
@mcprat
Copy link
Contributor Author

mcprat commented Aug 24, 2024

@jhasse ok. no more ctest

@mcprat
Copy link
Contributor Author

mcprat commented Aug 24, 2024

I'm not going to bother changing the title of the PR, if that's ok...

@mcprat mcprat force-pushed the workflow-ctest-verbose branch 2 times, most recently from 1963bf8 to a2df75c Compare August 25, 2024 21:15
@mcprat
Copy link
Contributor Author

mcprat commented Aug 25, 2024

@jhasse I dug deeper and found out that --config is only for multi-config generators

When ctest was used with the "Xcode" generator, it should have been used directly in the subdirectory without a -C option

@mcprat
Copy link
Contributor Author

mcprat commented Aug 25, 2024

It seems that new versions of CTest now require case-sensitive arguments,
resulting in an "unknown argument" error for "-vv", while documentation
shows the argument for the full verbosity option as "-VV".

In CMakeLists.txt, there is currently only 1 test registered to CMake
with add_test(), and that is to run the ninja_test binary.

Since using CTest is not reducing the number of commands used for testing,
fix the testing invocation by replacing CTest with a direct run of the test.

Signed-off-by: Michael Pratt <[email protected]>
@mcprat mcprat force-pushed the workflow-ctest-verbose branch from a2df75c to 6e77cdf Compare August 26, 2024 01:25
@mcprat
Copy link
Contributor Author

mcprat commented Aug 26, 2024

nevermind after reading more documentation I see it is considered a multi-config generator even though many people do not use it that way.

so now, just correcting the path

@digit-google
Copy link
Contributor

Thank you for this, I am glad we can get rid of CTest too. @jhasse, can we submit this asap to unblock all other PRs that fail their checks because of this issue?

@jhasse jhasse merged commit f220dc5 into ninja-build:master Aug 28, 2024
11 checks passed
@digit-google
Copy link
Contributor

Thanks!

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.

4 participants