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(a)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.
--
Configure bugmail:
https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.