Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=749132
--- Comment #3 from Ricardo Rocha rocha.porto@gmail.com 2011-10-31 11:07:32 EDT --- Hi Steve.
Thanks a lot for reviewing this package.
I'll try to do a couple of informal reviews soon, and will put the links here as you suggest.
Please see inline for the details fixes.
Issues to be checked: - -libs or not (see comment at the end) - gssapi_openssl.h (also details inline)
Spec URL: http://rocha.web.cern.ch/rocha/fedora/dpm-dsi.spec SRPM URL: http://rocha.web.cern.ch/rocha/fedora/dpm-dsi-1.8.2-1.src.rpm
(i've simply overwritten the previous files as this is not built/released yet. should i have increased the release number anyway?)
(In reply to comment #2)
Review of dpm-dsi, Sat 29th October 2011 https://bugzilla.redhat.com/show_bug.cgi?id=749132
[yes] specfiles match: dpm-dsi is the SVN module name. [no] source files match upstream: This is built from SVN. Your comments for createing the .spec file mention how to export the source but not how to create the tar ball. I realize it's obvious but please add it. See: http://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control Please expand on your instructions.
Done.
[yes] package meets naming and versioning guidelines. expect where detailed elsewhere. [no] spec is properly named, cleanly written, and uses macros consistently. You use '/usr' during the installation which should be %{prefix} or similar.
Changed to: make install prefix=$RPM_BUILD_ROOT%{_prefix}
[yes] dist tag is present. [yes] build root is correct. Though can be dropped unless EPEL5 is being targeted.
[no] license field matches the actual license. .spec file says ASL2.0 but src/globus_gridftp_server_dpm.c talks about a globus license.
On a similar note what is src/gssapi_openssl.h for instance, is not just a duplication of
Not sure what to do about gssapi_openssl.h.
I can't find it anywhere in Fedora with 'yum provides', there's a couple of other packages depending on it at build time but none shipping it.
Regarding the license, i had a look at the guidelines and it seems that the strictest license should stay, i guess in this case that's ASL2.0?
I'm glad to change it to something more appropriate if needed of course.
[yes] license is open source-compatible. ASL2.0 is but see above. [yes] license text included in package but see above. [notchecked] latest version is being packaged. [yes] BuildRequires are proper.
[no] compiler flags are appropriate. cc -g -Wall -fPIC -D_LARGEFILE64_SOURCE You just have a plain ./configure and not a %configure See: http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags
Updated to use: CFLAGS='${optflags}' ./configure ...
I can't simply use %configure as this is a hand written script not supporting most of the options given by the macro. But flags should be properly passed now.
[yes] %clean is present. But not needed unless EPEL5
[yes] package builds in mock. [no] package installs properly. See 'requires' below.
Fixed.
[yes] rpmlint is silent or justified. $ rpmlint dpm-dsi.spec dpm-dsi.spec: W: invalid-url Source0: dpm-dsi-1.8.2.tar.gz 0 packages and 1 specfiles checked; 0 errors, 1 warnings.
dpm-dsi.x86_64: W: incoherent-init-script-name dpm-gsiftp ('dpm-dsi', 'dpm-dsid')
This is expected
[no] final provides and requires are sane dpm-dsi-1.8.2-1.fc15.x86_64.rpm provides: libglobus_gridftp_server_dpm.so.1()(64bit) dpm-dsi(x86-64) = 1.8.2-1.fc15 dpm-dsi-devel-1.8.2-1.fc15.x86_64.rpm provides: dpm-dsi--devel = 1.8.2-1.fc15 dpm-dsi-devel(x86-64) = 1.8.2-1.fc15
dpm-dsi-1.8.2-1.fc15.x86_64.rpm requires config(dpm-dsi) = 1.8.2-1.fc15 globus-gridftp-server-progs(x86-64) initscripts libdl.so.2()(64bit) libdpm.so.1()(64bit) libglobus_ftp_control.so.1()(64bit) libvomsapi.so.1()(64bit) voms(x86-64)
So the explicit requirement 'voms(x86-64)' is not needed and should be removed.
dpm-dsi-devel-1.8.2-1.fc15.x86_64.rpm requires dpm-dsi-libs(x86-64) = 1.8.2-1.fc15 libglobus_gridftp_server_dpm.so.1()(64bit)
dpm-dsi-libs ? This is presumably just meant to be dpm-dsi unless you meant to create a seperate libs package in the first place?
'voms' removed, and sorry for not giving the -devel a try (just tried the installation of the main rpm, will get used to yum localinstall *rpm :-)).
dpm-dsi-devel now requires dpm-dsi (see comment at the end).
[none] %check is present and all tests pass: [none] no shared libraries are added to the regular linker search paths. [yes] owns the directories it creates. [yes] doesn't own any directories it shouldn't. [yes] no duplicates in %files. [yes] file permissions are appropriate. [no] scriptlets match those on ScriptletSnippets page. Check when ldconfig should be run, in all %post and %postun and not just on package removal.
Changed to execute ldconfig on postun upgrade too.
[yes] code, not content. [yes] documentation is small, so no -docs subpackage is necessary. [yes] %docs are not necessary for the proper functioning of the package. [question] headers are devel file. There are no header files in the -devel package?
There's one candidate, not included in the upstream install target though.
I've added it in the spec.
[yes] no pkgconfig files. [yes] no libtool .la droppings. [yes] desktop files valid and installed properly, no guis.
More comments: Can you confirm you are trargeting EPEL 5 (or even 4) otherwise a lot of "junk" can be dropped like BuildRoot.
Yes, confirmed. Target is EPEL 5 (not interested in 4).
You should add a '-p' to your install lines to preserve the timestamp on the files between releases.
Done.
The URL http://svnweb.cern.ch/trac/lcgdm/wiki/Dpm redirects to https and this causes an rpmlint warning. Is there any reason not to just use https in the first place.
Fixed.
The main one for now is dpm-dsi-libs or not since then I can actually install to check.
I added a dependency on the base package, following this: http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
but this means also the rest (daemon scripts, sysconfig, etc) will get installed. It's ok, but would be better to break it into -libs and depend on that one instead?
What is 'dsi' the package is called dpm-dsi but dsi is not mentined in the description at all?
Data Storage Interface (now documented in the description).
Thanks again.