RFE: file trigger scriptlet arguments#2755

pmatilai · 2023-11-08T11:51:02Z

Splitting this from #2655 as this is a clearly separate thing that should be doable without massive redesign of the whole thing:

First argument should reflect the triggered package count
Non-trans scriptlets have should get a second argument reflecting the triggering package count

dmnks · 2024-01-05T09:42:40Z

* Non-trans scriptlets have should get a second argument reflecting the triggering package count

I wonder if we want the triggering package count (arg2) to be an aggregate count of all the triggering packages or just that of the first match?

There's a subtle difference in the semantics between regular triggers and file triggers in the way they are fired. A regular trigger can only ever be triggered by one package (e.g. %triggerin -- foo, bar, baz means "fire if any of these packages is installed"), whereas a file trigger can be triggered by multiple packages (e.g. %filetriggerin -- /etc/foo.d means "fire if any file with such a prefix is installed, regardless of which package it belongs to").

In both cases, the trigger only executes once, however the semantics of "which package triggered it" differs. Therefore, for regular triggers, arg2 is straightforward (it's always the count of the triggering package), however for file triggers, it's not.

As an example, consider the following scenario:

# bar.spec:
%filetriggerin -- /etc/foo.d

# foo.spec:
%files
/etc/foo.d/foo.conf

# baz.spec:
%files
/etc/foo.d/baz.conf

If bar is installed first, installing foo or baz afterwards results in the trigger being run once for each of those, so the triggering count (arg2) is straighforward (same as with regular triggers).

However, if foo and baz are installed first, installing bar afterwards results in the trigger being run once for both of those (with both filenames served via stdin), so the trigerring count (arg2) is now ambiguous - it can either be the count of foo or baz, or it can be the sum of both.

My take: The count should be an aggregate since that enables a file trigger script to detect when all the files subject to the prefix are being removed (which is one of the purposes of the arguments, also for regular triggers, in the first place).

Any thoughts?

pmatilai · 2024-01-05T12:43:43Z

Aggregate is what I thought of when writing the description, I don't see anything else making much sense. But, it's not like I've given this any deep meditation. Usefulness is all that matters for the arguments, otherwise we could just as well not bother 😅

dmnks · 2024-01-05T12:56:32Z

Thanks, the part about usefulness is actually on point 😄 That said, I haven't given it too much thought either, this is just the most obvious solution that comes to mind. I'll ponder about it a bit more but there's probably not much to ponder about anyway.

pmatilai · 2024-01-05T13:27:18Z

The thing to ponder about is whether there are other arguments that should be passed in addition or in stead of these. The only "vision" or "design" behind this ticket description is "something close enough to other scriptlets that someone familiar with should feel at home". Which may leave some room for improvement from a more technical perspective 😆

dmnks · 2024-01-05T13:33:29Z

Good point! We shouldn't just blindly copy what the normal triggers do here. There's enough difference that it makes sense to step back for a moment and think about what functionality and use cases we really want to cover. Thanks, I'll take that into account 😄

dmnks · 2024-01-16T10:42:06Z

Note that there's no technical reason for not adding the second argument (the number of triggering packages) to transaction scriplets here, too. It's the same code path underneath.

Whether it's useful or not is another question but I'll probably lean towards consistency here and include it.

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same stages as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %preuntrans and %postuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those. To bring the file trigger tests on par with the regular trigger ones, extend the "basic file triggers 2" test with the remaining scenarios. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Related: rpm-software-management#2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same stages as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %preuntrans and %postuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Related: rpm-software-management#2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same stages as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %preuntrans and %postuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those, and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Related: rpm-software-management#2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same stages as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %preuntrans and %postuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those, and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Also add a short note about the argument to the file trigger docs. Related: rpm-software-management#2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same time as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %preuntrans and %postuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those, and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Also add a short note about the argument to the file trigger docs. Related: rpm-software-management#2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same time as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %postuntrans and %preuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those, and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Also add a short note about the argument to the file trigger docs. Related: rpm-software-management#2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same time as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %posttrans and %preuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those, and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Also add a short note about the argument to the file trigger docs. Related: rpm-software-management#2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same time as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %posttrans and %preuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets such as %pre. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those, and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Also add a short note about the argument to the file trigger docs. Related: rpm-software-management#2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same time as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %posttrans and %preuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets such as %pre. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those, and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Also add a short note about the argument to the file trigger docs. Argument no. 2 (triggering package count) will be added in a separate commit. Related: rpm-software-management#2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same time as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %posttrans and %preuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets such as %pre. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those, and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Also add a short note about the argument to the file trigger docs. The second argument $2 (triggering package count) will be added in a separate commit. Related: rpm-software-management#2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same time as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %posttrans and %preuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets such as %pre. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue #2868). Some tests already cover the file trigger arguments so adjust those, and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Also add a short note about the argument to the file trigger docs. The second argument $2 (triggering package count) will be added in a separate commit. Related: #2755

Pass the number of instances of the source (i.e. triggered) package left after the operation as the first argument ($1) to file trigger scripts, similarly to regular triggers. The %filetrigger{in,un,postun} variants execute at the same time as their regular trigger counterparts so use the same logic to compute the argument. The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %posttrans and %preuntrans scriptlets, respectively, so borrow the logic from those. Don't count packages in runImmedFileTriggers() again, though, and just reuse the psm->scriptArg value which is already computed in rpmpsmNew() and used for the normal scriptlets such as %pre. We could/should probably do this in runImmedTriggers(), too, but leave that for later (see issue rpm-software-management#2868). Some tests already cover the file trigger arguments so adjust those, and extend the "basic file triggers 2" test with the remaining scenarios to bring it on par with the regular trigger tests. This is easier than extending "basic file trigger scripts" since we don't really need to test the filenames returned on stdin here, just the arguments. Also add a short note about the argument to the file trigger docs. The second argument $2 (triggering package count) will be added in a separate commit. Related: rpm-software-management#2755

dmnks · 2024-01-24T14:41:36Z

Note that there's no technical reason for not adding the second argument (the number of triggering packages) to transaction scriplets here, too. It's the same code path underneath.

OK, looking closer, the challenge with transactional file triggers and package counts is that we can't easily obtain the correct (i.e. count-corrected for upgrades/removals) value for each involved package since we iterate through all files in the transaction set, as opposed to each transaction element separately (as we do with non-transactional file triggers). This is done for performance reasons (I suppose) so changing it just to add a second argument to transactional file triggers doesn't seem worth it.

After all, the original requirements of this ticket explicitly mention non-trans file triggers. I guess @pmatilai knew what he was talking about when filing this ticket, after all 😄

Whether it's useful or not is another question but I'll probably lean towards consistency here and include it.

So nope, taking it back. Let's stick with the plan and only do this for the non-trans variants.

Pass the number of instances of the target (i.e. triggering) package left after the operation as the second argument ($2) to file trigger scripts, similarly to regular triggers. CONTINUE HERE The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %posttrans and %preuntrans scriptlets, respectively, so borrow the logic from those. Also add a short note about the argument to the file trigger docs. Related: rpm-software-management#2755

Pass the number of instances of the target (i.e. triggering) package left after the operation as the second argument ($2) to file trigger scripts, similarly to regular triggers. CONTINUE HERE (explain why is arg2 disabled in transfiletrigger*) The %transfiletrigger{in,un} variants don't have any regular trigger counterparts but are close relatives of the %posttrans and %preuntrans scriptlets, respectively, so borrow the logic from those. Also add a short note about the argument to the file trigger docs. Related: rpm-software-management#2755

Pass the number of instances of the target (i.e. triggering) package left after the operation as the second argument ($2) to file trigger scripts, similarly to regular triggers. Don't pass it to the %transfiletrigger* variants, though, since we can't easily obtain a count-corrected value when such a file trigger is fired by a transaction element. This is because when runFileTriggers() is called in rpmtsRun(), no psm instance has been created yet and thus psm->scriptArg or psm->countCorrection, set by rpmpsmNew(), isn't readily available to us in runHandleTriggersInPkg(). While possible to work around, having arg2 in transaction file triggers doesn't seem worth the trouble. Make arg2 correspond to the first matching package found, as opposed to being an aggregate count of all the matching packages, even if the latter would be easier as we wouldn't have to store the matched package name in the mfi struct and just use rpmdbGetIteratorCount(mfi->pi). The reason for this is two-fold: it's consistent with how regular triggers work and it allows file triggers to detect an upgrade (arg2 > 1) of a triggering package. Fixes: rpm-software-management#2755

dmnks · 2024-02-02T15:45:55Z

As for arg2 and the question of "should it be an aggregate count?", I've come to realize that it actually should not be an aggregate since that would prevent the script from detecting a package upgrade (i.e. the value of 2 or higher could either represent an upgrade or multiple preinstalled triggering packages in the rpmdb). It would also be inconsistent with regular triggers. Hence, no aggregate count in #2883.

Of course, that falls apart as soon as you have multiple instances of the same package installed in parallel, but that's not a problem specific to file triggers but rather to all scripts.

Pass the number of instances of the target (i.e. triggering) package left after the operation as the second argument ($2) to file trigger scripts, similarly to regular triggers. Don't pass it to the %transfiletrigger* variants, though, since we can't easily obtain a count-corrected value when such a file trigger is fired by a transaction element. This is because when runFileTriggers() is called in rpmtsRun(), no psm instance has been created yet and thus psm->scriptArg, set by rpmpsmNew(), isn't readily available to us in runHandleTriggersInPkg(). While possible to work around, having arg2 in transaction file triggers doesn't seem worth the trouble. Make arg2 correspond to the first matching package found, as opposed to being an aggregate count of all the matching packages, even if the latter would be easier as we wouldn't have to store the matched package name in the mfi struct and just use rpmdbGetIteratorCount(mfi->pi). The reason for this is two-fold: it's consistent with how regular triggers work and it allows file triggers to detect an upgrade (arg2 > 1) of a triggering package. Fixes: rpm-software-management#2755

pmatilai added RFE transaction labels Nov 8, 2023

pmatilai mentioned this issue Nov 8, 2023

RFE: file triggers v2 #2655

Closed

4 tasks

dmnks self-assigned this Nov 8, 2023

This was referenced Jan 19, 2024

Unify regular and file trigger documentation #2860

Open

Update Runtime scriptlets section in manual #2867

Closed

dmnks mentioned this issue Jan 22, 2024

Pass arg1 to file trigger scripts #2871

Merged

dmnks mentioned this issue Feb 2, 2024

Pass arg2 to file trigger scripts #2883

Merged

pmatilai closed this as completed in 7f59c7d Feb 9, 2024

dmnks added this to the 4.20.0 milestone Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFE: file trigger scriptlet arguments#2755

RFE: file trigger scriptlet arguments #2755

pmatilai commented Nov 8, 2023

dmnks commented Jan 5, 2024 •
edited

pmatilai commented Jan 5, 2024

dmnks commented Jan 5, 2024

pmatilai commented Jan 5, 2024

dmnks commented Jan 5, 2024

dmnks commented Jan 16, 2024

dmnks commented Jan 24, 2024 •
edited

dmnks commented Feb 2, 2024 •
edited

RFE: file trigger scriptlet arguments#2755

RFE: file trigger scriptlet arguments #2755

Comments

pmatilai commented Nov 8, 2023

dmnks commented Jan 5, 2024 • edited

pmatilai commented Jan 5, 2024

dmnks commented Jan 5, 2024

pmatilai commented Jan 5, 2024

dmnks commented Jan 5, 2024

dmnks commented Jan 16, 2024

dmnks commented Jan 24, 2024 • edited

dmnks commented Feb 2, 2024 • edited

dmnks commented Jan 5, 2024 •
edited

dmnks commented Jan 24, 2024 •
edited

dmnks commented Feb 2, 2024 •
edited