Bug 749132

Summary: Review Request: dpm-dsi - Disk Pool Manager (DPM) plugin to GridFTP
Product: [Fedora] Fedora Reporter: Ricardo Rocha <rocha.porto>
Component: Package ReviewAssignee: Steve Traylen <steve.traylen>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: mattias.ellert, notting, package-review, pahan, steve.traylen
Target Milestone: ---Flags: steve.traylen: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: dpm-dsi-1.8.2-5.el6 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-11-25 02:11:02 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Ricardo Rocha 2011-10-26 09:52:54 UTC
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.

Comment 1 Steve Traylen 2011-10-29 08:57:30 UTC
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.

Comment 2 Steve Traylen 2011-10-29 13:19:46 UTC
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?

Comment 3 Ricardo Rocha 2011-10-31 15:07:32 UTC
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.

Comment 4 Steve Traylen 2011-10-31 15:18:49 UTC
>(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.

Comment 5 Steve Traylen 2011-11-02 19:48:42 UTC
(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

Comment 6 Ricardo Rocha 2011-11-03 09:54:27 UTC
(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_openssl.h?view=markup

Ricardo

Comment 7 Mattias Ellert 2011-11-03 15:10:38 UTC
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.

Comment 8 Mattias Ellert 2011-11-03 18:25:20 UTC
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.

Comment 9 Steve Traylen 2011-11-03 20:04:21 UTC
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.

Comment 10 Ricardo Rocha 2011-11-04 15:23:35 UTC
(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.

Comment 11 Ricardo Rocha 2011-11-07 17:06:30 UTC
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.

Comment 12 Ricardo Rocha 2011-11-08 14:26:41 UTC
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.

Comment 13 Ricardo Rocha 2011-11-08 16:51:00 UTC
Pointer to an informal review (for sponshorship):
https://bugzilla.redhat.com/show_bug.cgi?id=751652#c1

Package is rippit.

Comment 14 Ricardo Rocha 2011-11-08 18:13:56 UTC
Another information review (for sponsorship):
https://bugzilla.redhat.com/show_bug.cgi?id=751820#c3

Package is pius.

Comment 15 Steve Traylen 2011-11-08 18:44:59 UTC
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

Comment 16 Steve Traylen 2011-11-08 18:55:10 UTC
You should now able to move onto:

http://fedoraproject.org/wiki/Package_SCM_admin_requests

Welcome.

Steve.

Comment 17 Ricardo Rocha 2011-11-09 12:26:20 UTC
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?

Comment 18 Steve Traylen 2011-11-09 12:37:26 UTC
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.

Comment 19 Ricardo Rocha 2011-11-09 13:20:10 UTC
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.

Comment 20 Steve Traylen 2011-11-09 13:37:01 UTC
(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.

Comment 21 Ricardo Rocha 2011-11-09 13:47:29 UTC
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?

Comment 22 Steve Traylen 2011-11-09 13:53:55 UTC
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.

Comment 23 Ricardo Rocha 2011-11-09 13:58:14 UTC
Thanks, up to now i was using rpmbuild -bs instead of fedpkg srpm (but that'll change soon).

Comment 25 Ricardo Rocha 2011-11-09 15:18:40 UTC
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:

Comment 26 Gwyn Ciesla 2011-11-09 15:34:46 UTC
Git done (by process-git-requests).

Steve, please take ownership of review BZs.  Thanks!

Comment 27 Ricardo Rocha 2011-11-09 17:27:46 UTC
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_Update_in_Bodhi

Comment 28 Steve Traylen 2011-11-09 18:22:26 UTC
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.

Comment 29 Mattias Ellert 2011-11-10 10:18:56 UTC
(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.

Comment 30 Ricardo Rocha 2011-11-10 10:55:44 UTC
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.

Comment 31 Mattias Ellert 2011-11-10 12:09:59 UTC
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

Comment 32 Steve Traylen 2011-11-10 12:20:47 UTC
(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.

Comment 33 Fedora Update System 2011-11-11 13:27:58 UTC
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

Comment 34 Fedora Update System 2011-11-11 13:31:16 UTC
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

Comment 35 Fedora Update System 2011-11-11 13:31:24 UTC
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

Comment 36 Fedora Update System 2011-11-11 19:55:15 UTC
dpm-dsi-1.8.2-4.el5 has been pushed to the Fedora EPEL 5 testing repository.

Comment 37 Ricardo Rocha 2011-11-14 08:25:27 UTC
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.fedoraproject.org/depcheck/results /dpm-dsi-1.8.2-4.fc16.html (results are informative only)
"""

Looking deeper:
http://autoqa.fedoraproject.org/results/231613-autotest/virt06.qa.fedoraproject.org/depcheck/results/dpm-dsi-1.8.2-4.fc16.html
"""
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?

Comment 38 Steve Traylen 2011-11-14 21:31:22 UTC
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.

Comment 39 Fedora Update System 2011-11-15 10:43:03 UTC
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

Comment 40 Fedora Update System 2011-11-15 10:43:12 UTC
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

Comment 41 Fedora Update System 2011-11-15 10:43:21 UTC
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

Comment 42 Ricardo Rocha 2011-11-15 10:47:41 UTC
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.

Comment 43 Steve Traylen 2011-11-16 19:15:03 UTC
Ricardo,
 It woud be great if you update

Comment 44 Steve Traylen 2011-11-16 19:15:38 UTC
Ricardo,
 It woud be great if you update 
http://fedoraproject.org/wiki/SIGs/Grid_Computing
with some of these packages

Steve.

Comment 45 Ricardo Rocha 2011-11-16 19:35:38 UTC
Done.

Comment 46 Fedora Update System 2011-11-25 02:11:02 UTC
dpm-dsi-1.8.2-5.fc16 has been pushed to the Fedora 16 stable repository.

Comment 47 Fedora Update System 2011-11-30 22:36:48 UTC
dpm-dsi-1.8.2-5.el5 has been pushed to the Fedora EPEL 5 stable repository.

Comment 48 Fedora Update System 2011-11-30 22:36:59 UTC
dpm-dsi-1.8.2-5.el6 has been pushed to the Fedora EPEL 6 stable repository.