Spec URL: http://fedorapeople.org/cgit/adelton/public_git/mod_lookup_identity.git/plain/mod_lookup_identity.spec SRPM URL: http://copr-be.cloud.fedoraproject.org/results/adelton/identity_demo/fedora-19-x86_64/mod_lookup_identity-0.8.1-2.fc19/mod_lookup_identity-0.8.1-2.fc19.src.rpm Description: mod_lookup_identity can retrieve GECOS and group information about user authenticated in Apache httpd server, as well as attributes about the user provided by local sssd, and store these values in notes/environment variables to be consumed by web applications. Use of REMOTE_USER_* environment variables is recommended. Fedora Account System Username: adelton
As agreed on IRC, I'm stealing this package review from Nathaniel.
The specfile mostly looks good and both build and the binary works as advertised. The package builds cleanly in Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=6470246 I have some comments and questions, though: * You shouldn't Require httpd itself, but rather httpd-mmn. "mmn" stands for "module magic number" and will ensure that you only Require the version of httpd that has compatible module API. httpd-mmn is provided by httpd, it's just versioned, so the dependency tree stays the same. You can use the following one-liner: %{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat %{_includedir}/httpd/.mmn || echo 0-0)}} And then: Requires: httpd-mmn = %{_httpd_mmn} * Why do you filter the auto-provides? I think the comment should also explain "why", not only "what". * I think the description could be improved, too. Currently it says: """ mod_lookup_identity can retrieve GECOS and group information about user authenticated in Apache httpd server. """ I don't think the descrption should pitch GECOS in particular, but rather only talk about generic attributes, as it does later. * Most importantly -- you don't use the standard Fedora flags during %build, doesn't that also omit flags that improve security? I think an Apache module should be compiled in total paranoia mode.. For a local build, the following worked for me: %{_httpd_apxs} -c -Wc,"%{optflags} -Wall -pedantic $(pkg-config --cflags dbus-1)" $(pkg-config --libs dbus-1) mod_lookup_identity.c (I also moved the source file name to the end to separate the flags from the source on the build command line.) * When the sssd-ifp package is in Fedora, the Requires should change from dbus-libs to sssd-ifp. It's OK to keep the module as-is for now, but it's something we should keep in mind for the future.
(In reply to Jakub Hrozek from comment #2) > > * You shouldn't Require httpd itself, but rather httpd-mmn. "mmn" stands for > "module magic number" and will ensure that you only Require the version of > httpd that has compatible module API. httpd-mmn is provided by httpd, it's > just versioned, so the dependency tree stays the same. You can use the > following one-liner: > %{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat > %{_includedir}/httpd/.mmn || echo 0-0)}} > And then: > Requires: httpd-mmn = %{_httpd_mmn} Per https://fedoraproject.org/wiki/PackagingDrafts/ApacheHTTPModules I've attempted to define the macro as %{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat %{_includedir}/httpd/.mmn || echo missing-httpd-devel)}} My problem however is that I don't seem to be able to get httpd-devel installed on Fedora 21 in koji: http://kojipkgs.fedoraproject.org//work/tasks/930/6470930/root.log The net effect is that in build.log, there is then cat: /usr/include/httpd/.mmn: No such file or directory error: line 20: Double separator '-' in: missing-httpd-devel: Requires: httpd-mmn = missing-httpd-devel Building target platforms: x86_64 I could go with 0-0 as you propose but I wonder why the behaviour is different on Fedora 21. Also, having the missing-httpd-devel acts as nice safeguard against broken setup ... but I am not sure how to fix the setup. Note that on Fedora 20 it works fine: http://kojipkgs.fedoraproject.org//work/tasks/926/6470926/root.log
(In reply to Jan Pazdziora from comment #3) > cat: /usr/include/httpd/.mmn: No such file or directory > error: line 20: Double separator '-' in: missing-httpd-devel: Requires: > httpd-mmn = missing-httpd-devel > Building target platforms: x86_64 This is caused by change in rpmbuild and we have fixed this problem in all packages depending on httpd in rawhide last week. I've just updated the documentation you've been using.
(In reply to Jan Pazdziora from comment #3) > (In reply to Jakub Hrozek from comment #2) > > > > * You shouldn't Require httpd itself, but rather httpd-mmn. "mmn" stands for > > "module magic number" and will ensure that you only Require the version of > > httpd that has compatible module API. httpd-mmn is provided by httpd, it's > > just versioned, so the dependency tree stays the same. You can use the > > following one-liner: > > %{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat > > %{_includedir}/httpd/.mmn || echo 0-0)}} > > And then: > > Requires: httpd-mmn = %{_httpd_mmn} > > Per https://fedoraproject.org/wiki/PackagingDrafts/ApacheHTTPModules I've > attempted to define the macro as > I pinged one of the httpd developers. This draft was just outdated because of changes they did in rawhide recently. They've fixed the draft to recommend 0-0 now. > %{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat > %{_includedir}/httpd/.mmn || echo missing-httpd-devel)}} > > My problem however is that I don't seem to be able to get httpd-devel > installed on Fedora 21 in koji: > > http://kojipkgs.fedoraproject.org//work/tasks/930/6470930/root.log > I'm not sure what to look for in that log, I don't even see httpd-devel requested.
(In reply to Jakub Hrozek from comment #5) > (In reply to Jan Pazdziora from comment #3) > > %{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat > > %{_includedir}/httpd/.mmn || echo missing-httpd-devel)}} > > > > My problem however is that I don't seem to be able to get httpd-devel > > installed on Fedora 21 in koji: > > > > http://kojipkgs.fedoraproject.org//work/tasks/930/6470930/root.log > > > > I'm not sure what to look for in that log, I don't even see httpd-devel > requested. This gets fixed automatically once you will use 0-0 instead of missing-httpd-devel.
New .spec: http://fedorapeople.org/cgit/adelton/public_git/mod_lookup_identity.git/plain/mod_lookup_identity.spec?id=mod_lookup_identity-0.8.2 Diff against the previous .spec: http://fedorapeople.org/cgit/adelton/public_git/mod_lookup_identity.git/diff/mod_lookup_identity.spec?id=mod_lookup_identity-0.8.2&id2=mod_lookup_identity-0.8.1 New .src.rpm: http://copr-be.cloud.fedoraproject.org/results/adelton/identity_demo/fedora-19-x86_64/mod_lookup_identity-0.8.2-1.fc19/mod_lookup_identity-0.8.2-1.fc19.src.rpm I've addressed all issues from comment 2 except the comment about the filter_provides_in. That one is already in master but not in the 0.8.2, and I'd prefer not to respin because of it.
Thanks for the changes. The package now looks good and builds fine. There is one more change I'd like to see before I approve the package. The spec file explicitly requires dbus-libs, but that's not necessary, RPM does the right thing on its own: $ rpm -qp --requires /home/remote/jhrozek/rpmbuild/RPMS/x86_64/mod_lookup_identity-0.8.2-1.fc20.x86_64.rpm | grep dbus dbus-libs libdbus-1.so.3()(64bit) (the former is explicit, the latter autogenerated) actually even rpmdiff warns about the explicit requires: $ rpmlint /home/remote/jhrozek/rpmbuild/RPMS/x86_64/mod_lookup_identity-0.8.2-1.fc20.x86_64.rpm mod_lookup_identity.x86_64: E: explicit-lib-dependency dbus-libs mod_lookup_identity.x86_64: W: spelling-error %description -l en_US lookup -> lockup, hookup, look up The second warning is a false positive.
New .spec: http://fedorapeople.org/cgit/adelton/public_git/mod_lookup_identity.git/plain/mod_lookup_identity.spec?id=mod_lookup_identity-0.8.3 Diff against the previous .spec: http://fedorapeople.org/cgit/adelton/public_git/mod_lookup_identity.git/diff/mod_lookup_identity.spec?id=mod_lookup_identity-0.8.3&id2=mod_lookup_identity-0.8.2 New .src.rpm: http://adelton.fedorapeople.org/mod_lookup_identity-0.8.3-1.fc19.src.rpm
The spec file looks good to me, file ownership is sane, license matches the upstream, the package passed my sanity test. The tarball matches: * rpm: 16c9377e839a959172a2dfe7d32de5bf * upstream [*]: 16c9377e839a959172a2dfe7d32de5bf The package builds fine in Mock: * http://koji.fedoraproject.org/koji/taskinfo?taskID=6487502 Full rpmlint output: mod_lookup_identity.x86_64: W: spelling-error %description -l en_US lookup -> lockup, hookup, look up mod_lookup_identity-debuginfo.x86_64: W: spelling-error Summary(en_US) lookup -> lockup, hookup, look up mod_lookup_identity-debuginfo.x86_64: W: spelling-error %description -l en_US lookup -> lockup, hookup, look up This package is APPROVED. [*] You should fix two bugs on your website - there is no link to 0.8.3 on http://www.adelton.com/apache/mod_lookup_identity/ - following the link to "mod_lookup_identity 0.8.2" takes me to "http://www.adelton.com/apache/mod_authnz_pam/"
(In reply to Jakub Hrozek from comment #10) > > This package is APPROVED. Thanks. > [*] You should fix two bugs on your website > - there is no link to 0.8.3 on > http://www.adelton.com/apache/mod_lookup_identity/ > - following the link to "mod_lookup_identity 0.8.2" takes me to > "http://www.adelton.com/apache/mod_authnz_pam/" Both fixed. New Package SCM Request ======================= Package Name: mod_lookup_identity Short Description: Apache module to retrieve additional information about the authenticated user Owners: adelton Branches: f20 InitialCC:
Git done (by process-git-requests).
mod_lookup_identity-0.8.3-1.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/mod_lookup_identity-0.8.3-1.fc20
mod_lookup_identity-0.8.3-1.fc20 has been pushed to the Fedora 20 testing repository.
mod_lookup_identity-0.8.3-1.fc20 has been pushed to the Fedora 20 stable repository.