Spec URL: https://raw.githubusercontent.com/erigones/Ludolph/master/rpmbuild/SPECS/python-ludolph.spec SRPM URL: https://github.com/erigones/Ludolph/raw/master/rpmbuild/SRPMS/python-ludolph-1.0.1-1.fc26.src.rpm Description: Monitoring Jabber Bot with Zabbix support, completely written in Python. Features * Simple and modular design * Alerts from Zabbix * Multi-User Chat (XEP-0045) * Colorful messages (XEP-0071) * Attention (XEP-0224) * Avatars (XEP-0084) * Roster management and ACL configuration * Webhooks and Cron jobs * Plugins and commands Fedora Account System Username: ricco I am a co-creator of this bot and also an active developer and a package maintainer at GitHub. Repository: https://github.com/erigones/Ludolph This is my first package to Fedora, but hopefully not last and I will be able to find a sponsor. The bot has support to easily write plugins in Python, and my intention is to package some of the plugins as RPMs as well. Koji builds: f26: https://koji.fedoraproject.org/koji/taskinfo?taskID=21427587 f27: https://koji.fedoraproject.org/koji/taskinfo?taskID=21427631 f28: https://koji.fedoraproject.org/koji/taskinfo?taskID=21427756 rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=21427917
Will review the package and eventually sponsor Richard. I know him from real life. Lifting FE-NEEDSPONSOR.
Thanks for your contribution to Fedora :) Quick peek at the specfile: 1) this is there twice: BuildRequires: systemd 2) The %post, %preun, %postun as written would happen on the "main" package python-ludolph, but there is not such package built here, use %post python3-%{pypi_name} etc. to make them work with the package owning the %{_unitdir}/ludolph.service file: %post python3-%{pypi_name} %systemd_post ludolph.service ... You would also need to do this for the python2 subpackage, but see 3. 3) The %{_unitdir}/ludolph.service generally shouldn't be in both subpackages. The service calls /usr/bin/ludolph and that is only in the python3 subpackage. Having the file in both packages thus makes no sense. Also, a common file across 2 subpackages is almost always painful. I.e. if both packages are installed, they need to be in the same version-release, otherwise dnf/rpm would scream on us "File xyz form python2-... conflicts with file from python3-..." - rule of thumb -> avoid it when possible. Here it is possible and also easy. if not avoidable, create a common subpackage required by both subpackages needing the file (not relevant here). 4) The ghost config file is also shared and honestly I don't know if shared ghost files are problematic or not. Will investigate. 5) the systemd service file talks about /run/ludolph/ludolph.pid - and the package doesn't own /run/ludolph. I've looked at avahi (similar case), where /run/avahi-daemon/ belongs to the avahi package and it is a ghost (%ghost %dir %{_localstatedir}/run/avahi-daemon). The file inside does not belong to anything. Now I don't have a guideline here to quite, but what avahi does seems reasonable and others do it in similar way. (And the following are more curious questions, based of what I've seen last time we talked about this IRL.) 6) the package seems not to do anything about tmpfiles.d. Does it work? 7) no selinux problems? Otherwise the specfile look legible. Will do a full review once we finish this.
Also, when following 3, the python2 subpackage does not need to require systemd.
4) seems to be entirely OK.
8) There are 3 tests that run quite fine, please add a %check section that runs them both for python2 and python3 (note that you'll need to BuildRequire all the runtime Requires as well as the test runner (python2/3-nose)).
9) Also, please add the following provide to python3 subpackage, so users can just `dnf install ludolph`: Provides: %{pypi_name} = %{version}-%{release}
1) removed BuildRequires: systemd so it is there only one. 2) updated all sections, added with -n otherwise it was failing. %post -n python3-%{pypi_name} ... 3) I made python3 package the only one with the service and executable file. This way the python3 package is program and library and python2 package is just library. 4) Removed anyway as it is required only when executing in order to have a running instance. 5) You are right, I have tested systemd and it worked but I forgot to check that dir, but I didn't spot any issues during the installation or running a service. Updated to have a ghost dir. More on that in point 6. 6) We also have a ludolph.conf that is supposed to be copied in tmpfiles.d and once run systemd-tmpfiles --create it would create a directory in /run (according to ludolph.conf) where service (mentioned in 5) will store PID file. However, I have struggled with this for a while as fedora wiki (https://fedoraproject.org/wiki/Packaging:Tmpfiles.d) doesn't mention how to run systemd-tmpfiles --create I had to look to systemd source code to find out %tmpfiles_create macro. Not sure if I have permissions for wiki edit but it would definitely be helpful to mention it in the example. Systemd seems to work fine, even without doing this (conf file and run directory) and systemd is not complaining at all. It seems that it works fine either way, and I can not find even anything that doesn't work in Ludolph without the PID file. So it might be an obsolete code not used anywhere, will do further review and might be deleted in future versions. 7) Default settings have no SELinux issues, I have checked the logs and seems to be ok. I can imagine situations where user configuration can result in SELinux restrictions, but in that case, the user did that on purpose and he has to handle SELinux exceptions. The default is safe without problems. 8) I have almost forgotten about these :) Since it is very difficult to have a proper test as network connections would require a ton of mocking so there are only a few tests. I have added %check %{__python2} setup.py test %{__python3} setup.py test This seems to be working fine for python3 all test pass, but python2 does not execute the tests (not entirely sure why without further inspection). This way I do not need to introduce any new dependency to the project. Is this ok? 9) I have added that in python3 section as it makes sense only there as only this version has the executable script. I have updated SPEC and SRPM on GitHub, see the latest version there. My local build is working, but koji builds fail as it seems it can't connect to PyPI (at the moment, I will try again tommorow): Searching for dnspython Reading https://pypi.python.org/simple/dnspython/ Download error on https://pypi.python.org/simple/dnspython/: [Errno -2] Name or service not known -- Some packages may not be found!
(In reply to Richard Kellner from comment #7) > 2) updated all sections, added with -n otherwise it was failing. > > %post -n python3-%{pypi_name} > ... Oh, sorry, I forgot about the -n. > 5) You are right, I have tested systemd and it worked but I forgot to check > that dir, but I didn't spot any issues during the installation or running a > service. Updated to have a ghost dir. More on that in point 6. Generally there would not be any issues. This is just a "good manner". > 6) We also have a ludolph.conf that is supposed to be copied in tmpfiles.d > and once run systemd-tmpfiles --create it would create a directory in /run > (according to ludolph.conf) where service (mentioned in 5) will store PID > file. However, I have struggled with this for a while as fedora wiki > (https://fedoraproject.org/wiki/Packaging:Tmpfiles.d) doesn't mention how to > run systemd-tmpfiles --create I had to look to systemd source code to find > out %tmpfiles_create macro. Not sure if I have permissions for wiki edit but > it would definitely be helpful to mention it in the example. Probably no permissions for either of us. Changes there have to be proposed trough https://pagure.io/packaging-committee/issues > 8) I have almost forgotten about these :) Since it is very difficult to have > a proper test as network connections would require a ton of mocking so there > are only a few tests. I have added > > %check > %{__python2} setup.py test > %{__python3} setup.py test > > This seems to be working fine for python3 all test pass, but python2 does > not execute the tests (not entirely sure why without further inspection). > This way I do not need to introduce any new dependency to the project. Is > this ok? > > I have updated SPEC and SRPM on GitHub, see the latest version there. My > local build is working, but koji builds fail as it seems it can't connect to > PyPI (at the moment, I will try again tommorow): > > Searching for dnspython > Reading https://pypi.python.org/simple/dnspython/ > Download error on https://pypi.python.org/simple/dnspython/: [Errno -2] Name > or service not known -- Some packages may not be found! Koji builds cannot connect to the internet. You want to have all buidrequires specified and run test against stuff installed from RPM packages. It worked for me using: %check %{__python2} -m nose %{__python3} -m nose And adding: BuildRequires: python2-nose BuildRequires: python2-bottle BuildRequires: python2-dns BuildRequires: python2-sleekxmpp BuildRequires: python3-nose BuildRequires: python3-bottle BuildRequires: python3-dns BuildRequires: python3-sleekxmpp
6) I have opened an issue there: https://pagure.io/packaging-committee/issue/709 so we'll see. 8) I haven't realized that koji is cut out from the outside world. Anyways I understand that I have mixed pears with apples. I understood it from your first comment but I thought I would have one less dependency in Project and I didn't like the idea that in Fedora it would download one package that it doesn't download via pip. What I didn't realize is that it does so only during the build and not via regular install from repo. GutHub repo updated. Koji builds: f26: https://koji.fedoraproject.org/koji/taskinfo?taskID=21458294 f27: https://koji.fedoraproject.org/koji/taskinfo?taskID=21458463 f28: https://koji.fedoraproject.org/koji/taskinfo?taskID=21458532 rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=21458809
Cool. One more thing from me: Maybe the python2 subpackage review needs a more suitable description? Something like: Python 2 module with Ludolph, Monitoring Jabber Bot with Zabbix support. Use the python3-ludolph package to get the actual application and service. Formal review in progress.
Thanks. I have updated GitHub repo with your suggested description.
Package Review ============== Package is APPROVED. I'll sponsor you if you do at least one package review yourself. Feel free to pick some python package: https://fedoraproject.org/wiki/SIGs/Python#Review_Python_packages Please indicate that the review is not official, because you are not yet sponsored. Add me to CC (search for CC List on the page). Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== MUST items ===== Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. [x]: License file installed when any subpackage combination is installed. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [x]: Package contains systemd file(s) if in need. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 20480 bytes in 2 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local Python: [x]: Python eggs must not download any dependencies during the build process. [x]: A package which is used by another package via an egg interface should provide egg info. [x]: Package meets the Packaging Guidelines::Python [x]: Package contains BR: python2-devel or python3-devel [x]: Binary eggs must be removed in %prep ===== SHOULD items ===== Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [-]: Fully versioned dependency in subpackages if applicable. Note: Not applicable, no main package built. [?]: Package functions as described. [?]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Scriptlets must be sane, if used. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [?]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [?]: Packages should try to preserve timestamps of original installed files. [x]: Files in /run, var/run and /var/lock uses tmpfiles.d when appropriate [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: python2-ludolph-1.0.1-1.fc27.noarch.rpm python3-ludolph-1.0.1-1.fc27.noarch.rpm python-ludolph-1.0.1-1.fc27.src.rpm python3-ludolph.noarch: E: non-readable /etc/ludolph.cfg 0 python3-ludolph.noarch: E: dir-or-file-in-var-run /var/run/ludolph python3-ludolph.noarch: E: non-standard-dir-perm /var/run/ludolph 0 python3-ludolph.noarch: W: no-manual-page-for-binary ludolph python3-ludolph.noarch: W: empty-%postun 3 packages and 0 specfiles checked; 3 errors, 2 warnings. Rpmlint (installed packages) ---------------------------- python3-ludolph.noarch: E: non-readable /etc/ludolph.cfg 0 python3-ludolph.noarch: E: dir-or-file-in-var-run /var/run/ludolph python3-ludolph.noarch: E: non-standard-dir-perm /var/run/ludolph 0 python3-ludolph.noarch: W: no-manual-page-for-binary ludolph python3-ludolph.noarch: W: empty-%postun 2 packages and 0 specfiles checked; 3 errors, 2 warnings. The Errors are not applicable to ghost files. Empty %postun is weird, but %systemd_postun is empty, so probably desired. Requires -------- python2-ludolph (rpmlib, GLIBC filtered): python(abi) python2-bottle python2-dns python2-sleekxmpp python3-ludolph (rpmlib, GLIBC filtered): /bin/sh /usr/bin/python3 config(python3-ludolph) python(abi) python3-bottle python3-dns python3-sleekxmpp systemd Provides -------- python2-ludolph: python-ludolph python2-ludolph python2.7dist(ludolph) python2dist(ludolph) python3-ludolph: config(python3-ludolph) ludolph python3-ludolph python3.6dist(ludolph) python3dist(ludolph) Source checksums ---------------- https://files.pythonhosted.org/packages/source/l/ludolph/ludolph-1.0.1.tar.gz : CHECKSUM(SHA256) this package : 884a1fecabe1e1d5713d3a3a81b8ccee079831fa1b442f0cb0d6fe96a9d57acf CHECKSUM(SHA256) upstream package : 884a1fecabe1e1d5713d3a3a81b8ccee079831fa1b442f0cb0d6fe96a9d57acf
For review resources, see https://fedoraproject.org/wiki/Packaging:ReviewGuidelines and https://pagure.io/FedoraReview
I've sponsored you.
(fedrepo-req-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/python-ludolph. You may create the branch "f26" using git in about 10 minutes.
python-ludolph-1.0.1-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-e9862c651d
python-ludolph-1.0.1-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-e9862c651d
python-ludolph-1.0.1-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-6b048cca52
python-ludolph-1.0.1-1.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2017-724aa8d736
python-ludolph-1.0.1-1.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-724aa8d736
python-ludolph-1.0.1-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-6b048cca52
python-ludolph-1.0.1-1.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.
python-ludolph-1.0.1-1.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.
python-ludolph-1.0.1-1.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.