Skip to content

Fix: allow build with no selected build preset#334

Closed
bailwillharr wants to merge 1 commit intoCivitasv:masterfrom
bailwillharr:fix-build-presets
Closed

Fix: allow build with no selected build preset#334
bailwillharr wants to merge 1 commit intoCivitasv:masterfrom
bailwillharr:fix-build-presets

Conversation

@bailwillharr
Copy link
Copy Markdown

This pull requests fixes the behaviour I mentioned in Issue #333 (CMake build presets are required when using configure presets). It allows the user to proceed with building if a CMake configure preset has been selected but no build presets are available.

This change was implemented by creating a sentinel object for the BuildPreset class which represents the choice of no build preset. This choice cannot be specified with build_preset = nil as that represents the case where a build preset is yet to be selected.

Code can check for this 'no build preset wanted' state by calling build_preset.is_none which returns true in that case.

A few if statements in lua/cmake-tools/init.lua are modified to ensure that build_preset is not used when set to BuildPreset.None.

In addition, a minor typo causing undersired behaviour was fixed on line 33 (previously line 21) of lua/cmake-tools/build_preset.lua.

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.

Nice finding and I very much appreciate this MR!
While your changes do work, I think the handling of the "None" build-preset itself could be improved.
I don't like the special handling for a "None" choice and checking for that hardcoded string.

Hope you don't feel bad about this, but I've got another idea for a solution. I'd love hearing your feedback on #335

end

function Presets.createEmptyBuildPreset()
return BuildPreset.None
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.

That does not actually return a new instance. It returns the same table for each invocation. Modifications of that table would be shared across every "instance" you created that way. To properly create a new instance you should use
return setmetatable({}, <NonePreset-Table>)

local BuildPreset = {}

-- 'None' instance for when no build preset should be used.
BuildPreset.None = {
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.

I think this is not quite the effect you are trying to achieve. This is not an instance. It is something like a subclass within the BuildPreset class which re-uses the methods and fields of the BuildPreset class, yet some methods are overwritten/hidden (get_build_target and get_build_type)

See my comment on the createEmptyBuildPreset method on how to create individual instances of a None Preset

@lceWolf
Copy link
Copy Markdown
Collaborator

lceWolf commented Aug 26, 2025

Closed in favor of #335

@lceWolf lceWolf closed this Aug 26, 2025
@bailwillharr
Copy link
Copy Markdown
Author

Thanks a lot for this fix man. Tbh my experience with Lua is pretty limited so I'm glad someone who knows what they're doing stepped up :).

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.

2 participants