01729f
From 8266a4fb45621e08085c58537a531f33fc7eca74 Mon Sep 17 00:00:00 2001
01729f
From: Alan Jenkins <alan.christopher.jenkins@gmail.com>
01729f
Date: Wed, 10 Apr 2019 00:19:14 +0100
01729f
Subject: [PATCH 1/2] DNF: stderr is not ours to log to (and doing so breaks
01729f
 Ansible)
01729f
01729f
stderr does not belong to etckeeper-dnf
01729f
---------------------------------------
01729f
01729f
The Ansible dnf module uses the python dnf bindings.  In contexts like
01729f
these, stdout/stderr is owned by the host app (Ansible).  dnf should not
01729f
mess with stdout/stderr, unless the host app asks it to log there.
01729f
01729f
Specifically, we were breaking the JSON output of the Ansible dnf module.
01729f
This was only noticeable when the etckeeper message began with a "["
01729f
character.  Ansible has a mechanism to try and skip header messages like
01729f
login banners.  However, "[" is a valid character to start a JSON document.
01729f
01729f
https://unix.stackexchange.com/questions/511210/ansible-dnf-module-module-failure/
01729f
01729f
Solution
01729f
--------
01729f
01729f
Instead, log any non-zero exit status through the dnf logger.
01729f
01729f
For the pre-transaction etckeeper run, this message replaces an exception.
01729f
So we now avoid logging a traceback, which did not appear to add anything
01729f
useful.  (In my testing, dnf was continued to install after the exception
01729f
was logged).
01729f
01729f
I have also added a warning message for the post-transaction etckeeper run.
01729f
01729f
I switched from os.system() to subprocess.call().  The latter interface
01729f
supports better error reporting.  (When I tested os.system(), it returned
01729f
an errno number which was indistinguishable from an exit code).
01729f
01729f
etckeeper >/dev/null 2>&1
01729f
--------------------------
01729f
01729f
Any specific error messages from etckeeper are now discarded.  Because
01729f
unfortunately, python conventions about passing text strings have been
01729f
somewhat unclear.  It is an error to log strings if the encoding settings
01729f
of a registered logger cannot handle them.  Because a "bad" character
01729f
causes the entire string to be discarded, and optionally an exception
01729f
printed to stderr.  In a previous proposal I showed code that should be
01729f
correct in all expected cases.  However, the length of comment required to
01729f
define "all expected cases" was not very reassuring.
01729f
01729f
That was on top of explaining that dnf has a workaround that will avoid
01729f
raising exceptions when arbitrary unicode is logged to stdout.  My proposal
01729f
had to include that explanation, to show that we were not breaking a
01729f
system package manager.
01729f
01729f
It might sound strange to talk about error messages with characters
01729f
we cannot encode.  But I have one word for you: "filenames".
01729f
01729f
(After the recent python PR #14008[1], logging.basicConfig() defaults
01729f
to errors='backslashreplace'.  Also, errors= can be manually specified when
01729f
creating a lower-level logging "Handler".  I think this will resolve the
01729f
problem in the future.  It means that when programs are written for this
01729f
new version of python, it should be the responsibility of the log setup
01729f
code to prevent UnicodeEncodeError.)
01729f
01729f
[1] https://github.com/python/cpython/pull/14008
01729f
---
01729f
 etckeeper-dnf/etckeeper.py | 27 ++++++++++++++++-----------
01729f
 1 file changed, 16 insertions(+), 11 deletions(-)
01729f
01729f
diff --git a/etckeeper-dnf/etckeeper.py b/etckeeper-dnf/etckeeper.py
01729f
index e8a1a51..69edd88 100644
01729f
--- a/etckeeper-dnf/etckeeper.py
01729f
+++ b/etckeeper-dnf/etckeeper.py
01729f
@@ -9,7 +9,7 @@
01729f
 
01729f
 from dnfpluginscore import logger
01729f
 
01729f
-import os
01729f
+import subprocess
01729f
 import dnf
01729f
 
01729f
 
01729f
@@ -17,20 +17,25 @@ class Etckeeper(dnf.Plugin):
01729f
 
01729f
     name = 'etckeeper'
01729f
 
01729f
-    def _out(self, msg):
01729f
-        logger.debug('Etckeeper plugin: %s', msg)
01729f
+    def _run_command(self, command):
01729f
+        logger.debug('Etckeeper plugin: %s', command)
01729f
+        try:
01729f
+            with open("/dev/null", "wb") as devnull:
01729f
+                ret = subprocess.call(("etckeeper", command),
01729f
+                                      stdout=devnull, stderr=devnull,
01729f
+                                      close_fds=True)
01729f
+                if ret > 0:
01729f
+                    logger.warning('"etckeeper %s" failed (exit code %d)' % (command, ret))
01729f
+                if ret < 0:
01729f
+                    logger.warning('"etckeeper %s" died (signal %d)' % (command, -ret))
01729f
+        except OSError as err:
01729f
+            logger.warning('Failed to run "etckeeper %s": %s' % (command, err))
01729f
 
01729f
     def resolved(self):
01729f
-        self._out('pre transaction commit')
01729f
-        command = '%s %s' % ('etckeeper', " pre-install")
01729f
-        ret = os.system(command)
01729f
-        if ret != 0:
01729f
-            raise dnf.exceptions.Error('etckeeper returned %d' % (ret >> 8))
01729f
+        self._run_command("pre-install")
01729f
 
01729f
     def transaction(self):
01729f
-        self._out('post transaction commit')
01729f
-        command = '%s %s > /dev/null' % ('etckeeper', "post-install")
01729f
-        os.system(command)
01729f
+        self._run_command("post-install")
01729f
 
01729f
 if __name__ == "__main__":
01729f
     from distutils.core import setup
01729f
-- 
01729f
2.23.0
01729f
01729f
01729f
From 7cda9678b1e60c0495a2a522721b319843d5fae0 Mon Sep 17 00:00:00 2001
01729f
From: Alan Jenkins <alan.christopher.jenkins@gmail.com>
01729f
Date: Tue, 23 Apr 2019 20:15:01 +0100
01729f
Subject: [PATCH 2/2] Do not use dnfpluginscore.logger
01729f
01729f
dnfpluginscore is not core support for dnf plugins, it is just a
01729f
collection of "core plugins" for dnf.  There is equally
01729f
dnfpluginsextras and dnfpluginsextras.logger.
01729f
01729f
A comment in dnf.logger comment says the logger name 'dnf.plugin' is "api".
01729f
So we can safely rely on that name.
01729f
01729f
Technically you can install etckeeper without the dnfpluginscore package
01729f
(at least I managed to run into this for python2, on a Fedora 29 system
01729f
which uses python3 for dnf by default).
01729f
---
01729f
 etckeeper-dnf/etckeeper.py | 5 +++--
01729f
 1 file changed, 3 insertions(+), 2 deletions(-)
01729f
01729f
diff --git a/etckeeper-dnf/etckeeper.py b/etckeeper-dnf/etckeeper.py
01729f
index 69edd88..d9dd2c3 100644
01729f
--- a/etckeeper-dnf/etckeeper.py
01729f
+++ b/etckeeper-dnf/etckeeper.py
01729f
@@ -7,11 +7,12 @@
01729f
 # Distutils code below was copied from etckeeper-bzr distributed with v1.15
01729f
 #
01729f
 
01729f
-from dnfpluginscore import logger
01729f
-
01729f
+import logging
01729f
 import subprocess
01729f
 import dnf
01729f
 
01729f
+logger = logging.getLogger('dnf.plugin')
01729f
+
01729f
 
01729f
 class Etckeeper(dnf.Plugin):
01729f
 
01729f
-- 
01729f
2.23.0
01729f