Spec URL: https://abompard.fedorapeople.org/reviews/mailman3/mailman3.spec SRPM URL: https://abompard.fedorapeople.org/reviews/mailman3/mailman3-3.2.0-1.fc28.src.rpm Description: The next generation of the GNU mailing list manager Fedora Account System Username: abompard I have these rpmlint warnings/errors: mailman3.noarch: E: non-readable /etc/mailman.cfg 640 → This is deliberate, only root and the mailman user may read this file because it may contain the database password. mailman3.noarch: E: non-standard-dir-perm /var/lib/mailman3/data 2775 → The SetGID bit is deliberate, so files inherit the group ownership. mailman3.noarch: W: no-manual-page-for-binary mailman3 → The project does not ship any. mailman3.noarch: W: file-not-in-%lang /usr/lib/python3.6/site-packages/mailman/testing/mailman-fr.mo mailman3.noarch: W: file-not-in-%lang /usr/lib/python3.6/site-packages/mailman/testing/mailman-xx.mo mailman3.noarch: W: file-not-in-%lang /usr/lib/python3.6/site-packages/mailman/testing/mailman-yy.mo → Those are only for the test suite mailman3.noarch: E: explicit-lib-dependency libselinux-utils mailman3.noarch: W: dangerous-command-in-%pre cp mailman3.noarch: W: dangerous-command-in-%posttrans rm → Those lines come from the most recent page I've found about packaging SELinux modules on the wiki: https://fedoraproject.org/wiki/SELinux/IndependentPolicy
This is an informal review. Some comments: - Source -> you can use %pypi_source https://fedoraproject.org/wiki/Packaging:Python#Source_Files_from_PyPI - Why does build require git? - A brief comment on what the patches do, apart from the reference URLs, is advisable - You can use %{py3_dist} to resolve to standardized names https://fedoraproject.org/wiki/Packaging:Python#Requires_and_BuildRequires_with_standardized_names - You can use %{?systemd_requires} as a shortcut for listing the per-scriptlet dependencies on systemd https://fedoraproject.org/wiki/Packaging:Scriptlets#Systemd - There is a %{_unitdir} macro for systemd service files https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations - %setup, %patch -> %autosetup applies patches automatically https://fedoraproject.org/wiki/Packaging:Guidelines#Applying_patches - %{__python3} setup.py build -> %py3_build, %{__python3} setup.py install -> %py3_install https://fedoraproject.org/wiki/Packaging:Python#Macros - Not needed: rm -rf %{buildroot} https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections - rm -rf src/%{pypi_name}.egg-info -> the egg-info is not binary and must be included https://fedoraproject.org/wiki/Packaging:Python#Files_to_include - Packages with cron job files must have an explicit Requires: crontabs https://fedoraproject.org/wiki/Packaging:CronFiles
Also, it fails to build with > BUILDSTDERR: error: Could not find suitable distribution for Requirement.parse('aiosmtpd>=1.1')
Taking this review.
@Aurelien, is there a reason that we're doing this instead of just updating mailman and splitting mailman2 out as an alternate package? The idea would be that mailman3 would be the default mailman people would install.
Sorry for taking so long to reply. Indeed the mailman 2.x package is going to stop working in Fedora pretty soon, with the end of Python 2, so it would make sense to rename mailman to mailman2 and mailman3 to mailman. I started with mailman3 because of EPEL, I expect mailman2 to be available there for a while. Plus since it's a new package, it's easier to not have it depend on the renaming of another already included package. Let's review this one first, make sure it's correct according to the guidelines, and discuss with the mailman2 maintainer what we want to do then. Would that work for you? Suggestions?
(In reply to Aurelien Bompard from comment #5) > Sorry for taking so long to reply. Indeed the mailman 2.x package is going > to stop working in Fedora pretty soon, with the end of Python 2, so it would > make sense to rename mailman to mailman2 and mailman3 to mailman. > > I started with mailman3 because of EPEL, I expect mailman2 to be available > there for a while. Plus since it's a new package, it's easier to not have it > depend on the renaming of another already included package. Let's review > this one first, make sure it's correct according to the guidelines, and > discuss with the mailman2 maintainer what we want to do then. Would that > work for you? Suggestions? Sounds good to me. We'd probably want to prepare a Self-Contained Change to switch mailman package to mailman 3 for Fedora 30.
(In reply to Iñaki Ucar from comment #1) > This is an informal review. Some comments: > > - Source -> you can use %pypi_source > https://fedoraproject.org/wiki/Packaging:Python#Source_Files_from_PyPI > This is fine to not do, especially since this needs to be EPEL compatible. > - Why does build require git? > Aurelien, did you intend to use Git to apply patches? If so, you should switch from %setup and %patch invocations to %autosetup -n %{pypi_name}-%{version} -S git > - A brief comment on what the patches do, apart from the reference URLs, is > advisable > > - You can use %{py3_dist} to resolve to standardized names > https://fedoraproject.org/wiki/Packaging: > Python#Requires_and_BuildRequires_with_standardized_names > The standardized names don't work in EPEL. A better alternative would be to use the dependency generator on Fedora, and manually specify only for EPEL. An example of an EPEL compatible way to set that up is here: https://src.fedoraproject.org/rpms/pagure/blob/master/f/pagure.spec > - You can use %{?systemd_requires} as a shortcut for listing the > per-scriptlet dependencies on systemd > https://fedoraproject.org/wiki/Packaging:Scriptlets#Systemd > > - There is a %{_unitdir} macro for systemd service files > https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations > Please use this. > - %setup, %patch -> %autosetup applies patches automatically > https://fedoraproject.org/wiki/Packaging:Guidelines#Applying_patches > As noted above, you may want to consider "%autosetup -n %{pypi_name}-%{version} -S git" > - %{__python3} setup.py build -> %py3_build, %{__python3} setup.py install > -> %py3_install > https://fedoraproject.org/wiki/Packaging:Python#Macros > Please do this. > - Not needed: rm -rf %{buildroot} > https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections > Please fix this. > - rm -rf src/%{pypi_name}.egg-info -> the egg-info is not binary and must be > included > https://fedoraproject.org/wiki/Packaging:Python#Files_to_include > Please fix this. > - Packages with cron job files must have an explicit Requires: crontabs > https://fedoraproject.org/wiki/Packaging:CronFiles Why are we using cron jobs instead of systemd timers?
(In reply to Neal Gompa from comment #7) > (In reply to Iñaki Ucar from comment #1) > > > - rm -rf src/%{pypi_name}.egg-info -> the egg-info is not binary and must be > > included > > https://fedoraproject.org/wiki/Packaging:Python#Files_to_include > > > > Please fix this. Actually, this is fine, since %py3_build would regenerate it.
> - Why does build require git? This comes from the SELinux module template here: https://fedoraproject.org/wiki/SELinux/IndependentPolicy#Creating_the_Spec_File I don't know if or why it's necessary, really. > A better alternative would be to use the dependency generator on Fedora, and > manually specify only for EPEL. Done. > - You can use %{?systemd_requires} as a shortcut for listing the > per-scriptlet dependencies on systemd > - There is a %{_unitdir} macro for systemd service files Done. > - %setup, %patch -> %autosetup applies patches automatically > - %{__python3} setup.py build -> %py3_build, %{__python3} setup.py install > -> %py3_install > - Not needed: rm -rf %{buildroot} > - rm -rf src/%{pypi_name}.egg-info -> the egg-info is not binary and must be included Done. Clearly that specfile was first written a while ago... > Why are we using cron jobs instead of systemd timers? You're right, I switched to timers. I've updated the spec file, thanks a lot for your review. Spec URL: https://abompard.fedorapeople.org/reviews/mailman3/mailman3.spec SRPM URL: https://abompard.fedorapeople.org/reviews/mailman3/mailman3-3.2.0-1.fc28.src.rpm
> %if 0%{?fedora} > %{!?python3_pkgversion: %global python3_pkgversion 3} > %else > %{!?python3_pkgversion: %global python3_pkgversion 36} > %endif This conditional should be flipped, and you need to tweak this to actually work correctly in EPEL (right now, everything points to py34). The way that this should probably be done is: %if 0%{?epel} && 0%{?epel} < 8 # We need to force Python 3.6 in EPEL %global python3_pkgversion 36 %global __python3 /usr/bin/python3.6 %else %{!?python3_pkgversion: %global python3_pkgversion 3} %endif You're also missing the following at the top of the spec: %{?python_enable_dependency_generator}
> %systemd_post %{name}.service > %systemd_post %{name}-digests.timer You can put multiple systemd units on a single invocation of %systemd_* macros. For example: %systemd_post %{name}.service %{name}-digests.timer This reduces the number of executions of systemctl and slightly improves the installation speed. Please do this for all of your %systemd_* macro invocations.
> This conditional should be flipped, and you need to tweak this to actually > work correctly in EPEL (right now, everything points to py34). EPEL won't work for the moment because the dependencies aren't built for Python 3.6, but thanks for the change. > You're also missing the following at the top of the spec: > %{?python_enable_dependency_generator} Fixed, thanks. > You can put multiple systemd units on a single invocation of %systemd_* > macros. Oh I didn't know that, nice. Updated: Spec URL: https://abompard.fedorapeople.org/reviews/mailman3/mailman3.spec SRPM URL: https://abompard.fedorapeople.org/reviews/mailman3/mailman3-3.2.0-1.fc28.src.rpm
Created attachment 1497582 [details] mailman 3 mock build log The tests for mailman 3 are failing in my rawhide build. I've attached the log for your examination.
OK, I've added 2 patches pulled from upstream for Rawhide (click 7 and python 3.7). Spec URL: https://abompard.fedorapeople.org/reviews/mailman3/mailman3.spec SRPM URL: https://abompard.fedorapeople.org/reviews/mailman3/mailman3-3.2.0-1.fc30.src.rpm
Review notes: [x]: Package is named according to package naming guidelines [x]: Package complies with Fedora Packaging Guidelines [x]: Package complies with Fedora Python Packaging Guidelines [!]: Package builds and installs correctly fedora-review and Mock are choking on installing the package. I suspect this has to do with the SELinux scriptlets, though. [!]: Package includes documentation and license content per guidelines "%license COPYING" is missing for the package file list. Could you please address the license file issue and post updated spec and SRPM links?
I added the license file. The rpm does not build at the moment because it requires a more recent aiosmtpd. I've updated it in rawhide but it's not available just yet. Spec URL: https://abompard.fedorapeople.org/reviews/mailman3/mailman3.spec SRPM URL: https://abompard.fedorapeople.org/reviews/mailman3/mailman3-3.2.0-1.fc29.src.rpm
Package looks good to me. PACKAGE APPROVED.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/mailman3
Thanks again. I've built the package for rawhide, building it for F29 would require upgrading the aiosmtpd library and first I'd like to make sure it doesn't break anything.
So for future reference, "next release" here means F30, not F29?
Yes, in this circumstance.