Skip to content

Option to pass extra arguments to ctest#315

Open
kalebealvs wants to merge 3 commits intoCivitasv:masterfrom
kalebealvs:ctest_args
Open

Option to pass extra arguments to ctest#315
kalebealvs wants to merge 3 commits intoCivitasv:masterfrom
kalebealvs:ctest_args

Conversation

@kalebealvs
Copy link
Copy Markdown

This patch enables the plugin to pass extra args to ctest when running tests.

Resolves: #296

Comment thread README.md
require("cmake-tools").setup {
cmake_command = "cmake", -- this is used to specify cmake command path
ctest_command = "ctest", -- this is used to specify ctest command path
ctest_extra_args = {}, -- this is used to specify extra args for ctest
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Maybe we can put this variable into Config.base_settings? So we can edit it easily.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I like the idea. Done.

@Civitasv
Copy link
Copy Markdown
Owner

Sorry for my late. It's a great option, I personally think putting it into cmake_settings will be better. What do you think?

@kalebealvs
Copy link
Copy Markdown
Author

Sorry for my late. It's a great option, I personally think putting it into cmake_settings will be better. What do you think?

I'm so sorry. I completely forgot about this since I was using my fork and didn't pay much attention to the notifications.
Do you mean creating a new object called cmake_settings and putting ctest_extra_args under it?
That being the case, isn't this root object kinda like "cmake settings" already, since there are other things here also tied to cmake settings, like regenerate_on_save, generate_options, build_directory, among others?

Copy link
Copy Markdown
Collaborator

@lceWolf lceWolf left a comment

Choose a reason for hiding this comment

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

Sorry for my late reply. I think this PR is a move in the right direction, I only got some minor comments.

Unfortunately, there are merge conflicts now which need resolving.

obj.executor = const.cmake_executor
obj.runner = const.cmake_runner

for _, v in pairs(const.ctest_extra_args) do
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Using a for loop here might not have the desired outcome. Or at least only once^^
It will add the args to the Config table inject of the created obj.
This is not an issue introduced by this PR, but the way new is currently implemented might cause the args to be appended multiple times.
It might be better to assign it directly (like cmake_build_options) until the Config class is fixed

additionally, ctest_extra_args is an array -- you should use ipairs instead of pairs

Comment thread tests/ctest_spec.lua
it("takes extra args from user config", function()
local_const.ctest_extra_args = { "-j", "6" }
local config = Config:new(local_const)
ctest:run("test_name", "build_dir", "env", config, {})
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ctest is not actually an instance. it is a plain table. The colon syntax is not right and passes a wrong value to the ctest_command parameter

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.

Option to pass extra arguments to ctest

3 participants