Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: dpm-dsi - Disk Pool Manager (DPM) plugin to GridFTP
https://bugzilla.redhat.com/show_bug.cgi?id=749132
Summary: Review Request: dpm-dsi - Disk Pool Manager (DPM) plugin to GridFTP Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: unspecified Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: rocha.porto@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org Classification: Fedora Story Points: --- Type: ---
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 Description: The dpm-dsi package provides a Disk Pool Manager (DPM) plugin for the Globus GridFTP server.
The Disk Pool Manager (DPM) is a lightweight storage solution for grid sites. It offers a simple way to create a disk-based grid storage element and supports relevant protocols (SRM, gridFTP, RFIO) for file management and access.
Globus provides open source grid software, including a server implementation of the GridFTP protocol. This plugin implements the DPM backend specifics required to expose the data using this protocol.
This is my first package, and I am looking for a sponsor.
Other packages related to the DPM component will follow, covering the bits not yet available in Fedora/EPEL - the core components are already built in Fedora (see lcgdm).
This is part of a more general effort to get software already available and packaged in the EMI project (http://www.eu-emi.eu/) directly available in the main distributions.
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
Ricardo Rocha rocha.porto@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841(FE-NEEDSPONSOR)
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
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |steve.traylen@cern.ch Flag| |fedora-review?
--- Comment #1 from Steve Traylen steve.traylen@cern.ch 2011-10-29 04:57:30 EDT --- Am happy to sponsor you, I see you have three other packages in total up for review which is good.
nagios-plugins-lcgdm, Bug #749232 dpm-xrootd, Bug #749291 lcgdm-dav, Bug #749299
I'll start looking at couple of these, if you do at least one informal review of another package this is important to demonstrate you know the guidelines. Pick one from: http://fedoraproject.org/PackageReviewStatus/NEW.html and after informal review leave a link here to it.
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 #2 from Steve Traylen steve.traylen@cern.ch 2011-10-29 09:19:46 EDT --- 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.
[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.
[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
[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
[yes] %clean is present. But not needed unless EPEL5
[yes] package builds in mock. [no] package installs properly. See 'requires' below.
[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?
[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.
[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? [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.
You should add a '-p' to your install lines to preserve the timestamp on the files between releases.
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.
The main one for now is dpm-dsi-libs or not since then I can actually install to check.
What is 'dsi' the package is called dpm-dsi but dsi is not mentined in the description at all?
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.
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 #4 from Steve Traylen steve.traylen@cern.ch 2011-10-31 11:18:49 EDT ---
(i've simply overwritten the previous files as this is not built/released yet. should i have increased the release number anyway?)
fine this time but please increase Release at every update. This is standard practice for reviews.
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 #5 from Steve Traylen steve.traylen@cern.ch 2011-11-02 15:48:42 EDT --- (In reply to comment #3)
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?
Breaking out a libs pkg can makes sense, it means you will get the 32bit libs in the 64 bit repos.
However is anyone ever going to want the libraries and not the service and start up scripts. People are unlikely to link to this library I would have thought? Up to you.
Is gssapi_openssl.h not just an old version of
$ rpm -qf /usr/include/globus/gssapi.h globus-gssapi-gsi-devel-7.8-1.fc15.x86_64
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 #6 from Ricardo Rocha rocha.porto@gmail.com 2011-11-03 05:54:27 EDT --- (In reply to comment #5)
(In reply to comment #3)
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?
Breaking out a libs pkg can makes sense, it means you will get the 32bit libs in the 64 bit repos.
However is anyone ever going to want the libraries and not the service and start up scripts. People are unlikely to link to this library I would have thought? Up to you.
I'll leave it as it is then, noone should be linking against the library.
Is gssapi_openssl.h not just an old version of
$ rpm -qf /usr/include/globus/gssapi.h globus-gssapi-gsi-devel-7.8-1.fc15.x86_64
Don't think so. The original is part of the globus source: http://viewcvs.globus.org/viewcvs.cgi/gsi/gssapi/source/library/gssapi_opens...
Ricardo
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
Mattias Ellert mattias.ellert@fysast.uu.se changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mattias.ellert@fysast.uu.se
--- Comment #7 from Mattias Ellert mattias.ellert@fysast.uu.se 2011-11-03 11:10:38 EDT --- When looking around I see (slightly different) copies of the gssapi_openssl.h scattered around various source trees:
- globus-gssapi-gsi (which seems to be the original) - CGSI-gSOAP - lcgdm - lcas - lcas-lcmaps-gt4-interface - lcas-plugins-voms - lcmaps
The globus-gssapi-gsi Makefile.am file lists this file under libglobus_gssapi_gsi_la_SOURCES and not under include_HEADERS, so the authors considers this to be an internal header.
Relying on copies of internal headers of your dependencies is not a good thing.
Considering that so many projects seems to use copies of this file it would make sense to contact globus upstream and ask them what they think about installing this header as part of the installation.
I don't know if the whole file is used or if only parts of it. Maybe it would make sense to split it in a public installed part and a part that remains private.
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 #8 from Mattias Ellert mattias.ellert@fysast.uu.se 2011-11-03 14:25:20 EDT --- I'm not suggesting this has to be resolved before the package is approved. After all at least 2 packages already in Fedora has a copy of the same file.
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 #9 from Steve Traylen steve.traylen@cern.ch 2011-11-03 16:04:21 EDT --- Ricardo, If possible can you at least take the newer file from globus.org since is after globus switched to the apache license.
Otherwise this package becomes 'ASL2.0 and Globus' where globus is now the old defunct Globus license which is today not fedora approved. There would be no problem getting it approved I'm sure if that's the only option.
Mattias's suggestion to request globus exposes this header makes sense, could you request this so it may get fixed one day.
New items or things I missed first time:
(i) Can you parallelize the make? make %{?_smp_mflags}
(ii) Source code does not match. Your instructions say to use tar -czvf dpm-dsi-1.8.2.tar.gz dpm-dsi-1.8.2 however your .src.rpm contains a misnamed tar file only $ file ../SOURCES/dpm-dsi-1.8.2.tar.gz ../SOURCES/dpm-dsi-1.8.2.tar.gz: POSIX tar archive (GNU)
Moreover when I compare what is checkout vs what is in the tar ball they are different. $ diff --brief -r dpm-dsi-1.8.2 ../SOURCES/dpm-dsi-1.8.2 Only in ../SOURCES/dpm-dsi-1.8.2: config.status Only in ../SOURCES/dpm-dsi-1.8.2: Makefile i.e these files are only in the .src.rpm and not in the checkout.
(iii) Redundant files %doc LICENSE RELEASE-NOTES are not needed in devel since it can't be installed without the main package.
(iv) dpm-dsi-devel should probably Require
(v) Reading the init.d script if [ `uname -m` = "x86_64" ]; then LD_LIBRARY_PATH=/opt/glite/lib64:/opt/lcg/lib64:$GLOBUS_LOCATION/lib else LD_LIBRARY_PATH=/opt/glite/lib:/opt/lcg/lib:$GLOBUS_LOCATION/lib fi export LD_LIBRARY_PATH
The /opt directories have no place on FHS system, would better to junk it or at least case it so does not get used.
Everything else from comment #2 is good.
Other wise looking good.
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 #10 from Ricardo Rocha rocha.porto@gmail.com 2011-11-04 11:23:35 EDT --- (In reply to comment #9)
Ricardo, If possible can you at least take the newer file from globus.org since is after globus switched to the apache license.
Otherwise this package becomes 'ASL2.0 and Globus' where globus is now the old defunct Globus license which is today not fedora approved. There would be no problem getting it approved I'm sure if that's the only option.
I had to also include an additional header - globus_gsi_gss_constants.h - which was commented in the copied file of dpm-dsi (and not in the original).
I searched for it first and couldn't find a globus package providing. Guess i'll need to ask for this way to come with globus too.
Mattias's suggestion to request globus exposes this header makes sense, could you request this so it may get fixed one day.
Bug in the fedora tracker or globus?
New items or things I missed first time:
(i) Can you parallelize the make? make %{?_smp_mflags}
Done.
(ii) Source code does not match. Your instructions say to use tar -czvf dpm-dsi-1.8.2.tar.gz dpm-dsi-1.8.2 however your .src.rpm contains a misnamed tar file only $ file ../SOURCES/dpm-dsi-1.8.2.tar.gz ../SOURCES/dpm-dsi-1.8.2.tar.gz: POSIX tar archive (GNU)
Moreover when I compare what is checkout vs what is in the tar ball they are different. $ diff --brief -r dpm-dsi-1.8.2 ../SOURCES/dpm-dsi-1.8.2 Only in ../SOURCES/dpm-dsi-1.8.2: config.status Only in ../SOURCES/dpm-dsi-1.8.2: Makefile i.e these files are only in the .src.rpm and not in the checkout.
The contents of the tarball it's my bad, i've fixed it.
Regarding the commands... i didn't get it, how is it misnamed?
(iii) Redundant files %doc LICENSE RELEASE-NOTES are not needed in devel since it can't be installed without the main package.
Fixed.
(iv) dpm-dsi-devel should probably Require
It Requires dpm-dsi, which Require do you mean?
(v) Reading the init.d script if [ `uname -m` = "x86_64" ]; then LD_LIBRARY_PATH=/opt/glite/lib64:/opt/lcg/lib64:$GLOBUS_LOCATION/lib else LD_LIBRARY_PATH=/opt/glite/lib:/opt/lcg/lib:$GLOBUS_LOCATION/lib fi export LD_LIBRARY_PATH
The /opt directories have no place on FHS system, would better to junk it or at least case it so does not get used.
Should i put a patch for this one in Fedora? It will stay upstream given the same package is used for the gLite installations.
Everything else from comment #2 is good.
Other wise looking good.
I'll wait for your comments on the items above, and will provide a new version just after.
Thanks.
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 #11 from Ricardo Rocha rocha.porto@gmail.com 2011-11-07 12:06:30 EST --- Hi.
Here are updated spec and source rpm. 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-2.src.rpm
All fixed apart from:
* LD_LIBRARY_PATH (would i be able to leave this one? it simplifies life for a couple more months, or i can use a patch in the Fedora packaging)
* no-documentation in dpm-dsi-devel: this was introduced by removing the %doc from that rpm. It depends on dpm-dsi, so if i understood correctly the %doc is not needed - i must be doing something strange to still get it
Thanks.
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 #12 from Ricardo Rocha rocha.porto@gmail.com 2011-11-08 09:26:41 EST --- First go at informal reviews of other packages: https://bugzilla.redhat.com/show_bug.cgi?id=751820
stop quickly. I'll have a look at another one.
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 #13 from Ricardo Rocha rocha.porto@gmail.com 2011-11-08 11:51:00 EST --- Pointer to an informal review (for sponshorship): https://bugzilla.redhat.com/show_bug.cgi?id=751652#c1
Package is rippit.
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 #14 from Ricardo Rocha rocha.porto@gmail.com 2011-11-08 13:13:56 EST --- Another information review (for sponsorship): https://bugzilla.redhat.com/show_bug.cgi?id=751820#c3
Package is pius.
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
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #15 from Steve Traylen steve.traylen@cern.ch 2011-11-08 13:44:59 EST --- Your 3 reviews look good and thorough. Thankyou.
(In reply to comment #11)
Hi.
Here are updated spec and source rpm. 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-2.src.rpm
All fixed apart from:
- LD_LIBRARY_PATH (would i be able to leave this one? it simplifies life for a
couple more months, or i can use a patch in the Fedora packaging)
As discussed offline wrapping these statements with
if [ -d /opt/lcg/lib ] ; then do what ever then I am happy.
better still is a patch in the .spec file.
Please do this before you import.
- no-documentation in dpm-dsi-devel: this was introduced by removing the %doc
from that rpm. It depends on dpm-dsi, so if i understood correctly the %doc is not needed - i must be doing something strange to still get it
This is fine, it's only a warning and is negated by the fact you pull in the main package. It's mainly a warning for you to check if there is any sensible documentation that should have included.
Sources now match:
$ diff -r --brief dpm-dsi-1.8.2 ../SPECS/dpm-dsi-1.8.2/
I think we are there.
PACKAGE APPROVED
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
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|177841(FE-NEEDSPONSOR) |
--- Comment #16 from Steve Traylen steve.traylen@cern.ch 2011-11-08 13:55:10 EST --- You should now able to move onto:
http://fedoraproject.org/wiki/Package_SCM_admin_requests
Welcome.
Steve.
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 #17 from Ricardo Rocha rocha.porto@gmail.com 2011-11-09 07:26:20 EST --- I've added a patch as suggested (for the init script check), and have a new src rpm and spec: http://rocha.web.cern.ch/rocha/fedora/dpm-dsi.spec http://rocha.web.cern.ch/rocha/fedora/dpm-dsi-1.8.2-3.src.rpm
Just one last question before i import. Trying a serious of scratch builds it looks ok for EL5 and F16, but it fails in ppc for EL6, apparently due to missing globus headers: https://koji.fedoraproject.org/koji/taskinfo?taskID=3499253
the others are: https://koji.fedoraproject.org/koji/taskinfo?taskID=3499380 https://koji.fedoraproject.org/koji/taskinfo?taskID=3499390
Should i keep going with EL5 and F16 and leave EL6 for later?
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 #18 from Steve Traylen steve.traylen@cern.ch 2011-11-09 07:37:26 EST --- You should open a bug on the missing header against the globus package in question if there is not already one.
To your spec file add something like
# missing globus_config.h # http://bugzilla.redhat...... %{?el6:ExcludeArch: ppc64}
or what ever it is, obviously bump and rebuild once that bug is fixed.
Steve.
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 #19 from Ricardo Rocha rocha.porto@gmail.com 2011-11-09 08:20:10 EST --- I think i found the bug, it's in the dpm-dsi build.
It only appears for ppc64, which is not built in the koji builds above for dist-5E-epel and F16 (only ppc).
I'll provide a fix for this before importing.
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 #20 from Steve Traylen steve.traylen@cern.ch 2011-11-09 08:37:01 EST --- (In reply to comment #19)
I think i found the bug, it's in the dpm-dsi build.
Good
And a lesson for myself , it's a good idea during to review to do or ask for a link to a koji scratch build.
Please can you do so with your other packages. When I submit items I normally do it for the extreme ends of the range of OS releases I plan to support.
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 #21 from Ricardo Rocha rocha.porto@gmail.com 2011-11-09 08:47:29 EST --- Ok. I had done for EL5 only, which covers the two primary archs.
A simple koji scratch build will build in different archs dependending on the target, should i force more archs than what it picks up as default?
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 #22 from Steve Traylen steve.traylen@cern.ch 2011-11-09 08:53:55 EST --- A scratch build will build on all targets , (the source rpm is generated on a random target)
If that's not the case it's something I don't do often.
I normall
fedpkg srpm
koji build dist-6E-epel --scratch ../SRPMS/dpm-dsi.....src.rpm
in fact since then you can scratch build before pushing.
Steve.
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 #23 from Ricardo Rocha rocha.porto@gmail.com 2011-11-09 08:58:14 EST --- Thanks, up to now i was using rpmbuild -bs instead of fedpkg srpm (but that'll change soon).
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 #24 from Ricardo Rocha rocha.porto@gmail.com 2011-11-09 09:28:25 EST --- Here it goes:
http://rocha.web.cern.ch/rocha/fedora/dpm-dsi.spec http://rocha.web.cern.ch/rocha/fedora/dpm-dsi-1.8.2-4.src.rpm
And koji scratch builds: https://koji.fedoraproject.org/koji/taskinfo?taskID=3499645 (dist-5E-epel) https://koji.fedoraproject.org/koji/taskinfo?taskID=3499641 (dist-6E-epel) https://koji.fedoraproject.org/koji/taskinfo?taskID=3499649 (f16)
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 #25 from Ricardo Rocha rocha.porto@gmail.com 2011-11-09 10:18:40 EST --- New Package SCM Request ======================= Package Name: dpm-dsi Short Description: Disk Pool Manager (DPM) plugin for the Globus GridFTP server Owners: rocha Branches: f16 el5 el6 InitialCC:
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
Ricardo Rocha rocha.porto@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
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 #26 from Jon Ciesla limb@jcomserv.net 2011-11-09 10:34:46 EST --- Git done (by process-git-requests).
Steve, please take ownership of review BZs. Thanks!
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
Steve Traylen steve.traylen@cern.ch changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |steve.traylen@cern.ch
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 #27 from Ricardo Rocha rocha.porto@gmail.com 2011-11-09 12:27:46 EST --- I've pushed to git (master and the 3 branches), and 'fedpkg build' on all of them (success).
I guess i can push the update to bodhi now? https://fedoraproject.org/wiki/PackageMaintainers/Join#Submit_Package_as_Upd...
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 #28 from Steve Traylen steve.traylen@cern.ch 2011-11-09 13:22:26 EST --- I don't think its a requirement but I and many people do a
git tag $(fedpkg verrel) git push --tags
on each branch, I expect that "fedpkg tag" and "fedpkg push" do something similar though I have never tried. It makes it easier for someone browsing gitweb basically.
Yes bodhi is next either
* via the bodhi web interface * The bodhi-client itself. * fedpkg update (which presumably calls bodhi client)
You should include this bug in the list of fixed bugs when submitting.
Steve.
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 #29 from Mattias Ellert mattias.ellert@fysast.uu.se 2011-11-10 05:18:56 EST --- (In reply to comment #17)
Just one last question before i import. Trying a serious of scratch builds it looks ok for EL5 and F16, but it fails in ppc for EL6, apparently due to missing globus headers: https://koji.fedoraproject.org/koji/taskinfo?taskID=3499253
Instead of adding a patch for this you could do:
make lib_m=${_lib} ....
in the specfile. This will work for all 64 bit architectures. Currently with your patch, the makefile handles x86_64 and ppc64, which is sufficient for primary architecture in Fedora and EPEL, but it will not work when someone tries to do a build for a secondary Fedora 64 bit architecture like x390x and sperc64.
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 #30 from Ricardo Rocha rocha.porto@gmail.com 2011-11-10 05:55:44 EST --- Hi Mattias.
Is it ok if i put that for the next update?
I've started with the bodhi update with the current version. Should be fine no?
Thanks.
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 #31 from Mattias Ellert mattias.ellert@fysast.uu.se 2011-11-10 07:09:59 EST --- Sure, the builds for the secondary architectures are way behind anyway at the moment, so they will probably not try to build this package any time soon. But eventually they will get to it.
Mattias
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 #32 from Steve Traylen steve.traylen@cern.ch 2011-11-10 07:20:47 EST --- (In reply to comment #30)
I've started with the bodhi update with the current version. Should be fine no?
Just for information you can edit an existing bodhi update with a new package... Unless it's in the 1 day or so window of being pushed to testing. Once there you can update.
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
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
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 #33 from Fedora Update System updates@fedoraproject.org 2011-11-11 08:27:58 EST --- dpm-dsi-1.8.2-4.el5 has been submitted as an update for Fedora EPEL 5. https://admin.fedoraproject.org/updates/dpm-dsi-1.8.2-4.el5
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 #34 from Fedora Update System updates@fedoraproject.org 2011-11-11 08:31:16 EST --- dpm-dsi-1.8.2-4.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/dpm-dsi-1.8.2-4.el6
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 #35 from Fedora Update System updates@fedoraproject.org 2011-11-11 08:31:24 EST --- dpm-dsi-1.8.2-4.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/dpm-dsi-1.8.2-4.fc16
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
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #36 from Fedora Update System updates@fedoraproject.org 2011-11-11 14:55:15 EST --- dpm-dsi-1.8.2-4.el5 has been pushed to the Fedora EPEL 5 testing repository.
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 #37 from Ricardo Rocha rocha.porto@gmail.com 2011-11-14 03:25:27 EST --- EL5/EL6 look ok, but there seems to be a dependency missing in FC16/64.
https://admin.fedoraproject.org/updates/dpm-dsi-1.8.2-4.fc16 """ AutoQA: depcheck test FAILED on x86_64. Result log: http://autoqa.fedoraproject.org/results/231613-autotest/virt06.qa.fedoraproj... /dpm-dsi-1.8.2-4.fc16.html (results are informative only) """
Looking deeper: http://autoqa.fedoraproject.org/results/231613-autotest/virt06.qa.fedoraproj... """ SKIPBROKEN: dpm-dsi-1.8.2-4.fc16.i686 from pending has depsolving problems SKIPBROKEN: --> Package: dpm-dsi-1.8.2-4.fc16.i686 (pending) --> Requires: globus-gridftp-server-progs(x86-32) [view ยป] ""
It seems to try to build both the 64 and 32 versions, meaning it should also depend on the globus(32) libraries. Probably removing the ${_isa} from the dependency will do the trick, i'll have a look.
I wonder why it does not fail in EL5/EL6? Anything obvious changed?
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 #38 from Steve Traylen steve.traylen@cern.ch 2011-11-14 16:31:22 EST --- Did a bit more digging.
So 32 bit packages make it into the 64 repository if they end in a '-devel'. Of course they pull in all their dependencies as well. So when
globus-gridftp-server-devel.i686 is selected it pulls in globus-gridftp-server.i686 but not globus-gridftp-server-progs.i686.
So that's why its missing but as mentioned why would you ever want this "progs" package at 32 bit on a 64 bit machine.
Drop the %{_isa} tag and it should go way.
why no epel6 error, simple autoqa is not running :-) and epel5 %{?_isa} is "" anyway.
Steve.
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 #39 from Fedora Update System updates@fedoraproject.org 2011-11-15 05:43:03 EST --- dpm-dsi-1.8.2-5.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/dpm-dsi-1.8.2-5.el6
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 #40 from Fedora Update System updates@fedoraproject.org 2011-11-15 05:43:12 EST --- dpm-dsi-1.8.2-5.el5 has been submitted as an update for Fedora EPEL 5. https://admin.fedoraproject.org/updates/dpm-dsi-1.8.2-5.el5
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 #41 from Fedora Update System updates@fedoraproject.org 2011-11-15 05:43:21 EST --- dpm-dsi-1.8.2-5.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/dpm-dsi-1.8.2-5.fc16
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 #42 from Ricardo Rocha rocha.porto@gmail.com 2011-11-15 05:47:41 EST --- As you see above i've pushed an update with the fix for the globus-gridftp-server-progs dependency.
Let's see how it goes.
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 #43 from Steve Traylen steve.traylen@cern.ch 2011-11-16 14:15:03 EST --- Ricardo, It woud be great if you update
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 #44 from Steve Traylen steve.traylen@cern.ch 2011-11-16 14:15:38 EST --- Ricardo, It woud be great if you update http://fedoraproject.org/wiki/SIGs/Grid_Computing with some of these packages
Steve.
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 #45 from Ricardo Rocha rocha.porto@gmail.com 2011-11-16 14:35:38 EST --- Done.
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
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Fixed In Version| |dpm-dsi-1.8.2-5.fc16 Resolution| |ERRATA Last Closed| |2011-11-24 21:11:02
--- Comment #46 from Fedora Update System updates@fedoraproject.org 2011-11-24 21:11:02 EST --- dpm-dsi-1.8.2-5.fc16 has been pushed to the Fedora 16 stable repository.
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
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version|dpm-dsi-1.8.2-5.fc16 |dpm-dsi-1.8.2-5.el5
--- Comment #47 from Fedora Update System updates@fedoraproject.org 2011-11-30 17:36:48 EST --- dpm-dsi-1.8.2-5.el5 has been pushed to the Fedora EPEL 5 stable repository.
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
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version|dpm-dsi-1.8.2-5.el5 |dpm-dsi-1.8.2-5.el6
--- Comment #48 from Fedora Update System updates@fedoraproject.org 2011-11-30 17:36:59 EST --- dpm-dsi-1.8.2-5.el6 has been pushed to the Fedora EPEL 6 stable repository.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla