Ceedling icon indicating copy to clipboard operation
Ceedling copied to clipboard

Fix tools arrays.

Open deltalejo opened this issue 2 years ago • 2 comments

This will close #459 and #774.

deltalejo avatar May 25 '23 05:05 deltalejo

I think this issue need to be merged at ceedling_0_32 branch

claudiiolima avatar Dec 13 '23 17:12 claudiiolima

@mvandervoord Any idea on fixing tools arrays for command hooks? Maybe they can be supported only for command hooks tools and not for the normal ones (test compiler, test linker, etc.).

deltalejo avatar Dec 14 '23 00:12 deltalejo

@mvandervoord @mkarlesky I want to resume my work on this. My initial plan was to fix tools array handling on setup and validation, but that will leave not uniform behavior between normal tools (preprocessor, compiler, linker, etc.) and command hooks plugin tools (pre_compile_execute, pre_link_execute, etc.). The former ones will not allow an Array of tools (I can not think of a use case for having an array of compilers, preprocessors, etc. Or may there be any?) and the last ones will accept both a tool Hash or an Array of tool Hashes, that is why I proposed here to take out command hooks plugin tools from tools section and move them to a dedicated command hooks section. Are you OK with the above behavior? Let me know your thoughts.

deltalejo avatar Jun 09 '24 20:06 deltalejo

@deltalejo @mvandervoord It seems pretty clear that folks would love an array of tools for command hooks. I am in support. The current structure of the :tools configuration section does not support this and probably should not support this.

Once upon a time, it only made sense to place tool definitions in :tools because of Ceedling's simplistic validation scheme. That's no longer a limit, though. Tool validation has been broken out into its own utility object, ToolValidator, that many of the rewritten plugins now depend on. Specifically, if a particular feature in the plugin's configuration is enabled, it validates the corresponding tool definition (only internally defined to the plugin to date). Tools should always be validated at startup to avoid frustrating errors on the backside of a build, but tools should only be validated for features that depend on them. The plugin's configuration handling/validation and any tool validation happens in the plugin’s setup(). The Beep plugin is the simplest that demonstrates this.

Further, as Ceedling has been evolving for the next release a pattern has emerged for plugins: (1) enable the plugin in :plugins and (2) complement this with a dedicated top-level configuration section corresponding to that plugin (again, the Beep plugin demonstrates this simply).

So, I think Command Hooks should get these same treatments. This will bring it into alignment with other plugins and allow for multiple tools per hook.

  1. If Command Hooks is enabled, all its configuration should be in a top-level configuration block :command_hooks.
  2. The second tier of configuration beneath :command_hooks should be the command hook keys, e.g. :command_hooks:pre_compile_execute.
  3. Instead of organizing command hook tools beneath :tools they should be in an array beneath any hook key in the plugin config block.
  4. At plugin startup, the plugin should validate that all hook keys are from among the recognized list, raising an exception for any that are not (these would be mistakes with silent failures otherwise). The plugin should also use ToolValidator to validate each tool associated with a hook. All this would happen from within setup(). If command hooks are enabled and tools are specified for any hooks, we know a build will rely on them and, therefore, the plugin should forcibly validate them from within setup().
  5. The rest of the plugin's behavior would remain unchanged apart from necessary refactoring to connect existing code with the new configuration above.
:plugins:
  :enabled:
    - command_hooks

:command_hooks:
  :pre_compile_execute:
    - <tool definition>
    - <tool definition>

mkarlesky avatar Jun 13 '24 13:06 mkarlesky

@mkarlesky @mvandervoord This is ready for review now.

deltalejo avatar Jun 18 '24 14:06 deltalejo

Thank you for this, @deltalejo! This is good work. I'll get it merged possibly as soon as today.

mkarlesky avatar Jun 24 '24 21:06 mkarlesky