-
Notifications
You must be signed in to change notification settings - Fork 1.1k
CMake.ctest() runner new tools.cmake:ctest_args conf #19198
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
CMake.ctest() runner new tools.cmake:ctest_args conf #19198
Conversation
| # Remove --quiet if incompatible with new args, as latest arg doesn't win in ctest | ||
| if any(a in ("--debug", "--verbose", "--extra-verbose") for a in extra_args): | ||
| arg_list = [a for a in arg_list if a != "--quiet"] | ||
| arg_list.extend(extra_args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can end up with duplicated arguments if they are declared both in the ctest_args and cli_args? I guess it's fine but maybe we want to filter them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is possible that some args are duplicated. I only managed the --quiet because it is clearly incompatible to the others and would defeat one of the most basic and probably most likely usages that is to change the output.
| # Options such as --verbose, --extra-verbose, and --debug are | ||
| # ignored if --quiet is specified. | ||
| arg_list = [a for a in arg_list if a not in ("--quiet", "--verbose")] | ||
| arg_list.append(f"--{verbosity}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related directly with this PR, but checking this:
arg_list.append(f"--{verbosity}")
can't this break in the future if we extend the conf to accept other choices than "quiet", "verbose" and add an unsupported argument to ctest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that this conf is used in many different build systems, that don't accept other different values.
I thought about this, and it would be doable by defining a mapping in every build system.
franramirez688
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, but IMO, I think that I would not manage the verbosity flags nor the duplicated ones. I would keep it as easy as appending the ctest_args, that's all.
After running the command, it'll show the command with all the parameters applied, so the user will realize if there's something wrong. We can always implement it later. Wdyt?
franramirez688
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changelog: Feature:
CMake.ctest()runner newtools.cmake:ctest_argsconf.Docs: conan-io/docs#4324
Close #19183