[Rpm-maint] Plugin ponderings

[Panu Matilainen]

On 11/28/2012 02:40 PM, Reshetova, Elena wrote:
> Hi,
>
> New version is attached. I have corrected (hopefully) the indentation and
> other cosmetics.

Um, this changes the indentation of all the existing call-hook functions 
(Tsm and Psm Pre/Post) too for no good reason - they are already 
correctly indented. This kinda makes me wonder are we looking at the 
same view of the code at all... is your hard-tab width set to something 
other than the default of 8, such as 4? If so, rpm codebase will look 
absolutely horrible and that'd explain wanting to change these to 
soft-tabs (which is what the patch does), and the other strugles with 
indentation issues.

What editor are you using? Since this whole thing is obviously much more 
than just a one-off drive-by patch, we need to find a setup for you that 
"just works". Continuously fiddling with these trivialities is just 
frustrating waste of time for both of us.

Also on the subject of cosmetics: whitespace and other cosmetics changes 
should never ever be combined with actual code changes. Using an example 
from the patch:

@@ -204,10 +204,10 @@ rpmRC rpmpluginsCallTsmPre(rpmPlugins plugins, 
rpmts ts)
      const char *name = NULL;

      for (i = 0; i < plugins->count; i++) {
-       name = plugins->names[i];
-       RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_TSM_PRE);
-       if (hookFunc(ts) == RPMRC_FAIL)
-           rc = RPMRC_FAIL;
+        name = plugins->names[i];
+        RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_TSM_PRE);
+        if (hookFunc(ts) == RPMRC_FAIL)
+            rc = RPMRC_FAIL;
      }

      return rc;

It's not possible to tell with a quick glance whether the actual code 
changed here or not, and that is poisonous when combined with other 
changes in a commit. Even when done in separate commits stating 
"indentation fixes" only, such changes disrupt the actual code history, 
which is why its better to get the things right from the start. Which is 
why I'm such a nit-picker over these things.

> Also, changed the  rpmScriptRun() as you suggested (including bits for
> script execution flow indication), it indeed looks much better!

There's a slight problem with this however - it doesn't compile :)

rpmscript.c: In function 'runExtScript':
rpmscript.c:265:61: error: 'SCRIPTLET_TYPE_EXTERNAL' undeclared (first 
use in this function)
rpmscript.c:265:61: note: each undeclared identifier is reported only 
once for each function it appears in
make[3]: *** [rpmscript.lo] Error 1

Also I dont think there's any added value in a separate 
RPMSCRIPTLET_EMBEDDED (note spelling) bit, that's what RPMSCRIPTLET_EXEC 
is for: if nothing is going to be exec()'ed, the scriptlet interpreter 
by definition must be rpm itself.

> I also removed the return code assignment from POST hook, because it indeed
> overwrites the actual script return code and anyway again there is no way
> rpm can handle any failures from this hook.

Doesn't look that way:

+
+    /* Run scriptlet post hook for all plugins */
+    rc = rpmpluginsCallScriptletPost(plugins, script->descr, 
script_type, rc);
+

Wrong version of patch sent maybe?

> And about changing the fsm commit to commit everything after all files are
> installed to tmp and all plugins/checks/whatever has executed, I understand
> that this would be a BIG change. Just maybe smth for really far future..
> Just it could help improve robustness, too IMO.

It would indeed be good for overall robustness, plugins or not. Who 
knows, it might even end up sanitizing the code (much stranger things 
have happened :)

	- Panu -