Skip to content

Better handling of build preset targets #295

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

Open
arntanguy wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Better handling of build preset targets #295

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 36 additions & 15 deletions lua/cmake-tools/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,9 @@ function cmake.build(opt, callback)
end)
end

if opt.target == nil and config.build_target == nil then
local presets = Presets:parse(config.cwd)
local build_preset = presets:get_build_preset(config.build_preset)
if (not config.build_preset or config.build_preset and not build_preset.targets) and opt.target == nil and config.build_target == nil then
Copy link
Copy Markdown
Collaborator

@lceWolf lceWolf Feb 22, 2025

Choose a reason for hiding this comment

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

build_preset could be nil. You will run into an index into nill error on build_preset.targets in this case. Maybe check for if (not build_preset or not build_preset.targets) ... ?

return cmake.select_build_target(true, function(result)
if result:is_ok() then
cmake.build(opt, callback)
Expand All @@ -353,23 +355,26 @@ function cmake.build(opt, callback)
}
end

if opt.target ~= nil then
vim.list_extend(args, { "--target", opt.target })
vim.list_extend(args, fargs)
elseif config.build_target == "all" then
vim.list_extend(args, { "--target", "all" })
vim.list_extend(args, fargs)
else
vim.list_extend(args, { "--target", config.build_target })
vim.list_extend(args, fargs)
if not build_preset or build_preset and not build_preset.targets then
if opt.target ~= nil then
vim.list_extend(args, { "--target", opt.target })
vim.list_extend(args, fargs)
elseif config.build_target == "all" then
vim.list_extend(args, { "--target", "all" })
vim.list_extend(args, fargs)
else
vim.list_extend(args, { "--target", config.build_target })
vim.list_extend(args, fargs)
end
Comment on lines +358 to +368
Copy link
Copy Markdown
Collaborator

@lceWolf lceWolf Feb 22, 2025

Choose a reason for hiding this comment

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

If you want the build_preset.targets to take precedence, you can check for if build_preset and build_presets.targets first and have elseif opt.target second, right?

end

if config.build_type ~= nil then
vim.list_extend(args, { "--config", config.build_type })
if not build_preset then
Copy link
Copy Markdown
Collaborator

@lceWolf lceWolf Feb 22, 2025

Choose a reason for hiding this comment

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

why this additional if?

if config.build_type ~= nil then
vim.list_extend(args, { "--config", config.build_type })
end
vim.list_extend(args, config:build_options())
end

vim.list_extend(args, config:build_options())

local env = environment.get_build_environment(config)
local cmd = const.cmake_command
return utils.execute(cmd, config.env_script, env, args, config.cwd, config.executor, callback)
Expand Down Expand Up @@ -825,8 +830,24 @@ function cmake.select_build_preset(callback)
if config.build_preset ~= choice then
config.build_preset = choice
end
local build_preset = presets:get_build_preset(choice)
if build_preset.targets then
if type(build_preset.targets) == string then
config.build_target = build_preset.targets
else -- build preset is an array
if #(build_preset.targets) == 1 then
config.build_target = build_preset.targets[1]
else
-- XXX: Array targets are not supported in other code paths
-- As a workaround, unset the current build_target if any (it is not used when using build presets)
config.build_target = nil
Copy link
Copy Markdown
Collaborator

@lceWolf lceWolf Feb 22, 2025

Choose a reason for hiding this comment

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

as we do not have a handling for multiple build targets currently, you could either keep the currently selected one (if it is part of the array) or always select the first one from the list. The user can still change the target, if needed.
Resetting the selected target is not desirable, I think

I think a refactoring to support multiple targets might be helpful. But this is beyond the scope of this MR for sure.

end
end
else -- build preset does not have a target, ask
config.build_target = nil
Copy link
Copy Markdown
Collaborator

@lceWolf lceWolf Feb 22, 2025

Choose a reason for hiding this comment

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

if the build_preset does not have a target, the currently selected one should be kept.

end
local associated_configure_preset = presets:get_configure_preset(
presets:get_build_preset(choice).configurePreset,
build_preset.configurePreset,
{ include_hidden = true }
)
local associated_configure_preset_name = associated_configure_preset
Expand Down