[Bug 2173751] Review Request: mod_auth_cas - unretire package

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux