[Bug 749132] Review Request: dpm-dsi - Disk Pool Manager (DPM) plugin to GridFTP
bugzilla at redhat.com
bugzilla at redhat.com
Mon Oct 31 15:07:33 UTC 2011
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 at 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.
More information about the package-review
mailing list