Bug 1725584

Description of problem:

While reviewing frr I came across the below:

~~~
Issues:
=======
...
- systemd_post is invoked in %post, systemd_preun in %preun, and
  systemd_postun in %postun for Systemd service files.
  Note: Systemd service file(s) in frr
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/Scriptlets/#_scriptlets
~~~

The above got reported even if frr.spec contains:

~~~
%post
%systemd_post frr.service
...
%postun
%systemd_postun_with_restart frr.service
...
%preun
%systemd_preun frr.service
~~~


How reproducible:

Always.


Steps to Reproduce:

1. Run fedora-review on frr


Actual results:

The below gets reported:

~~~
- systemd_post is invoked in %post, systemd_preun in %preun, and
  systemd_postun in %postun for Systemd service files.
  Note: Systemd service file(s) in frr
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/Scriptlets/#_scriptlets
~~~


Expected results:

No report of the above.


Additional info:

Please, see the below comment containing thorough investigation results:

https://bugzilla.redhat.com/show_bug.cgi?id=1702720#c10

Copy&pasting the comment from there:

============================================================
It looks like a fedora-review bug to me. If we add extra debug statements, we find out that lines #2023 & #2029 (comment 9) containing:

rpm.expandMacro("%systemd_post .*.service")
rpm.expandMacro("%systemd_preun .*.service")

expand to:

~~~
if [ $1 -eq 1 ] ; then 
        # Initial installation 
        systemctl --no-reload preset .*.service >/dev/null 2>&1 || : 
fi

~~~

and (respectively):

~~~
if [ $1 -eq 0 ] ; then 
        # Package removal, not upgrade 
        systemctl --no-reload disable --now .*.service >/dev/null 2>&1 || : 
fi

~~~

whereas the %post and %preun from lines #2039 & #2040 (comment 9):

str(rpmpkg.post)
str(rpmpkg.preun)

expand to:

~~~
/bin/sh
 
if [ $1 -eq 1 ] && [ -x /usr/bin/systemctl ] ; then 
        # Initial installation 
        /usr/bin/systemctl --no-reload preset frr.service || : 
fi 


if [ -f /usr/share/info/frr.inf* ]; then
    install-info /usr/share/info/frr.info /usr/share/info/dir || :
fi

# Create dummy files if they don't exist so basic functions can be used.
if [ ! -e /etc/frr/frr.conf ]; then
    echo "hostname `hostname`" > /etc/frr/frr.conf
    chown frr:frr /etc/frr/frr.conf
    chmod 640 /etc/frr/frr.conf
fi
~~~

and (respectively):

~~~
/bin/sh
 
if [ $1 -eq 0 ] && [ -x /usr/bin/systemctl ] ; then 
        # Package removal, not upgrade 
        /usr/bin/systemctl --no-reload disable --now frr.service || : 
fi
~~~

while it is not easy to spot it at first, there is a missing part ">/dev/null 2>&1" in them which are present in the output from rpm.expandMacro().

This leads to setting 'failed' to true at line #2041 (comment 9) and reporting it as missing.

I don't have time to dig further to find out what exactly is behind this, that the actual code in frr.spec:

~~~
115 %post
116 %systemd_post frr.service
...
139 %preun
140 %systemd_preun frr.service
~~~

doesn't resolve to the same output as from rpm.expandMacro(), but that what's happening.




Additional info:

Code with debug statements and related parts:

/usr/lib/python3.7/site-packages/FedoraReview/rpm_file.py
~~~
 96     def _scriptlet(self, prog_tag, script_tag):
 97         """ Return inline -p script, script or None. """
 98         self.init()
 99         prog = self.header_to_str(prog_tag)
100         script = self.header_to_str(script_tag)
101         print("PROG", prog)
102         print("SCRIPT", script)
103         if prog and script:
104             return prog + script
105         if prog:
106             return prog
107         return script
...
124     @property
125     def preun(self):
126         """ Return preUn scriptlet. """
127         return self._scriptlet(rpm.RPMTAG_PREUNPROG, rpm.RPMTAG_PREUN)
128 
129     @property
130     def post(self):
131         """ Return post scriptlet. """
132         return self._scriptlet(rpm.RPMTAG_POSTINPROG, rpm.RPMTAG_POSTIN)
~~~

/usr/share/fedora-review/plugins/generic.py:
~~~
2012 class CheckSystemdUnitdirScriplets(GenericCheckBase):
2013     """ Check that Systemd unitdir scriplets are run if required. """
2014 
2015     def __init__(self, base):
2016         GenericCheckBase.__init__(self, base)
2017         self.url = (
2018             "https://docs.fedoraproject.org/en-US"
2019             "/packaging-guidelines/Scriptlets/#_scriptlets"
2020         )
2021         self.text = (
2022             "systemd_post is invoked in %post, systemd_preun in"
2023             " %preun, and systemd_postun in %postun for Systemd"
2024             " service files."
2025         )
2026         self.automatic = True
2027         self.type = "MUST"
2028 
2029     def run(self):
2030         using = []
2031         failed = False
2032         systemd_post_re = re.compile(
2033             re.escape(rpm.expandMacro("%systemd_post .*.service")).replace(
2034                 r"\.\*", ".*"
2035             )[2:-4],
2036             re.M,
2037         )
2038         systemd_preun_re = re.compile(
2039             re.escape(rpm.expandMacro("%systemd_preun .*.service")).replace(
2040                 r"\.\*", ".*"
2041             )[2:-4],
2042             re.M,
2043         )
2044         print("SYSTEMD_POST_RE", systemd_post_re)
2045         print("SYSTEMD_PREUN_RE", systemd_preun_re)
2046         print("EXPANDMACRO POST", rpm.expandMacro("%systemd_post .*.service"))
2047         print("EXPANDMACRO PREUN", rpm.expandMacro("%systemd_preun .*.service"))
2048         for pkg in self.spec.packages:
2049             print("PKG", pkg)
2050             if self.rpms.find("/usr/lib/systemd/system/*", pkg):
2051                 using.append(pkg)
2052                 rpmpkg = self.rpms.get(pkg)
2053                 print("STR(RPMKG.POST)", str(rpmpkg.post))
2054                 print("STR(RPMKG.PREUN)", str(rpmpkg.preun))
2055                 if not systemd_post_re.search(
2056                     str(rpmpkg.post)
2057                 ) or not systemd_preun_re.search(str(rpmpkg.preun)):
2058                     print("FAILED")
2059                     failed = True
2060 
2061         if not using:
2062             self.set_passed(self.NA)
2063             return
2064         text = "Systemd service file(s) in " + ", ".join(using)
2065         self.set_passed(self.FAIL if failed else self.PASS, text)
~~~

debug output from the above altered code:

~~~
SYSTEMD_POST_RE re.compile('if\\ \\[\\ \\$1\\ \\-eq\\ 1\\ \\]\\ ;\\ then\\ \\\n\\ \\ \\ \\ \\ \\ \\ \\ \\#\\ Initial\\ installation\\ \\\n\\ \\ \\ \\ \\ \\ \\ \\ systemctl\\ \\-\\-no\\-reload\\ preset\\ .*\\.service\\ >/dev/nul, re.MULTILINE)
SYSTEMD_PREUN_RE re.compile('if\\ \\[\\ \\$1\\ \\-eq\\ 0\\ \\]\\ ;\\ then\\ \\\n\\ \\ \\ \\ \\ \\ \\ \\ \\#\\ Package\\ removal,\\ not\\ upgrade\\ \\\n\\ \\ \\ \\ \\ \\ \\ \\ systemctl\\ \\-\\-no\\-reload\\ disable\\ \\-\\-now\\, re.MULTILINE)
EXPANDMACRO POST 
if [ $1 -eq 1 ] ; then 
        # Initial installation 
        systemctl --no-reload preset .*.service >/dev/null 2>&1 || : 
fi 

EXPANDMACRO PREUN 
if [ $1 -eq 0 ] ; then 
        # Package removal, not upgrade 
        systemctl --no-reload disable --now .*.service >/dev/null 2>&1 || : 
fi 

PKG frr
PROG /bin/sh
SCRIPT 
 
if [ $1 -eq 1 ] && [ -x /usr/bin/systemctl ] ; then 
        # Initial installation 
        /usr/bin/systemctl --no-reload preset frr.service || : 
fi 


if [ -f /usr/share/info/frr.inf* ]; then
    install-info /usr/share/info/frr.info /usr/share/info/dir || :
fi

# Create dummy files if they don't exist so basic functions can be used.
if [ ! -e /etc/frr/frr.conf ]; then
    echo "hostname `hostname`" > /etc/frr/frr.conf
    chown frr:frr /etc/frr/frr.conf
    chmod 640 /etc/frr/frr.conf
fi
STR(RPMKG.POST) /bin/sh
 
if [ $1 -eq 1 ] && [ -x /usr/bin/systemctl ] ; then 
        # Initial installation 
        /usr/bin/systemctl --no-reload preset frr.service || : 
fi 


if [ -f /usr/share/info/frr.inf* ]; then
    install-info /usr/share/info/frr.info /usr/share/info/dir || :
fi

# Create dummy files if they don't exist so basic functions can be used.
if [ ! -e /etc/frr/frr.conf ]; then
    echo "hostname `hostname`" > /etc/frr/frr.conf
    chown frr:frr /etc/frr/frr.conf
    chmod 640 /etc/frr/frr.conf
fi
PROG /bin/sh
SCRIPT 
 
if [ $1 -eq 0 ] && [ -x /usr/bin/systemctl ] ; then 
        # Package removal, not upgrade 
        /usr/bin/systemctl --no-reload disable --now frr.service || : 
fi
STR(RPMKG.PREUN) /bin/sh
 
if [ $1 -eq 0 ] && [ -x /usr/bin/systemctl ] ; then 
        # Package removal, not upgrade 
        /usr/bin/systemctl --no-reload disable --now frr.service || : 
fi
PROG /bin/sh
SCRIPT 
 
if [ $1 -eq 1 ] && [ -x /usr/bin/systemctl ] ; then 
        # Initial installation 
        /usr/bin/systemctl --no-reload preset frr.service || : 
fi 


if [ -f /usr/share/info/frr.inf* ]; then
    install-info /usr/share/info/frr.info /usr/share/info/dir || :
fi

# Create dummy files if they don't exist so basic functions can be used.
if [ ! -e /etc/frr/frr.conf ]; then
    echo "hostname `hostname`" > /etc/frr/frr.conf
    chown frr:frr /etc/frr/frr.conf
    chmod 640 /etc/frr/frr.conf
fi
FAILED
PKG frr-debuginfo
PKG frr-debugsource
~~~
============================================================