ENH: Add NO_SOURCE_GROUPS option to ctkBuild macros - #1447
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a NO_SOURCE_GROUPS opt-out across CTK’s build macros so external consumers can keep their own source_group() layout without CTK overriding it, and ensures the flag is properly parsed (avoiding it being accidentally appended to a preceding named argument like OUTPUT_DIR).
Changes:
- Recognize
NO_SOURCE_GROUPSas a supported option in all four build macros’ argument parsing. - Guard
source_group("Resources" ...)(andsource_group("Generated" ...)in the plugin macro) behindif(NOT MY_NO_SOURCE_GROUPS).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| CMake/ctkMacroBuildQtPlugin.cmake | Adds NO_SOURCE_GROUPS option parsing and skips CTK resource grouping when requested. |
| CMake/ctkMacroBuildPlugin.cmake | Adds NO_SOURCE_GROUPS option parsing and conditionally disables both Resources/Generated group assignments. |
| CMake/ctkMacroBuildLib.cmake | Adds NO_SOURCE_GROUPS option parsing and conditionally disables Resources grouping. |
| CMake/ctkMacroBuildApp.cmake | Adds NO_SOURCE_GROUPS option parsing and conditionally disables Resources grouping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lassoan
left a comment
There was a problem hiding this comment.
It looks good, I have just one general question (and a minor comment inline): Do you really want to specify NO_SOURCE_GROUPS manually for every call of all these macros? If a CTK_NO_SOURCE_GROUPS CMake option was introduced that set the desired behavior globally, then all the callers could be left unchanged.
| PLUGIN_DIR designer | ||
| ${ARGN}) | ||
| cmake_parse_arguments(MY | ||
| "" # no options |
There was a problem hiding this comment.
If you want to be consistent then include all arguments (including NO_SOURCE_GROUPS) here; or just do the minimum (only list NAME, as nothing else is used here).
ctkMacroBuildApp, ctkMacroBuildLib, ctkMacroBuildPlugin and ctkMacroBuildQtPlugin call source_group() to categorize resources and generated files. A project that embeds these macros and organizes its sources itself cannot keep its own layout, because the calls inside the macros run after the caller's and take precedence. Guard them with a CTK_NO_SOURCE_GROUPS option, so such a project can opt out centrally instead of passing an argument at every call site.
f59c778 to
498beb9
Compare
|
You're right. We are using the macro(s) only in our own wrapper macro(s) so we do not need to specify the arguments over and over again, but the global solution is more elegant. I amended the commit accordingly. |
Disclaimer: We (MITK) are currently catching up on the latest CTK master branch
since we forked for Qt 6 back in 2023. We found a few bugs that are not caught by
your CI and mostly affect external users of CTK.
Adds an opt-out so a consumer can apply its own
source_group()layout.Without it, CTK's
source_group("Resources" ...)andsource_group("Generated" ...)override the grouping the consumer set upbefore calling the macro.
Note this is currently a hard break rather than a cosmetic one for consumers
that already pass the flag:
CtkMacroParseArgumentsappends an unrecognisedtoken to the preceding named argument's list, so passing
NO_SOURCE_GROUPSafter
OUTPUT_DIR <dir>silently yieldsMY_OUTPUT_DIR=<dir>;NO_SOURCE_GROUPSand the plug-in lands in a bogus directory.
Applied to all four macros (
ctkMacroBuildApp,ctkMacroBuildLib,ctkMacroBuildPlugin,ctkMacroBuildQtPlugin) for consistency.