https://bugzilla.redhat.com/show_bug.cgi?id=2173751 --- Comment #8 from Carl George 🤠 <carl@xxxxxxxxxx> --- Thanks for submitting this re-review. This is a fairly old spec file, so it will need a bit of cleaning up to bring it up to modern Fedora packaging standards. Don't get discouraged by the following wall of text, it's not a criticism of you, just things we need to work on in the package. ================================================================================ The conditional %_httpd_* macro definitions at the start of the spec file are no longer necessary and should be removed. They're defined by httpd-devel in all current releases of RHEL and Fedora. ================================================================================ Since all current RHEL and Fedora release includes httpd 2.4, the sections of the spec that reference httpd 2.2 should probably be removed. ================================================================================ Optional suggestion, consider refreshing the summary and description with sections of the upstream README. The first sentence of the first section (without the period) would work well as the summary, and the first sentence of the introduction section would work well as the description. ================================================================================ There are a few deprectated tags and commands that should be removed. -Group: System Environment/Daemons -BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) -rm -rf %{buildroot} -%clean -rm -rf %{buildroot} https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections ================================================================================ The license field needs to use the new SPDX identifiers. -License: APLv2 +License: Apache-2.0 https://docs.fedoraproject.org/en-US/legal/license-field/#_basic_policy ================================================================================ Since this is a C program, you'll need to have a buildrequires for gcc. +BuildRequires: gcc https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/#_buildrequires_and_requires ================================================================================ This has a buildrequires on a deprecated package, pcre-devel. File an issue with upstream to see if they're interested in porting this program over to pcre2, and leave a comment above the pcre-devel line with a link to that issue. Technically during reviews depending on deprecated packages isn't allowed, but I think we can waive that since this is a re-review to unretire. https://docs.fedoraproject.org/en-US/packaging-guidelines/deprecating-packages/ ================================================================================ I don't see this in the guidelines, but other httpd modules packaged in Fedora tend to require httpd-mmn, not httpd directly. This will also allow users to install a smaller footprint of httpd with the httpd-core package on Fedora and RHEL9. -Requires: httpd +Requires: httpd-mmn = %{_httpd_mmn} ================================================================================ Instead of hardcoding the version in %prep it would be better to use the version macro, so that way when a new version is released it only has to be updated on one line. And since the default value for -n is %{name}-%{version}, we can just leave it off entirely. -%setup -q -n %{name}-1.2 +%setup -q ================================================================================ Switch to the modern %make_build and %make_install macros. -make %{?_smp_mflags} +%make_build -make install DESTDIR=%{buildroot} LIBEXECDIR=%{_httpd_moddir} +%make_install https://docs.fedoraproject.org/en-US/packaging-guidelines/#_parallel_make https://docs.fedoraproject.org/en-US/packaging-guidelines/#_why_the_makeinstall_macro_should_not_be_used ================================================================================ Instead of directly using the /var path, consider switching to %{_localstatedir}. -mkdir -p %{buildroot}/var/cache/httpd/%{name} +mkdir -p %{buildroot}%{_localstatedir}/cache/httpd/%{name} -%dir /var/cache/httpd/%{name} +%dir %{_localstatedir}/cache/httpd/%{name} ================================================================================ Setting permissions to root:root is not necessary since that is the default. -%defattr(-,root,root,-) For the one directory that needs apache:apache permissions, consider doing it in one line. -%defattr(-,apache,apache,-) -%dir %{_localstatedir}/cache/httpd/%{name} +%dir %attr(-,apache,apache) %{_localstatedir}/cache/httpd/%{name} https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_permissions ================================================================================ It's best practice to minimize usage of globs in %files. In this case they are matching one file each, so we can just list the full filename explictly. -%{_libdir}/httpd/modules/*.so -%config(noreplace) %{_httpd_confdir}/*.conf -%config(noreplace) %{_httpd_modconfdir}/*.conf +%{_libdir}/httpd/modules/mod_auth_cas.so +%config(noreplace) %{_httpd_confdir}/auth_cas.conf +%config(noreplace) %{_httpd_modconfdir}/10-auth_cas.conf https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_lists ================================================================================ The latest changelog entry doesn't match the release. -* Tue Mar 02 2021 Scott Williams <vwbusguy@xxxxxxxxxxxxxxxxx> - 1.2-0 +* Tue Mar 02 2021 Scott Williams <vwbusguy@xxxxxxxxxxxxxxxxx> - 1.2-1 -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2173751 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
