The default commit title when installing new packages is:
committing changes in /etc made by "/usr/bin/bash"
Instead of referencing the proper package manager process (pacman in my case) This change fixes it:
--- a/etckeeper/post-install.d/50vcs-commit
+++ b/etckeeper/post-install.d/50vcs-commit
@@ -4,9 +4,7 @@ set -e
pl="/var/cache/etckeeper/packagelist"
# Parent process is etckeeper
-# (Only procps ps is currently supported, others will fail,
-# so this may end up empty.)
-ETCKEEPER_PID=$( ps --no-headers -o ppid "${PPID}" 2>/dev/null | sed 's/^ *//' )
+ETCKEEPER_PID=${PPID}
# Find the parent of etckeeper and get the command line of the process
if ! [ -z "${ETCKEEPER_PID}" ]; then
So that the commit title is now proper, eg.:
committing changes in /etc made by "pacman -S minidlna"
Will you include this fix? Or is it expected that I open a pull request? Not sure how it's done on this site as I'm not familiar with it...
I took at look at what's happening here. I see this kind of process tree when running etckeeper post-install by hand:
And
ETCKEEPER_PIDis set to 397909 andETCKEEPER_PPIDto 397908. Which is indeed kind of wrong in this case. Your patch fixes this.However, when using
apt-get installon Debian, the current code actually makes it do the right thing, and report eg "committing changes in /etc made by "apt-get install foo". In that case, the process tree is like this:With your patch it will pick 400895 as
ETCKEEPER_PPID. Which technically, it is. But that is not the command that the user ran, it's a hook script that apt is configured to run, in order to run etckeeper post-install.So in order to apply your change, this change would also be needed:
But the same kind of change would also need to be made to other package managers' hook scripts.
This is making me start to wonder if all this complexity is needed at all. There is an alternative message it can used, "committing changes in /etc after $HIGHLEVEL_PACKAGE_MANAGER run" which avoids this complexity and should be fine. This business of walking the process tree is a fairly recent addition and is buggy, so why do it.
On the third hand, it is pretty nice, when it works, to see the actual package manager command line in the commit message.
I guess what we could do is vary how
ETCKEEPER_PPIDis determined based onHIGHLEVEL_PACKAGE_MANAGER. And only use it for the message when it's a package manager we know how to get the pid of. Which right now, for me, is only apt..Were you using pacman, or pacman-g2? I guess pacman since it looks like it probably exec's etckeeper without any
sh -cprocess.joey, thanks for the feedback, I understand now why it was implemented that way. I was using pacman not pacman-g2. For reference here is how a process tree looks like with pacman:
Perhaps this could be done in a more generic way by traversing the parents until HIGHLEVEL_PACKAGE_MANAGER is found, I'll try to play with that.
Here is what I came up with:
Seems to work for me.
What it does is get the process tree of current process, reverse order, grep for HIGHLEVEL_PACKAGE_MANAGER to get the pid of it. If that fails it continues with the old method. Then it gets the command line of the found process. Not sure if dependency on pstree is something that is a problem or not...
This situation is arguably made even worse by the transaction-hooks implemented for DNF 5, which currently cause commit messages like this:
That nonsense command line is the one found in
/etc/dnf/libdnf5-plugins/actions.d/etckeeper.actionswhere it's configured as apost_transactionhook. (Ignore thesedcommand it's piped into in order to convert the script's output into DNF 5 log messages. Awful design decision in libdnf5, at least IMHO. Why it wouldn't just log whatever the script dumps to stdout is beyond my comprehension.) Presumably you'd need to look at its parent, to discover thatdnfis the package management tool in use.But anyway, all of this is to say that I agree: the process discovery used in 50vcs-commit is too simplistic/limited to handle all of the scenarios etckeeper may find itself operating under.
It's stupid, but I wonder if something like a parent-level argument passed to etckeeper when a VCS runs commands might not be the most universal approach.
i.e.
etckeeperitself gets logic similar to this, in its option parsing, to replace the simplistic "-d" handling that's currently there:(Or I suppose it could just use
getopt/getopts, since it's gotten complex enough.)Regardless,
ETCKEEPER_PARENT_LEVELis set to the numeric value that follows the-lflag, which would dictate how many levels up the process tree it should climb when looking for the relevant parent process. (The default would beETCKEEPER_PARENT_LEVEL=1, IOW look for the direct parent of the etckeeper script invocation.)The command in
libdnf5-plugins/actions.d/etckeeper.actionscould be changed to:...so that etckeeper knows it has to look two levels upward to find the relevant
dnfcommand, in that instance. Any other VCS integrations where etckeeper will execute at a deeper level than a direct child process could similarly provide an appropriate-largument for their process structure.It's unquestionably kludgey and hackish. But given the unpredictable variety of scenarios etckeeper may be expected to handle, it's also the most flexible approach. And etckeeper doesn't have to be extended with explicit supporting logic each time a new packaging tool or integration scenario comes up, in order to do the right thing.
ETCKEEPER_PARENT_LEVELwould be2, not1. The DNF scripts would pass-l 3when they runetckeeper, and thepacmanintegration would use-l 1.