Better handling of build preset targets#295
Better handling of build preset targets#295arntanguy wants to merge 1 commit intoCivitasv:masterfrom
Conversation
| 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 |
There was a problem hiding this comment.
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) ... ?
| 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 |
There was a problem hiding this comment.
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?
|
|
||
| if config.build_type ~= nil then | ||
| vim.list_extend(args, { "--config", config.build_type }) | ||
| if not build_preset then |
| end | ||
| end | ||
| else -- build preset does not have a target, ask | ||
| config.build_target = nil |
There was a problem hiding this comment.
if the build_preset does not have a target, the currently selected one should be kept.
| 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 |
There was a problem hiding this comment.
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.
|
I appreciate the PR and think it is a step in the right direction for sure. Very nice write-up of the motivation and your approach! |
2 participants
In the current implementation, the
targetsfield ofCMakePresets.jsonis ignored. This leads to inconsistencies between whatcmake-tools.nvimdoes and displays and what the user expects to happen.Here is an example
buildPresetsshowcasing the 4 possible cases:One expects that :
:CMakeSelectBuildPresetthe internalbuild_targetto be updated. This ensures that callingget_build_target()returns the target specified in the preset (for lualine for instance).:CMakeBuildthe command executed to becmake --preset <preset_name>without additional--targetargumentsThe current behaviour priour to this PR is to:
targetsfield of theCMakePresets.jsoncmake --preset <preset_name> --target <whatever the previously specified target was>For a more concrete use-case, consider embeded development with a
build-presetpreset with abuild-targettarget and and aflash-presetpreset with aflash-targettarget that flashes the program onto the device.Currently, the following would happen:
:CMakeSelectBuildPreset build-presetbuild-presetis indeed selectedbuild_target: none => we expectedbuild-targetinstead:CMakeBuildwill prompt for a target, we selectbuild-target, andcmake --preset build-preset --target build-targetis run:CMakeSelectBuildPreset flash-presetflash-presetis indeed selectedbuild_target:build-target=> we expectedflash-target:CMakeBuildrunscmake --preset flash-preset --target build-target=> we expected to run theflash-targetYou get the gist. This PR fixes this behaviour in the following way:
build_targetis changed to what's specified in that preset with one caveat: multiple targetstargets: ["target1", "target2"]. Since this case is not handled anywhere else in the code, I simply put thebuild_target = nilcmake --preset <selected_preset>if at least one target is specified in the preset, otherwise it iscmake --preset <selected_preset> --target <user selected target>It would probably be better to have a proper way of handle the multiple-targets case. However this seemed too big of a change, for little benefit for me to tackle right now.