[pacman-dev] Potential bug in alpm-hooks; fix provided
Hi, I'm providing a fix [1] for the following issue. I've noticed that exactly one `Exec` directive is allowed in alpm-hooks(5). This is not explicitly stated in the manual (the word "required" does not usually imply "non-repeatable", as seems to be intended by the manual), nor does it rise an error. Instead, multiple occurrences are silently ignored. Use case: # When installed as a pacman hook [1], this will call the # `my-update-pacman-mirrorlist` script every time the file # `/etc/pacman.d/mirrorlist` should be updated. [Trigger] Operation = Upgrade Operation = Install Type = File Target = etc/pacman.d/mirrorlist [Action] When = PostTransaction Exec = /usr/bin/my-update-pacman-mirrorlist Exec = /usr/bin/rm -f /etc/pacman.d/mirrorlist.pacnew Only the last `Exec` is used. I think it is semantically correct since the syntax looks like an assignment that overwrites the previous one. But it is inconsistent with the use of `Operation` in the `Trigger` section. I would expect the commands to be evaluated in order, or to see an error due to reusing `Exec`. I think raising an error would be best, otherwise pacman would have to provide means to decide how to handle individually failing commands, a situation that is more flexibly handled by an external script, which should be called instead of using multiple `Exec`s. I have implemented this in [1] and added a test. If you like that, I would update the documentation, and also look for other values that are silently overwritten when parsing a hook. Also, I'd like to make `Operation` accept things like Operation = Upgrade Install and in future versions deprecate the current use of multiple assignments which I consider rather unfortunate. Cheers, Stefan ____________________ [1] https://github.com/s5k6/pacman/tree/fix-hook-exec -- http://stefan-klinger.de o/X Send plain text messages only, not exceeding 32kB. /\/ \
On 01/02/17 at 05:10pm, Stefan Klinger wrote:
Hi,
I'm providing a fix [1] for the following issue.
I've noticed that exactly one `Exec` directive is allowed in alpm-hooks(5). This is not explicitly stated in the manual (the word "required" does not usually imply "non-repeatable", as seems to be intended by the manual), nor does it rise an error. Instead, multiple occurrences are silently ignored.
Use case:
# When installed as a pacman hook [1], this will call the # `my-update-pacman-mirrorlist` script every time the file # `/etc/pacman.d/mirrorlist` should be updated.
[Trigger] Operation = Upgrade Operation = Install Type = File Target = etc/pacman.d/mirrorlist
[Action] When = PostTransaction Exec = /usr/bin/my-update-pacman-mirrorlist Exec = /usr/bin/rm -f /etc/pacman.d/mirrorlist.pacnew
Only the last `Exec` is used. I think it is semantically correct since the syntax looks like an assignment that overwrites the previous one. But it is inconsistent with the use of `Operation` in the `Trigger` section. I would expect the commands to be evaluated in order, or to see an error due to reusing `Exec`.
All options accept a single value unless the documentation says otherwise. For single-value options, using the last value is fairly standard. I'm not sure why you would expect hooks to behave any differently.
I think raising an error would be best, otherwise pacman would have to provide means to decide how to handle individually failing commands, a situation that is more flexibly handled by an external script, which should be called instead of using multiple `Exec`s.
I'm neutral on treating overwriting single-value options as an error. I'm generally in favor of being strict when parsing hooks, but I'm still considering allowing Include's in hooks, which would complicate this.
I have implemented this in [1] and added a test. If you like that, I would update the documentation, and also look for other values that are silently overwritten when parsing a hook. Also, I'd like to make `Operation` accept things like
Operation = Upgrade Install
and in future versions deprecate the current use of multiple assignments which I consider rather unfortunate.
Multiple "assignments" are not going away. Triggers can have multiple Targets and forcing people to cram them all on one line is a terrible idea. This is also how multi-value options in pacman's configuration are parsed.
Cheers, Stefan
____________________ [1] https://github.com/s5k6/pacman/tree/fix-hook-exec
On 2017-Jan-02, Andrew Gregory wrote with possible deletions:
On 01/02/17 at 05:10pm, Stefan Klinger wrote:
I've noticed that exactly one `Exec` directive is allowed in alpm-hooks(5). Instead, multiple occurrences are silently ignored. I think raising an error would be best
Multiple "assignments" are not going away. Triggers can have multiple Targets and forcing people to cram them all on one line is a terrible idea. This is also how multi-value options in pacman's configuration are parsed.
Ok, if that actually is usual behavior in ini-style files, so be it. Could we print a warning if information is lost by reassigning Exec? I have updated my proposal [1] accordingly.
____________________ [1] https://github.com/s5k6/pacman/tree/fix-hook-exec
-- Stefan Klinger, Ph.D. -- computer scientist o/X http://stefan-klinger.de /\/ https://github.com/s5k6 \ Please send plain text messages only, not exceeding 32kB.
On 01/02/17 at 07:20pm, git@stefan-klinger.de wrote:
On 2017-Jan-02, Andrew Gregory wrote with possible deletions:
On 01/02/17 at 05:10pm, Stefan Klinger wrote:
I've noticed that exactly one `Exec` directive is allowed in alpm-hooks(5). Instead, multiple occurrences are silently ignored. I think raising an error would be best
Multiple "assignments" are not going away. Triggers can have multiple Targets and forcing people to cram them all on one line is a terrible idea. This is also how multi-value options in pacman's configuration are parsed.
Ok, if that actually is usual behavior in ini-style files, so be it. Could we print a warning if information is lost by reassigning Exec? I have updated my proposal [1] accordingly.
____________________ [1] https://github.com/s5k6/pacman/tree/fix-hook-exec
As I said before, I have no real opinion one way or the other about issuing a warning/error on overwriting single-value options, so I'll leave that to Allan. If you want your patch to actually be reviewed, though, you need to submit it to this list with `git send-email`. apg
As far as I can tell this is relevant to the other discussion in [0]. I recommend you have a look and check the code duplication, so you don't fix things that are being removed anyway. cheers! mar77i [0] https://lists.archlinux.org/pipermail/pacman-dev/2017-January/021770.html
On 03/01/17 07:44, Martin Kühne wrote:
As far as I can tell this is relevant to the other discussion in [0]. I recommend you have a look and check the code duplication, so you don't fix things that are being removed anyway.
cheers! mar77i
[0] https://lists.archlinux.org/pipermail/pacman-dev/2017-January/021770.html .
First in wins... no patch has been submitted for that.
On 01/03/17 at 01:56pm, Allan McRae wrote:
On 03/01/17 07:44, Martin Kühne wrote:
As far as I can tell this is relevant to the other discussion in [0]. I recommend you have a look and check the code duplication, so you don't fix things that are being removed anyway.
cheers! mar77i
[0] https://lists.archlinux.org/pipermail/pacman-dev/2017-January/021770.html .
First in wins... no patch has been submitted for that.
I see no reason why these would overlap.
On 03/01/17 04:27, Andrew Gregory wrote:
On 01/02/17 at 07:20pm, git@stefan-klinger.de wrote:
On 2017-Jan-02, Andrew Gregory wrote with possible deletions:
On 01/02/17 at 05:10pm, Stefan Klinger wrote:
I've noticed that exactly one `Exec` directive is allowed in alpm-hooks(5). Instead, multiple occurrences are silently ignored. I think raising an error would be best
Multiple "assignments" are not going away. Triggers can have multiple Targets and forcing people to cram them all on one line is a terrible idea. This is also how multi-value options in pacman's configuration are parsed.
Ok, if that actually is usual behavior in ini-style files, so be it. Could we print a warning if information is lost by reassigning Exec? I have updated my proposal [1] accordingly.
____________________ [1] https://github.com/s5k6/pacman/tree/fix-hook-exec
As I said before, I have no real opinion one way or the other about issuing a warning/error on overwriting single-value options, so I'll leave that to Allan. If you want your patch to actually be reviewed, though, you need to submit it to this list with `git send-email`.
I think an error message is too strict and a debug message is not enough. So I will accept a warning message being printed. A
participants (5)
-
Allan McRae
-
Andrew Gregory
-
git＠stefan-klinger.de
-
Martin Kühne
-
Stefan Klinger