Bug 191200

Initial question to think about while I review it is would this package be
better suited for Extras?  What reasoning is there for including it in core?

Bad:
- Buildroot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
- Prereq should not be used.  Use Requires(pre): instead.
- Init file is not a %config file.  Don't mark it as such.


Indifferent:
- Mixuse of %{_foo} style variable and $FOO variable.  One should be used.

Questions:
- Why exclusive arch?  Please list in spec file.
- Why define _exec_prefix?  _exec_prefix is an alias of _prefix, which is /usr.
- --with-user= --with-group=  Does that even work, non defined users?

Also, I can't actually build this package because the deps aren't available in
rawhide.

Error: Missing Dependency: /lib/modules/2.6.16-1.2096_FC5 is needed by package
dlm-kernel
Error: Missing Dependency: kernel = 2.6.16-1.2096_FC5 is needed by package
dlm-kernel
Error: Missing Dependency: /lib/modules/2.6.16-1.2096_FC5 is needed by package
cman-kernel
Error: Missing Dependency: kernel = 2.6.16-1.2096_FC5 is needed by package
cman-kernel

Core vs extras:  Before too long, some people will need this package in their
initrd to boot their system (with their root filesystem on a clustered logical
volume with a clustered filesystem) and the installer will need to support this
in some fashion...  (These people will also want the package on any rescue CD.)

This package is a member of the set of clustering packages, and the decision was
taken to put the rest of them in core not extras (not least because some people
will already be trying to have system filesystems using them).  Most people
using gfs need to use the lvm2-cluster package *before* the gfs package, hence
the complaints I'm getting from people about it missing from fc5.

The dependencies for fc6 will be sorted out over the next few days in parallel
with this (should work with fc5) - we want this package to be ready in fc6 at
the same time.

Also, 
Requires(preun): chkconfig
Requires(post): chkconfig

Looking closer, you have some %pre and %preun tasks commented out.   I think
these tasks were what was driving the Requires(pre) of lvm2 and device-mapper. 
Since these are commented out, you should also comment out or remove the
Requires(pre) on these things.  I see only a preun-req on chkconfig, and a post
require on chkconfig.

Can't build on FC5 either, as you have a requirement of device-mapper >=
1.02.07, however the latest package in FC5+updates is device-mapper-1.02.02-3.2.

Updated to lvm2 2.02.06 (built today).

Changed BuildRoot.
Removed PreReq.
Removed %config.

Added Requires chkconfig for post & preun.
Added Requires lvm2 & device-mapper for preun.

Removed the commented out post line that enabled clustering.  (In Fedora, if you
install the package it doesn't enable clustering by default: you must run
lvmconf explicitly when you want to start using clustering.)

Reactivated the commented out line in preun to disable clustering: if you *did*
enable the package's functionality by running lvmconf --enable-cluster, we
*should* turn it off before removing the package.

Added missing libsepol-devel buildrequires.

Where are these changes made?  I can't see them in internal CVS, and the link in
the first comment is no longer valid.  Can you re-post the spec file and an
updated srpm for which to test?

--with-group= --with-user=

If unset like that, 'make install' doesn't explicitly set the group or owner on
any files.  (People complained they couldn't build it as a non-root user, so I
added that workaround.  %defattr sets the user & group instead.)

ExclusiveArch restricts the package to the archs for which the kernel clustering
components are available.

From google, http://www.redhat.com/archives/rpm-list/2002-July/msg00121.html
says $RPM_BUILD_ROOT is preferred to %{buildroot}.  Everything else already uses
the %{} form.

The exec_prefix setting does look pointless.  It probably got copied from the
device-mapper package where it is set to '/'.

Ok, can you please mark IN the specfile why it is exclusivearch?  It helps if
you give the package over to somebody, or somebody else has to mess with it. 
Out of 'norm' things like this should be commented.

That email is rather old, as in 4 years old.  The Fedora Packaging Guidelines
recommend that a singular alias method is used, including RPM_BUILD_ROOT.  This
is a SHOULD, not a must.

Also, where can I look at this updated spec file?

http://cvs.devel.redhat.com/cgi-bin/cvsweb.cgi/~checkout~/rpms/lvm2-cluster/devel/lvm2-cluster.spec?rev=1.21;content-type=text%2Fplain

(Still waiting for updated dlm-kernel package.)

Looking better.  Do you have an updated srpm that I can try building in mock?

This was built into rawhide.  

ooops, that was gfs-cluster I think.  I was wrong, here is the missing comments:

On Tue, Jun 13, 2006 at 06:54:20PM -0400, Jesse Keating wrote:
> rpmlint is unhappy:
 
# yum install rpmlint
# man rpmlint
No manual entry for rpmlint

So the rpmlint package itself needs fixing to include a man page for
a binary it installed!  Hardly instills confidence.
/usr/share/doc has nothing useful for it either.

> E: lvm2-cluster non-standard-executable-perm /usr/sbin/clvmd 0555

What's rpmlint complaining about?  It's got the executable bits and it can't be
written to by a non-root user.  Some would argue 0111 would be better, but this
is a distribution so there's little to gain from a security-by-obscurity
argument as it's trivial for a user to get hold of a copy of the binary from
elsewhere.

- Ignoring.  (rpmlint bug?)

> E: lvm2-cluster postin-without-ldconfig /usr/lib/liblvm2clusterlock.so.2.02
> E: lvm2-cluster library-without-ldconfig-postun
/usr/lib/liblvm2clusterlock.so.2.02

OK: the packaging installation process doesn't run ldconfig automatically yet
so it has to be included in every spec file that handles shared libraries.

However, other packages look to have '%postun -p /sbin/ldconfig' and I've
googled and searched the Fedora wiki and the new online book you mentioned, but
as usual, I can't find documentation for what I need to know, viz. what '-p'
does and whether you're meant to use it if there are other commands to run in
the same section.

For safety, opted for:
  %post
  /sbin/chkconfig --add clvmd
  /sbin/ldconfig
 
  %postun -p /sbin/ldconfig

> E: lvm2-cluster non-standard-executable-perm
/usr/lib/liblvm2clusterlock.so.2.02 0555

Puzzling: I thought linux wanted both the read and execute bits to be set these days
on shared objects, not just the read bit (which is all that's required at the kernel
level).

- Ignoring.  (rpmlint bug?)

> W: lvm2-cluster devel-file-in-non-devel-package /usr/lib/liblvm2clusterlock.so

Seems overkill to create a lvm2-cluster-devel package containing just 
one symlink?  I don't spot other packages with shared libraries doing 
that.

- Ignoring.

> W: lvm2-cluster no-reload-entry /etc/rc.d/init.d/clvmd

The daemon doesn't support 'reload', so it would be misleading
to offer that option in the script.  Or should it give a message
saying 'unsupported'?  But I don't see other scripts doing that:
they're just silent. (e.g. mailman, spamassassin, mysqld, haldaemon).

> W: lvm2-cluster service-default-enabled /etc/rc.d/init.d/clvmd

The RHEL4->Fedora patch got dropped from the RPM.  Fixing.

> W: lvm2-cluster incoherent-init-script-name clvmd

Calling the init script 'lvm2-cluster' or renaming the package to 'clvmd' will
surely only confuse people more?

- Ignoring.

> W: lvm2-cluster-debuginfo dangling-relative-symlink
/usr/src/debug/LVM2.2.02.06/include/lvm-types.h ../lib/datastruct/lvm-types.h
> W: lvm2-cluster-debuginfo dangling-relative-symlink
/usr/src/debug/LVM2.2.02.06/include/log.h ../lib/log/log.h
> W: lvm2-cluster-debuginfo dangling-relative-symlink
/usr/src/debug/LVM2.2.02.06/include/list.h ../lib/datastruct/list.h

I've never done anything with debuginfo packages before.
Is this a bug in whatever bit of rpm generates them?

I've installed the 'lvm2' debuginfo package, and it has a similar problem.

I don't understand enough about how debuginfo packages are used to know whether
the problem is the symlink that shouldn't be there, or if it's the file at the 
end of it that shouldn't be missing.

On Wed, Jun 14, 2006 at 02:27:07PM -0400, Jesse Keating wrote:
> A standard executable should have permission set to 0755. If you get
> this
> message, it means that you have a wrong executable permissions in some
> files
> included in your package.
 
Oh!  So it prefers the owner of the executable to have write permission.
The file is owned by root so owner write is irrelevant, but it's better not
to set it IMHO as that gives out the wrong message, suggesting it's a file
other applications might want to modify: for example, editors will often warn 
the file is read-only if you try to modify it even as root.

I think that 'Error' from rpmlint should be downgraded to 'information', and
it should be inverted - warning if the owner write bit is *set* on an
executable.

> >   %postun -p /sbin/ldconfig
> All I know of -p is that it automatically figures out what the
> Requires(postun) would need.

Ah.  Does it work split line I wonder?

  %postun -p
  /sbin/ldconfig
 

> That's why its just a warning.  Although 'clvmd' is rather cryptic.

Would have been nice to have had a better name for it.
 
> Thanks, can you incorporate some of the above changes?

The ones I mentioned in the email I'd already put into lvm2-cluster-2_02_06-1_2.

Probably still missing the requires for ldconfig though.

The permissions are more generic and reference both binary files and scripts. 
Scripts you may want to edit.  Binaries probably not.  But your text editor will
most likely complain if you try to edit a binary (:

I don't know if the %post -p stuff works multi-line.

With your changes this is now accepted, but just need Bill's signoff on it, and
information about where to put it in comps.

Bill?

(In reply to comment #18)
> So the rpmlint package itself needs fixing to include a man page for
> a binary it installed!

Already there since version 0.76.

$ rpm -qdp
http://download.fedora.redhat.com/pub/fedora/linux/extras/5/i386/rpmlint-0.76-1.fc5.noarch.rpm
| grep man
/usr/share/man/man1/rpmlint.1.gz

(In reply to comment #18)
> > E: lvm2-cluster non-standard-executable-perm /usr/sbin/clvmd 0555
> 
> What's rpmlint complaining about?  It's got the executable bits and it can't be
> written to by a non-root user.  Some would argue 0111 would be better, but this
> is a distribution so there's little to gain from a security-by-obscurity
> argument as it's trivial for a user to get hold of a copy of the binary from
> elsewhere.
> 
> - Ignoring.  (rpmlint bug?)

Using perms 755 would shut rpmlint up.

> > E: lvm2-cluster postin-without-ldconfig /usr/lib/liblvm2clusterlock.so.2.02
> > E: lvm2-cluster library-without-ldconfig-postun
> /usr/lib/liblvm2clusterlock.so.2.02
> 
> OK: the packaging installation process doesn't run ldconfig automatically yet
> so it has to be included in every spec file that handles shared libraries.
> 
> However, other packages look to have '%postun -p /sbin/ldconfig' and I've
> googled and searched the Fedora wiki and the new online book you mentioned, but
> as usual, I can't find documentation for what I need to know, viz. what '-p'
> does and whether you're meant to use it if there are other commands to run in
> the same section.

The -p option specifies the script interpreter to use for the scriplet.
"%post -p /sbin/ldconfig" with an empty script is a standard idiom for running a
single program in the scriptlet without having to use a shell.

> For safety, opted for:
>   %post
>   /sbin/chkconfig --add clvmd
>   /sbin/ldconfig
>  
>   %postun -p /sbin/ldconfig

That's correct usage,

> > E: lvm2-cluster non-standard-executable-perm
> /usr/lib/liblvm2clusterlock.so.2.02 0555
> 
> Puzzling: I thought linux wanted both the read and execute bits to be set
these days
> on shared objects, not just the read bit (which is all that's required at the
kernel
> level).
> 
> - Ignoring.  (rpmlint bug?)

rpmlint is expecting mode 755 as per most other libs in /usr/lib

> > W: lvm2-cluster devel-file-in-non-devel-package /usr/lib/liblvm2clusterlock.so
> 
> Seems overkill to create a lvm2-cluster-devel package containing just 
> one symlink?  I don't spot other packages with shared libraries doing 
> that.
> 
> - Ignoring.

$ rpm -qlp xorg-x11-drv-i810-devel-1.6.0-4.i386.rpm
/usr/lib
/usr/lib/libI810XvMC.so

(that package should not be owning /usr/ilb)

> > W: lvm2-cluster-debuginfo dangling-relative-symlink
> /usr/src/debug/LVM2.2.02.06/include/lvm-types.h ../lib/datastruct/lvm-types.h
> > W: lvm2-cluster-debuginfo dangling-relative-symlink
> /usr/src/debug/LVM2.2.02.06/include/log.h ../lib/log/log.h
> > W: lvm2-cluster-debuginfo dangling-relative-symlink
> /usr/src/debug/LVM2.2.02.06/include/list.h ../lib/datastruct/list.h
> 
> I've never done anything with debuginfo packages before.
> Is this a bug in whatever bit of rpm generates them?

Yes.

> I've installed the 'lvm2' debuginfo package, and it has a similar problem.
> 
> I don't understand enough about how debuginfo packages are used to know whether
> the problem is the symlink that shouldn't be there, or if it's the file at the 
> end of it that shouldn't be missing.

I believe it's the latter.

> On Wed, Jun 14, 2006 at 02:27:07PM -0400, Jesse Keating wrote:
> > A standard executable should have permission set to 0755. If you get
> > this
> > message, it means that you have a wrong executable permissions in some
> > files
> > included in your package.
>  
> Oh!  So it prefers the owner of the executable to have write permission.
> The file is owned by root so owner write is irrelevant, but it's better not
> to set it IMHO as that gives out the wrong message, suggesting it's a file
> other applications might want to modify: for example, editors will often warn 
> the file is read-only if you try to modify it even as root.
> 
> I think that 'Error' from rpmlint should be downgraded to 'information', and
> it should be inverted - warning if the owner write bit is *set* on an
> executable.

Fair comment,

> The ones I mentioned in the email I'd already put into lvm2-cluster-2_02_06-1_2.
> 
> Probably still missing the requires for ldconfig though.

Perhaps. I can't see the spec file so I don't know.

Please address Paul's comments and make the latest spec file viewable so this
review can continue.

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

Chris -

There have been some recent requests.  Is this going to happen?  thanks.

spec file changed according requests and commited to internal CVS
http://cvs.devel.redhat.com/cgi-bin/cvsweb.cgi/~checkout~/rpms/lvm2-cluster/devel/lvm2-cluster.spec

Please continue with review, thanks.

There are still some rpmlint issues:

E: lvm2-cluster non-standard-executable-perm
/usr/lib64/liblvm2clusterlock.so.2.02 0555
E: lvm2-cluster non-standard-executable-perm /usr/sbin/clvmd 0555

These are suposed to be 0755

E: lvm2-cluster shlib-with-non-pic-code /usr/lib64/liblvm2clusterlock.so.2.02

shlib-with-non-pic-code :
The listed shared libraries contain object code that was compiled
without -fPIC. All object code in shared libraries should be
recompiled separately from the static libraries with the -fPIC option.

Another common mistake that causes this problem is linking with
``gcc -Wl,-shared'' instead of ``gcc -shared''.

W: lvm2-cluster macro-in-%changelog _libdir

This warning is because some macros could cause problems as rpm will expand them
no matter where they are.

Permissions set to 0755, macro expansion in changelog repaired.
Spec file commited to CVS - link above.

I cannot get rpmlint error shlib-with-non-pic-code (on local build rpm & brew
build), how did you get this ?

I get the shlib warning when building for x86_64.

rpmlint lvm2-cluster-2.02.06-1.4.src.rpm
W: lvm2-cluster rpm-buildroot-usage %build make DESTDIR=$RPM_BUILD_ROOT

rpmlint lvm2-cluster-2.02.06-1.4.x86_64.rpm
W: lvm2-cluster devel-file-in-non-devel-package /usr/lib64/liblvm2clusterlock.so
W: lvm2-cluster no-reload-entry /etc/rc.d/init.d/clvmd
W: lvm2-cluster incoherent-init-script-name clvmd

package built in brew (same with local x86_64 build in rawhide),
http://brewweb.devel.redhat.com/brew/getfile?taskID=141216&name=lvm2-cluster-2.02.06-1.4.x86_64.rpm

rpm -q rpmlint
rpmlint-0.77-1.fc6

Where is the problem ?

hrm, it may be the host in which I'm calling rpmlint from...

Yep it was.  All looks good now.

Where does htis need to be in comps?  

Based on comment #2 I assume that there wll be dependency on cluster packages only.

I've added lvm2-cluster to dist-fc6 as owned by mbroz.  It is too late for
Test2, however if it is a blocker for RHEL5 Beta1 you can probably get it
included there.

Please close this report when built for dist-fc6

Built into dist-fc6-HEAD.

In RHEL, the package currently gets released separately in cluster suite.