[Rpm-maint] Plugin ponderings

[Reshetova, Elena]

Yes, I am sorry: I have made two mistakes now: 

1) patch is wrong (sent the intermediate version before the compilation and
plenty of other changes)
2) and yes, I had default indentation of 4, which was confusing me for a
long long while :( I was just using simple Ubuntu file editing, I changed it
now and it looks much better. 

I will send a new version in a sec with things corrected.

Sorry again for all of this!

Best Regards,
Elena.

-----Original Message-----
From: Panu Matilainen [mailto:pmatilai at laiskiainen.org] 
Sent: Thursday, November 29, 2012 10:23 AM
To: Reshetova, Elena
Cc: rpm-maint at lists.rpm.org
Subject: Re: Plugin ponderings

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 -
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 7220 bytes
Desc: not available
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20121129/c001f576/attachment.p7s>