BUG: Do not discard the RESOURCES of a plug-in

[kislinsk]

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.

Priority: highest. Regression that makes ctkMacroBuildPlugin silently
ignore a documented parameter.

ctkMacroBuildPlugin accepts RESOURCES, but clears the variable before use:

# Make sure variable are cleared
set(MY_RESOURCES)

Only the generated manifest and cached-resource .qrc files are appended
afterwards, so a plug-in's own .qrc files never become target sources and
AUTORCC never compiles them. Every icon, stylesheet and other embedded file of
a plug-in is missing at runtime.

The clearing made sense while the macro compiled RESOURCES into a separate
list of generated sources (MY_QRC_SRCS); with AUTORCC the .qrc files
themselves are the target sources, so the reset just drops them.

Why CI does not catch it: the in-tree plug-ins pass no RESOURCES
(set(PLUGIN_resources ) is empty in both org.commontk.eventadmin and
org.commontk.configadmin), so nothing exercises the parameter.

How it presents: builds and links without a single warning. The plug-in
loads, and only at runtime does every resource lookup fail:

WARNING: Could not read :/org.blueberry.ui.qt/darkstyle.qss
WARNING: Could not read :/org_mitk_icons/icons/awesome/scalable/actions/document-open.svg

Verifiable in the build tree: no qrc_<name>.cpp is generated for the
plug-in's own .qrc, only the _cached and _manifest ones.

[lassoan]

It is an interesting experience to review the result of someone else's AI chatbot session. Using AI is of course the right thing to do, but it generates very verbose and somewhat inaccurate results. Since I don't want to waste my time, I need to start my own AI session to pose questions and challenge potential inaccuracies and improve the solution. But then @kislinsk you don't have access to that session and you'll again need to start another discussion to review the results and ask your own questions. All these iterations take much longer than the direct human code writing and review...

Let's see if we can have a shared AI-assisted discussion here to avoid the private AI sessions. I'll try to summon the copilot agent with some questions to see if it leads a more efficient workflow.

[lassoan]

It seems that we cannot ask copilot in pull requests like this:

So, let's just discuss things here and manually copy here relevant external AI-produced outputs.

But before going into the details, I would share my unpleasant experience of receiving a bunch of AI-heavy PRs. I initially tried to manually review this PR but I got immediately stuck at an error in the main statement Regression that makes ctkMacroBuildPlugin silently ignore a documented parameter. Neither RESOURCES or MY_RESOURCES was documented anywhere. I did a quick manual search in the full CTK source, nothing came up. I did not want to waste more time with searching, so I had to fire up my own AI session, asked Claude to review this PR. It suggested to accept it as is. But then I asked questions about what documented variable we are talking about, why the change was necessary, what changed that it is not necessary anymore, and why it is actually causing problems now, and after a while it produced output that I'm satisfied with. I copy the results here:

Why the clearing originally existed. fd4845f2 (2010, "Make sure variable are cleared") added it for the output variables of the Qt wrap macros — MY_UI_CXX, MY_QRC_SRCS, later MY_MOC_CPP. Those names are not in the CtkMacroParseArguments keyword list, so nothing initialized them, and the Qt wrap macros append to their output variable rather than assigning it. Because ctkMacroBuildPlugin is an unscoped macro(), a second invocation in the same directory scope would append onto the previous plug-in's generated-file list ("weird behavior", per the commit message). For those variables the manual clear was genuinely necessary — the parser only initializes its own keywords.

Why it is not necessary anymore. The Qt6-prep migration (37f5f51a AUTOUIC, 17587367 AUTORCC, b33c2338 AUTOMOC) removed the explicit wrap calls and with them every such output variable — none of MY_UI_CXX/MY_QRC_SRCS/MY_MOC_CPP appears in the macro today. Every remaining MY_* variable is a parsed keyword, and CtkMacroParseArguments unconditionally resets each declared keyword at the start of every call, so nothing can leak between invocations. There is no variable left that the clear protects.

Why it caused trouble now. 17587367 mechanically renamed the clear from set(MY_QRC_SRCS) (generated-output list — correct to clear) to set(MY_RESOURCES) (the parsed input — wrong to clear), so the caller's .qrc files were discarded before ever reaching add_library(), and AUTORCC had nothing to compile. Notably, the project already learned this lesson once: fd4845f2 also cleared MY_SRCS, and e588f214 deleted that clear 48 minutes later because parsed inputs must not be cleared. This PR is the same fix for the same class of bug.

Since there is no placement where the clear adds value — before parsing it duplicates the parser, after parsing it discards the input — deletion is correct.

Two follow-up notes (neither blocking):

CI misses this because the only in-tree plug-in passing a real .qrc via RESOURCES (org.commontk.dah.core) is OFF by default, and the failure mode is silent until runtime. A small runtime test asserting a known resource path is readable would be the only effective regression guard.
The macro's doxygen block still doesn't enumerate its keywords ("takes the usual arguments"); documenting them would help prevent this class of ambiguity.

Recommended commit comment:

BUG: Do not discard the RESOURCES argument of ctkMacroBuildPlugin

The macro cleared MY_RESOURCES immediately after argument parsing:

  # Make sure variable are cleared
  set(MY_RESOURCES)

Since MY_RESOURCES holds the caller's RESOURCES argument, the clear
discarded every .qrc file a plug-in passed in. Only the generated
manifest and cached-resource .qrc files were appended afterwards, so
the plug-in's own .qrc files never became target sources and AUTORCC
never compiled them. The failure is silent: configure, build, link
and plug-in loading all succeed, and only runtime resource lookups
fail (e.g. "Could not read :/org.blueberry.ui.qt/darkstyle.qss").

Why the clearing existed: fd4845f2 (2010) added it for the OUTPUT
variables of the Qt wrap macros (MY_UI_CXX, MY_QRC_SRCS, MY_MOC_CPP).
Those names are not in the CtkMacroParseArguments keyword list, so
nothing initialized them, and the Qt wrap macros append to their
output variable instead of assigning it. Because ctkMacroBuildPlugin
is an unscoped macro(), a second invocation in the same directory
scope would append onto the previous plug-in's generated-file list.
For those variables the manual clear was the only defense.

Why it is no longer needed: the Qt6-prep commits (37f5f51a AUTOUIC,
17587367 AUTORCC, b33c2338 AUTOMOC) removed the explicit wrap calls,
and with them every such output variable. The only variables left in
the macro are parsed keywords, which CtkMacroParseArguments already
resets unconditionally at the start of every call, so no stale value
can leak between invocations.

Why it broke now: 17587367 renamed the clear from set(MY_QRC_SRCS)
(a generated-output list, correct to clear) to set(MY_RESOURCES)
(the parsed input, wrong to clear) while migrating to AUTORCC. The
same mistake was made and fixed once before: fd4845f2 also cleared
MY_SRCS, and e588f214 removed that clear the same day because parsed
inputs must not be cleared. There is no correct placement for the
clear - before parsing it duplicates what the parser does, after
parsing it discards the input - so it is deleted.

In-tree plug-ins do not catch this because the only one passing a
real .qrc via RESOURCES (org.commontk.dah.core) is OFF by default.

[lassoan]

The change is correct but consider adding more accurate details in the commit comment (see my previous comment).

[kislinsk]

Thank you for looking into the PRs so quickly. I know it's not the best etiquette to drop a whole bunch of AI-generated PRs on you. Sometimes I forget that we probably have very different perspectives on CTK. Things that seem obvious to us can be difficult for you to understand, and vice versa.

I've amended the commit to use your commit message. It is indeed much easier to understand.