Bug 446390

You shouldn't need BuildRequires: openais, the BR on openais-devel will pull
that in everytime. :)

A few additional minor items:

%postun -n cman
/sbin/ldconfig > /dev/null

can be changed into

%postun -n cman -p /sbin/ldconfig

Please avoid using file requires, except where absolutely necessary. Instead of
Requires: net-tools mount /sbin/findfs /bin/bash
 use
Requires: net-tools mount e2fsprogs
(bash is already a Requires, two lines above)

Please show an updated SPEC/SRPM and I will finish the review. :)

(In reply to comment #1)
> You shouldn't need BuildRequires: openais, the BR on openais-devel will pull
> that in everytime. :)

true that.. done.

(In reply to comment #2)
> A few additional minor items:
> 
> %postun -n cman
> /sbin/ldconfig > /dev/null
> 
> can be changed into
> 
> %postun -n cman -p /sbin/ldconfig

I have done the postun section on your suggestion but i have a question:
does this apply also to %post? google is not helping me here to find references
to this format and I don't like to do blind changes (even when i know they are
coming from trusted sources ;))
What does -p mean?

> Please avoid using file requires, except where absolutely necessary. Instead of
> Requires: net-tools mount /sbin/findfs /bin/bash
>  use
> Requires: net-tools mount e2fsprogs
> (bash is already a Requires, two lines above)

done.

> 
> Please show an updated SPEC/SRPM and I will finish the review. :)

Spec URL: http://bassotto.seabone.net/~fabbione/cluster.spec
65f4fe55f70bb14a85d83c5232d8a0f7  cluster.spec (just in case there is proxy on
the way since the URL is the same)
SRPM URL: http://bassotto.seabone.net/~fabbione/cluster-2.99.01-2.fc10.src.rpm

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-d0dbcb7eec27622a21df280009c5b089b02f5bef
has some description of -p, but essentially, if you only have a single command
to execute in a %post/%postun scriptlet, you can call it with -p and it will
automatically resolve the dependencies for it.

Of course, this doesn't appear to be cleanly documented anywhere, but I assure
you, it works. :)

(In reply to comment #5)
>
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-d0dbcb7eec27622a21df280009c5b089b02f5bef
> has some description of -p, but essentially, if you only have a single command
> to execute in a %post/%postun scriptlet, you can call it with -p and it will
> automatically resolve the dependencies for it.
> 
> Of course, this doesn't appear to be cleanly documented anywhere, but I assure
> you, it works. :)

I absolutely believe you it does.. i am just being extra paranoid to build solid
references for the future.

Thanks
Fabio

Since you're replacing existing packages, please be sure to coordinate with
their maintainers and properly End-Of-Life them, according to:
http://fedoraproject.org/wiki/PackageMaintainers/PackageEndOfLife

Now, for the review:

Good:

- rpmlint checks return:
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/qdiskd
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/qdiskd
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/scsi_reserve
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/scsi_reserve
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/cman
cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/cman
rgmanager.x86_64: W: service-default-enabled /etc/rc.d/init.d/rgmanager
rgmanager.x86_64: W: service-default-enabled /etc/rc.d/init.d/rgmanager
gfs2-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/gfs2
gfs2-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/gfs2
gfs2-utils.x86_64: W: incoherent-init-script-name gfs2

The incoherent-init-script-name one is safe to ignore, but you should consider
whether all of those services should be enabled by default.

- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2 and LGPLv2) OK, text in %doc, matches source

You should correct the license tag for the rgmanager subpackage, as it is
GPLv2+. Just add:

License: GPLv2+ into the rgmanager subpackage definition.

- spec file legible, in am. english
- source matches upstream (50ca482ccd4d2ee525a982c7cf08ac1100279251)
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files
- post/postun ldconfig ok
- devel requires base package n-v-r 

Aside from the License tag and the initscripts, everything is fine. Show me a
fixed SRPM and I will approve and sponsor. Good work!

(In reply to comment #7)
> Since you're replacing existing packages, please be sure to coordinate with
> their maintainers and properly End-Of-Life them, according to:
> http://fedoraproject.org/wiki/PackageMaintainers/PackageEndOfLife

All the maintainers of the binary packages I am replacing are in CC in this bug
and we all agreed in advance on this step. I will make sure both Chris and
Steven will obsolete cman/gfs2-utils/rgmanager source packages for rawhide/F10.

Older branches will stay. Once i will be able to access them, they will receive
proper updates as well.

> - rpmlint checks return:
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/qdiskd
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/qdiskd
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/scsi_reserve
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/scsi_reserve
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/cman
> cman.x86_64: W: service-default-enabled /etc/rc.d/init.d/cman
> rgmanager.x86_64: W: service-default-enabled /etc/rc.d/init.d/rgmanager
> rgmanager.x86_64: W: service-default-enabled /etc/rc.d/init.d/rgmanager
> gfs2-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/gfs2
> gfs2-utils.x86_64: W: service-default-enabled /etc/rc.d/init.d/gfs2

So this is something i have been pondering myself for a few days now. All the
old .spec files and init.d files had services enabled by default, without
declaring dependencies etc.

These new ones are (in one way or another) all dependent on cman to start
properly and that happens only if cman is configured correctly (otherwise it's a
bug in the init scripts that should be addressed upstream _immediatly_).

cman is not autoconfigured from the packages since there is no such thing as
"cluster default config that will work everywhere" so basically having those
scripts enabled or disabled by default makes no difference.

enabling the services by default will (IMHO) avoid extra steps for the users
given the failsafe embedded in the scripts.

> gfs2-utils.x86_64: W: incoherent-init-script-name gfs2
> 
> The incoherent-init-script-name one is safe to ignore, but you should consider
> whether all of those services should be enabled by default.

Ok thanks. I did confirm this one also checking the recent gfs2-utils review.

> You should correct the license tag for the rgmanager subpackage, as it is
> GPLv2+. Just add:
> 
> License: GPLv2+ into the rgmanager subpackage definition.

Done. On the licensing side, we are working upstream to clean it up and also
provide a proper set of files to land in the %doc areas (usual
COPYING/COPYRIGHT/LICENCES shebang for the whole stack)

> Aside from the License tag and the initscripts, everything is fine. Show me a
> fixed SRPM and I will approve and sponsor.

Spec URL: http://bassotto.seabone.net/~fabbione/cluster.spec
dd1026852f948f97061f5573b27c5085  cluster.spec
SRPM URL: http://bassotto.seabone.net/~fabbione/cluster-2.99.01-3.fc10.src.rpm

> Good work!

Thanks :)

Fabio

*** Bug 225646 has been marked as a duplicate of this bug. ***

> These new ones are (in one way or another) all dependent on cman to start
> properly and that happens only if cman is configured correctly (otherwise 
> it's a bug in the init scripts that should be addressed upstream 
> _immediatly_).

But, since cman is not autoconfigured, does it really make sense for those
initscripts to be enabled by default? It seems like users configuring these
applications can also turn on these services. I'll leave that decision up to you.

The only other thing remaining is that there is a typo in the Summary for
rgmanager: Open Source HA Resource Group Failove for Red Hat Cluster

Should be Open Source HA Resource Group Failover for Red Hat Cluster

Package approved. :)

(In reply to comment #10)
> > These new ones are (in one way or another) all dependent on cman to start
> > properly and that happens only if cman is configured correctly (otherwise 
> > it's a bug in the init scripts that should be addressed upstream 
> > _immediatly_).
> 
> But, since cman is not autoconfigured, does it really make sense for those
> initscripts to be enabled by default? It seems like users configuring these
> applications can also turn on these services. I'll leave that decision up to you.

I guess it makes no real difference to me. I don't have strong opinions either
way. I will disable them in the next upstream release that I am planning to do
next monday.

> 
> The only other thing remaining is that there is a typo in the Summary for
> rgmanager: Open Source HA Resource Group Failove for Red Hat Cluster
> 
> Should be Open Source HA Resource Group Failover for Red Hat Cluster

Done.

SRPMS: http://bassotto.seabone.net/~fabbione/cluster-2.99.01-4.fc10.src.rpm
SPEC: http://bassotto.seabone.net/~fabbione/cluster.spec
3da6d1bb600a1bff02442e652c2ac476  SPECS/cluster.spec

> Package approved. :)

great.

Thanks
Fabio

New Package CVS Request
=======================
Package Name: cluster
Short Description: Red Hat Cluster
Owners: fabbione swhiteho cfeist lon
Branches: F-10
InitialCC: fabbione
Cvsextras Commits: no

This package will be only in F-10 and higher. Not sure if that requires a branch
to be specified at all at this point in time.

Regarding cvsextra, since this is my first package, i would like to have full
control over it and relax my paranoia once i am more used to the Fedora way of
cross handling packages between maintainers. As upstream, we are also putting a
lot of effort to include distro patches in the main tree. I don't want CVS to
become a gateway between community patches and upstream nor a place where to
fight if a patch should be in or out. I also assume it is a flag its possible to
change in time..

Thanks
Fabio

The 'devel' branch (which is default) will much later in this development cycle
be branched off to become the F-10 branch. 

You are of course free to change cvsextras commits later. It's quite rare for
anyone non closely associated with a particular package to change it however.
Allowing cvsextras commits does allow other maintainers to make changes in cases
where there is a simple bug, or where there is something like a broken
dependency where the package simply needs to be rebuilt. Do consider opening it
up as soon as you are comfortable. 

cvs done.

Hi Kevin,

thanks a lot for your work. Very much appreciated.

Fabio

(In reply to comment #12)
> New Package CVS Request
> =======================
> Package Name: cluster
> Short Description: Red Hat Cluster
> Owners: fabbione swhiteho cfeist lon
> Branches: F-10

Hi Fabio,

please consider pushing this also into F-9 -- there shouldn't be any problems
with it now, are there any? (I mean F-9 and current rawhide is still almost the
same thing now).

Thanks in advance,
Milos

This package will not go in F-9. F-9 will get updates soon but from the stable2
branch (version 2.03.xx) once i have done with testing.

Fabio