Bug 1725584 - fedora-review: strange behavior in CheckSystemdUnitdirScriplets()
Summary: fedora-review: strange behavior in CheckSystemdUnitdirScriplets()
Keywords:
Status: NEW
Alias: None
Product: Fedora
Classification: Fedora
Component: fedora-review
Version: 31
Hardware: Unspecified
OS: Unspecified
unspecified
low
Target Milestone: ---
Assignee: Stanislav Ochotnicky
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-07-01 06:43 UTC by Martin Osvald 🛹
Modified: 2019-08-13 19:07 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:


Attachments (Terms of Use)

Description Martin Osvald 🛹 2019-07-01 06:43:11 UTC
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
~~~
============================================================

Comment 1 Ben Cotton 2019-08-13 17:12:58 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 31 development cycle.
Changing version to '31'.

Comment 2 Ben Cotton 2019-08-13 19:07:40 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 31 development cycle.
Changing version to 31.


Note You need to log in before you can comment on or make changes to this bug.