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@xxxxxxxxx> 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review