Added Chris as CC.

A few initial comments:
*  omit static libs.  If there's good reason to include, please add comment in 
specfile to justify inclusion

* IMO, not enough docs here to warrant a separate package, but those bits 
where appropriate either in main pkg or -devel.

* Why are libs placed in %{_libdir}/openais/ which forces use of ld.so.conf.d/ 
instead of simply putting in %{_libdir}?

* use %%{_initrddir} macro instead of hard-coding /etc/rc.d/init.d

* There seem to be %{_libdir}/openais/lib*.so symlinks not packaged 
(in -devel?)

*Usually* pkgs that include shared libs include in main pkg (something like):
%{libdir}/lib*.so.*

and in -devel:
%{_libdir}/lib*.so

Unowned dirs:
%{_libdir}/openais/
%{_libdir}/openais/lcrso/
%{_includedir}/openais/
%{_includedir}/openais/lcr

Thanks Rex for your review.  I've updated the spec file to address these
problems and put the patch here

http://developer.osdl.org:/dev/openais/SRPM/fix-unowned-dirs.patch

Would you review the patch and see if it resolves the problem?

I have also updated the spec file and SRPM on the web at the above addresses.

Regards

Rex I have only addressed the unowned dirs problem with comment #5.  I'll take a
look at the other issues next.  Thanks.

Addressing comment #2 point #2 on the topic of documentation, we intend in the
community to add about 75-150 man pages in the coming months.  Is this suitable
for a separate docs package?  In planning for the future, this is the motivation
behind a separate docs package.  I'm not sure how we could later add a docs
package to fedora core 6 or offer an upgrade path.

Most of these docs are "API" documentation which the user may not want.  For the
bits that are necessary for configuration and usage (openais_overview.8,
openais.conf.5) I could add it to the main pakage.

I am happy to remove the docs package and integrate them into the other
packages.  I just want to follow the guidelines as closely as possible and
thought the 150+ man pages may warrant a separate docs package.

Your comments on this topic welcome.

Addressing comment #2 point #3 on the topic of location of library dirs:

Upstream places the files in the separate openais directory.  The reason this is
done is because it may be that people want to install two competing SA Forum AIS
standard implementations (this is what openais provides) on their Fedora system
at once.  This would cause collisions if they both used /usr/lib allowing only
one implementation at a time.  The standard specifies the name of the shared
objects.  We could declare that only openais could be installed on a FC6 system,
but I think we would be more friendly if we allowed other vendors to operate on
Fedora Core 6 as well.

On the topic of ld.so.conf.d, the project uses a component model to load
services (that is what those lcrso files are).  Upstream doesn't place the lcrso
files in the /usr/lib dir since they are not generally useful shared libraries.
 Either way, ld.so.conf.d must be modified as this is how the component loader
finds its components.

First it scans the cwd, then /usr/lib then /usr/lib64, then /etc/ld.so.conf
(which may have include lines which are then scanned).

Addressing comment #3:
Do you mean by this comment that I should not list out each individual shared
object file, but instead use wildcards?  It appears from the many examples I
found that each file is listed separately but I could remove all of the separate
filenames and then use the wildcard lines.  I have no preference either way,
although it is somewhat helpful for the package maintainer (me) if the specfile
were to complain during build because of a shared object additiion/removal.

(In reply to comment #7)
> Addressing comment #2 point #2 on the topic of documentation, we intend in the
> community to add about 75-150 man pages in the coming months.  Is this suitable
> for a separate docs package?
Nope. man pages should always be part of the package, which implements what they
document. I.e. in general, API docs should be part of *-devel packages, while
user application man pages would be part of <non-devel> packages.

A separate *-doc packages would be useful for other (non-man, non-info)
documentation, e.g. html formated books.

(In reply to comment #0)
> I have followed the packaging guidelines as closely as possible.  I have also
run rpmlint (with some strange complaints about the init script).

I took a look at this package with a view to fixing the rpmlint/initscript
issues. However, I found a lot more to do here as well.

(In reply to comment #9)
> Addressing comment #3:
> Do you mean by this comment that I should not list out each individual shared
> object file, but instead use wildcards?  It appears from the many examples I
> found that each file is listed separately but I could remove all of the separate
> filenames and then use the wildcard lines.  I have no preference either way,
> although it is somewhat helpful for the package maintainer (me) if the specfile
> were to complain during build because of a shared object additiion/removal.

I agree with you here, and tend to explicitly list binaries and important shared
libs explictly. It's a personal preference thing. However, I'd be inclined to
just have:

%{_includedir}/openais/

rather than enumerating each and every include file, and you might consider
something similar for the manpages if there eventually going to be hundreds of them.

(In reply to comment #10)
> Nope. man pages should always be part of the package, which implements what they
> document. I.e. in general, API docs should be part of *-devel packages, while
> user application man pages would be part of <non-devel> packages.
> 
> A separate *-doc packages would be useful for other (non-man, non-info)
> documentation, e.g. html formated books.

I agree enirely with this.

Now, on to my suggested changes to the spec.

Much of what rpmlint was complaining about was due to the initscript, which
wasn't installed using chkconfig in %post despite the PreReq: of chkconfig being
in the spec, and the initscript itself was "enabled by default", which is
generally a no-no.

Suggestions:
* Add %post and %preun calls to chkconfig to install and remove the initscript
links properly
* Use Requires(post) and Requires(presun) style scriptlet deps instead of PreReq:
* Patch the initscript to not be enabled by default

Now for the other things I came across:
* The URL points to a tarball and not the project home page
* Source0 is not a full URL (the value of the current Url: would work)
* "%define _exec_prefix /usr" should be avoided
* Directory ownership is still not right, e.g. for include files
* I'd suggest ending manpage %files entries with "*" rather than ".gz" so as to
enable the package to build on systems where manpages aren't compressed, or are
compressed with something else, like bzip
* Please bump the release for every package change during the review process so
that it's easier to tell which comments apply to which version of the package

I'll attach an updated spec and patches.

And finally for now...
* The package does not honor %{optflags}. Worse still, when CFLAGS is set to
"%{optflags}" to correct this, the package fails to build, with some
worrying-looking errors:

...
==== /nis-home/phowarth/BUILD/BUILD/openais-0.75/exec ===
make[1]: Entering directory `/nis-home/phowarth/BUILD/BUILD/openais-0.75/exec'
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o aispoll.o aispoll.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o totemip.o totemip.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o totemnet.o totemnet.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o totemrrp.o totemrrp.c
totemrrp.c: In function 'active_instance_initialize':
totemrrp.c:873: warning: call to __builtin___memset_chk will always overflow
destination buffer
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o totemsrp.o totemsrp.c
totemsrp.c:1151: warning: 'memb_set_print' defined but not used
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o totemmrp.o totemmrp.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o totempg.o totempg.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o tlist.o tlist.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o crypto.o crypto.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o wthread.o wthread.c
ar -rc libtotem_pg.a aispoll.o totemip.o totemnet.o totemrrp.o totemsrp.o
totemmrp.o totempg.o tlist.o crypto.o wthread.o
cc   -ldl -lpthread -L./  -rdynamic -ldl -shared -Wl,-soname,libtotem_pg.so.1
aispoll.o totemip.o totemnet.o totemrrp.o totemsrp.o totemmrp.o totempg.o
tlist.o crypto.o wthread.o -o libtotem_pg.so.1.0
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables   -c -o main.o main.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables   -c -o print.o print.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables   -c -o mempool.o mempool.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables   -c -o util.o util.c
util.c: In function 'getSaNameT':
util.c:99: warning: pointer targets in return differ in signedness
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables   -c -o sync.o sync.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables   -c -o service.o service.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables   -c -o ipc.o ipc.c
ipc.c: In function 'libais_deliver':
ipc.c:532: warning: implicit declaration of function 'getpeereid'
ipc.c: At top level:
ipc.c:675: warning: 'deliver_fn' defined but not used
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables  -fPIC -c -o totemconfig.o totemconfig.c
cc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
--param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables   -c -o mainconfig.o mainconfig.c
cc   -ldl -lpthread -L./  -rdynamic -ldl aispoll.o totemip.o totemnet.o
totemrrp.o totemsrp.o totemmrp.o totempg.o tlist.o crypto.o wthread.o main.o
print.o mempool.o util.o sync.o service.o ipc.o totemconfig.o mainconfig.o
../lcr/lcr_ifact.o libtotem_pg.a -o aisexec
totemnet.o: In function `netif_determine':
/nis-home/phowarth/BUILD/BUILD/openais-0.75/exec/totemnet.c:696: undefined
reference to `totemip_iface_check'
ipc.o: In function `libais_deliver':
/nis-home/phowarth/BUILD/BUILD/openais-0.75/exec/ipc.c:532: undefined reference
to `getpeereid'
collect2: ld returned 1 exit status
make[1]: *** [aisexec] Error 1
make[1]: Leaving directory `/nis-home/phowarth/BUILD/BUILD/openais-0.75/exec'
make: *** [all] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.861 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.861 (%build)

Created attachment 129912 [details]
New spec file promised in Comment #11

rpmlint output from building with this spec:
E: openais non-readable /usr/sbin/ais-keygen 0700
E: openais non-standard-executable-perm /usr/sbin/ais-keygen 0700
W: openais incoherent-subsys /etc/rc.d/init.d/openais $prog

The first two are unavoidable due to the permissions having to be restrictive.
The last is I believe unavoidable if the initscript name doesn't match the
executable name (as is the case here) and you want to be able to use the
standard "status" function.

Created attachment 129913 [details]
Patch for initscript to not enable the service by default

Created attachment 129914 [details]
Patch to Makefile to use DESTDIR more sensibly

Paul

Thanks for all the help!!!  As you can probably tell, I am not expert in RPM
packages..  

The openais project doesn't allow CFLAGS to be overridden from Make.  I think
this is an error upstream and will work to get it resolved and a new release
issued.  I'll also get your other patches merged upstream.  Thanks for the patches.

The problem you are having with the optflags is that -DOPENAIS_LINUX is not
being defined.

A few questions about optflags...

First how do I use rpmbuild to set the optflags like you have done so I can test
out the package build?

Second, The code should compile with -O3 for performance reasons.  But if
optflags is set to -O2 or less, how do I resolve this in the Makefiles?

Here is what I am thinking.

In the toplevel openais Makefile.inc, I will only add to the CFLAGS variable
instead of ever setting it directly.  But this brings up the issue of a compile
with -O2 and -O3 both specified in the command line.

Your expterise would be helpful here.

Thanks!
-steve

(In reply to comment #15)
> The openais project doesn't allow CFLAGS to be overridden from Make.  I think
> this is an error upstream and will work to get it resolved and a new release
> issued.

Good; I had a quick look at the Makefile and I thought it was just adding to
CFLAGS but I was mistaken.

> I'll also get your other patches merged upstream.  Thanks for the patches.

No problem.

> The problem you are having with the optflags is that -DOPENAIS_LINUX is not
> being defined.
> 
> A few questions about optflags...
> 
> First how do I use rpmbuild to set the optflags like you have done so I can test
> out the package build?
> 
> Second, The code should compile with -O3 for performance reasons.  But if
> optflags is set to -O2 or less, how do I resolve this in the Makefiles?
> 
> Here is what I am thinking.
> 
> In the toplevel openais Makefile.inc, I will only add to the CFLAGS variable
> instead of ever setting it directly.  But this brings up the issue of a compile
> with -O2 and -O3 both specified in the command line.

Getting it to honor %optflags and use -O3 isn't too difficult actually, and can
be done with a bit of sed trickery. Use this for the %build section of the spec:

%build
# The code should compile with -O3 for performance reasons
CFLAGS="$(echo '%{optflags}' | sed -e 's/-O[0-9]*//') -O3 -DOPENAIS_LINUX"
make CFLAGS="$CFLAGS"

The spec file from Comment #12 modified in this way builds successfully in mock,
at least on i386.

Paul,
Thanks for all of your wonderful comments.  There is a new srpm and specfile
available below:

Spec URL: http://developer.osdl.org/dev/openais/SRPM/openais.spec
SRPM URL: http://developer.osdl.org/dev/openais/SRPM/openais-0.76-1.0.src.rpm

I have tested that the "make install" part works on x86_64 and i386 and that on
fedora core 5 the openais package runs properly from the init script.

Would you provide one last sanity check?
Thanks

- Still shipping static libs
- Packages still contain unowned dirs
- Compilation still doesn't honor optflags

Without having looked into details, packaging of dynamic libs/plugins seems to
be way off from any common conventions on Linux.

(In reply to comment #18)
> - Still shipping static libs
> - Packages still contain unowned dirs
> - Compilation still doesn't honor optflags
> 
> Without having looked into details, packaging of dynamic libs/plugins seems to
> be way off from any common conventions on Linux.
> 

Thank you for your comments.

Regarding issue #1:
Based upon feedback with the user community there are many people which would
like to link statically to the libraries.  That is one of the motivations of the
project being BSD instead of lgpl.  I see two choices.  I can either a) add the
above text as a comment in the specfile or b) remove the static libs entirely
from the rpm.  I suppose option B has the effect of people then complaining that
the openais package doesn't have the static libs.  Rather more important for me
is inclusion of the package in Fedora Core 6, so if static libs are
non-negotiable they can be removed and I'll deal with the complaining from the
user community.

Regarding issue #2:
I am not sure which unowned dirs I am missing.  Would you be kind enough point
them out to me?  Thanks.

Regarding issue #3:
Somehow I uploaded the wrong specfile to the site.  I did fix the optflags issue
as per Paul's suggestion, and checked the specfile on my laptop has this change.
 I'll upload another tomorrow with the unowned dirs change (if you could point
out the unowned dirs).  rpmlint doesn't seem to complain about this.

Regarding issue #4:
Could you please provide an argument for packaging conventions being way off
from common conventions in Linux.  What exactly is wrong?  I need to understand
what is unsuitable about the packaging methods so they can be fixed (or I can
convince you otherwise).

I'll take a stab anyway:
The packaging of the dynamic libs as files "libSaAmf.so.1.0" as an example is
required by the SA Forum specification.  We try to match the specification as
closely as possible for code portability between implementations.  Hence using
"libsaamf.so.1.0" is not suitable.  First it would not be compliant with the
specification, second code portability would be reduced.  I know the naming is
obnoxious.  The libraries should go into a separate library (and this is indeed
the default of upstream) so that different SA Forum implementations may be tried
on the same system.  Since the filenames of the libraries are defined in the
spec, if we installed to /usr/lib or /usr/lib64, it would prevent people from
installing alternative implementations.

The plugins (the .lcrso files) are a mechanism for exporting interfaces and
supporting "Live Component Replacement" ie: replacing a plugin that is already
loaded with a replacement component without service interruption.  This is not
really a plugin and a shared object doesn't get the job done.  It is more of a
complete component providing a specific service (for example, we have an object
database component which is never used directly by any APIs).  If you have a
suggestion as to where these should "live" rather then /usr/lib/lcrso or
/usr/lib64/lcrso, I'd entertain getting this changed upstream.  As is, we intend
to produce various other lcrso components for use by various projects (and our
lcrso's are totally reusable).

Regards
-steve

(In reply to comment #19)
> (In reply to comment #18)
> > - Still shipping static libs
> > - Packages still contain unowned dirs
> > - Compilation still doesn't honor optflags
> > 
> > Without having looked into details, packaging of dynamic libs/plugins seems to
> > be way off from any common conventions on Linux.
> > 
> 
> Thank you for your comments.
> 
> Regarding issue #1
>               :
> Based upon feedback with the user community there are many people which would
> like to link statically to the libraries.
Yes, some undereducated users and some undereducated maintainers are stubborn
about static libs.

> I see two choices.
Nope, you have only have these choices:
1. Not shipping static libs
2. Prove why you must ship static libs.

> Regarding issue #2
>               :
> I am not sure which unowned dirs I am missing.  Would you be kind enough point
> them out to me?  Thanks.
Pardon, but this is your job.

Install and deinstall your packages and check which dirs remain on the file
system or check the output of rpm -qlp <your-packages>

Hint:
rpm -qlp openais-0.76-1.0.i386.rpm  | grep share/doc
rpm -qlp openais-0.76-1.0.i386.rpm | grep etc

> Regarding issue #3
>               :
> Somehow I uploaded the wrong specfile to the site. 
I used the src.rpm from comment #17.

> Regarding issue #4
>               :
> Could you please provide an argument for packaging conventions being way off
> from common conventions in Linux.
>  What exactly is wrong?
Almost everything:
From the non-devel package:
/usr/lib/openais/lcrso/service_amf.lcrso

From the devel package:
/usr/lib/openais/libSaAmf.a
/usr/lib/openais/libSaAmf.so.1.0

Now compare this against any arbitrary package.
Devel package: libXXX.so -> libXXX.so.1.2.3
non-devel package: libXXX.so.1 -> libXXX.so.1.2.3


> The packaging of the dynamic libs as files "libSaAmf.so.1.0" as an example is
> required by the SA Forum specification.  We try to match the specification as
> closely as possible for code portability between implementations.
Any such approach is doomed to fail. Nothing about shared library naming is
portable, but you can't avoid using the conventions and implications of the OS,
so you must be using the conventions of the OS.

Wrt. plugins, there is no convention. Their names can be arbibrarily choosen.
IMO, xxxxxso is one of the weirdest approaches I've seen. In most cases their
are named "*.so".

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > - Still shipping static libs
> > > - Packages still contain unowned dirs
> > > - Compilation still doesn't honor optflags
> > > 
> > > Without having looked into details, packaging of dynamic libs/plugins seems to
> > > be way off from any common conventions on Linux.
> > > 
> > 
> > Thank you for your comments.

<snip>

Thank you for your comments and suggestions.

I have modified the specfile and project as follows:
* Indeed this time I believe I have the unowned dirs problem solved.
* The static libraries have been removed from the RPM.
* The devel libraries have been renamed library.so.1.0.0 with links from
library.so and library.so.1
* The lcrso files have been moved to /usr/libexec/lcrso as this was suggested as
a better location for the files.
* optflags is now honored
* all warnings related to optflags have been resolved and merged upstream

The lcrso files (part of the main package) do not need a version number.  They
are not meant to be processed by ldconfig or other system utilities.  The LCRSO
system handles all versioning since it must handle live replacement while a
process is active.

Please find the new files at the following location:
Spec URL: http://developer.osdl.org/dev/openais/SRPM/openais.spec
SRPM URL: http://developer.osdl.org/dev/openais/SRPM/openais-0.76-1.1.src.rpm

"man openais_overview" mentions adding a user "ais" to the system. Shouldn't the
package be doing that?

Would it be approriate to do a "/sbin/service openais condrestart" in %post to
restart the service if it's running when the main package is upgraded?

Directory ownership looks OK to me now.

I think %{_libdir}/openais/lib*.so.* should be in the main package, and only
%{_libdir}/openais/lib*.so should be in the devel package. This would be
consistent with other packages. If the libs are only needed for the devel
package, then the /etc/ld.so.conf.d/openais-*.conf file should be in the devel
package and the scriptlet calls of ldconfig should be in the devel package too.

(In reply to comment #22)
> "man openais_overview" mentions adding a user "ais" to the system. Shouldn't the
> package be doing that?
> 

yes check out the new source rpm I have added the functionality.  Unfortunately
I am not sure which UID to use.  Is there a UID registry maintained for fedora?

> Would it be approriate to do a "/sbin/service openais condrestart" in %post to
> restart the service if it's running when the main package is upgraded?
> 

good suggestion and that has been added.

> Directory ownership looks OK to me now.
> 
> I think %{_libdir}/openais/lib*.so.* should be in the main package, and only
> %{_libdir}/openais/lib*.so should be in the devel package. This would be
> consistent with other packages. If the libs are only needed for the devel
> package, then the /etc/ld.so.conf.d/openais-*.conf file should be in the devel
> package and the scriptlet calls of ldconfig should be in the devel package too.
> 

I don't understand why lib*.so.* should be in main and lib*.so be in devel?  As
an example, consider libevs.

The library is libevs.so.1.0.0.  There are two dynamic links libevs.so.1 and
libevs.so that point to libevs.so.1.0.0.

With your suggestion this would put the real binary library libevs.so.1.0.0 and
one of the links libevs.so.1 into the main package and the link libevs.so into
the devel.  But in fact they are all development libraries.
/etc/ld.so.conf.d should be moved and has been.  You are right about the
scriptlets that call ldconfig.  This was causing a weird bug which I couldn't
figure out until your suggestion.

Thanks you have been a world of help.

Here is the latest
* Thu May 31 2006 Steven Dake <sdake> - 0.76-1.2
- Add user account for AIS applications and authentication.
- Move /etc/ld.so.conf/openais-*.conf to devel package since it is
  only needed there.
- Move ldconfig to devel package.
- Execute condrestart on upgrade

Please find the new files at the following location:
Spec URL: http://developer.osdl.org/dev/openais/SRPM/openais.spec
SRPM URL: http://developer.osdl.org/dev/openais/SRPM/openais-0.76-1.2.src.rpm

Further review and help with the default UID to use and better explination of
the .so packaging conventions would be helpful.

Thanks Paul!
-steve

(In reply to comment #23)
> (In reply to comment #22)
> > "man openais_overview" mentions adding a user "ais" to the system. Shouldn't the
> > package be doing that?
> > 
> 
> yes check out the new source rpm I have added the functionality.  Unfortunately
> I am not sure which UID to use.  Is there a UID registry maintained for fedora?

There probably is but I don't know where (Bill Nottingham will probably know).
However, unless there is a requirement for the account to have the same UID
across different machines on a network (e.g. if they required shared access to
the same files using NFS), you could just use the "-r" option of useradd to
create a systen account and not worry about the exact UID.

Should any of the package files be owned by this user?

>> Would it be approriate to do a "/sbin/service openais condrestart" in %post to
>> restart the service if it's running when the main package is upgraded?
>> 
>
> good suggestion and that has been added.

Please add "|| :" to ensure that an initscript problem doesn't damage the rpm
transaction.

You also need to add /sbin/service to Requires(post)

> > I think %{_libdir}/openais/lib*.so.* should be in the main package, and only
> > %{_libdir}/openais/lib*.so should be in the devel package. This would be
> > consistent with other packages. If the libs are only needed for the devel
> > package, then the /etc/ld.so.conf.d/openais-*.conf file should be in the devel
> > package and the scriptlet calls of ldconfig should be in the devel package too.
> > 
> 
> I don't understand why lib*.so.* should be in main and lib*.so be in devel?  As
> an example, consider libevs.
> The library is libevs.so.1.0.0.  There are two dynamic links libevs.so.1 and
> libevs.so that point to libevs.so.1.0.0.

That's normal.

> With your suggestion this would put the real binary library libevs.so.1.0.0 and
> one of the links libevs.so.1 into the main package and the link libevs.so into
> the devel.

Yes, that is the normal thing to do. The real library libXYZ.so.a.b.c and the
versioned symlink libXYZ.so.a are usually needed for runtime use, and the
unversioned symlink libXYZ.so is usually only needed at compile/link time.

> But in fact they are all development libraries.

This is what's unusual (i.e. shared libs needed only for development). Is that
really true? If so, it's correct to put the whole lot in the devel package.

> /etc/ld.so.conf.d should be moved and has been.  You are right about the
> scriptlets that call ldconfig.  This was causing a weird bug which I couldn't
> figure out until your suggestion.

The %post devel scriptlet should be:
%post devel -p /sbin/ldconfig
as with the %postun devel scriptlet; this will get rid of an rpmlint warning and
will save you needing:
Requires(post): /sbin/ldconfig
for the devel package.

Youshould also remove /sbin/ldconfig from the Requires(post) for the main
package, since it's no longer required.

This package is now gettimg close to the point where I feel I could approve it
if it was an Extras package. However, this is a review for Core and I'm not sure
whether I, as a non-Red Hat employee, am able to approve Core packages. That may
be a question for Jesse.

> > yes check out the new source rpm I have added the functionality.  Unfortunately
> > I am not sure which UID to use.  Is there a UID registry maintained for fedora?
> 
> There probably is but I don't know where (Bill Nottingham will probably know).
> However, unless there is a requirement for the account to have the same UID
> across different machines on a network (e.g. if they required shared access to
> the same files using NFS), you could just use the "-r" option of useradd to
> create a systen account and not worry about the exact UID.

Reserve a uid in the uidgid registry (part of the setup package).

Added Phil as CC for review of new uidgid file.

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > "man openais_overview" mentions adding a user "ais" to the system.
Shouldn't the
> > > package be doing that?
> > > 
> > 
> > yes check out the new source rpm I have added the functionality.  Unfortunately
> > I am not sure which UID to use.  Is there a UID registry maintained for fedora?
> 
> There probably is but I don't know where (Bill Nottingham will probably know).
> However, unless there is a requirement for the account to have the same UID
> across different machines on a network (e.g. if they required shared access to
> the same files using NFS), you could just use the "-r" option of useradd to
> create a systen account and not worry about the exact UID.
> 
> Should any of the package files be owned by this user?
> 
> >> Would it be approriate to do a "/sbin/service openais condrestart" in %post to
> >> restart the service if it's running when the main package is upgraded?
> >> 
> >
> > good suggestion and that has been added.
> 
> Please add "|| :" to ensure that an initscript problem doesn't damage the rpm
> transaction.
> 
> You also need to add /sbin/service to Requires(post)
> 
> > > I think %{_libdir}/openais/lib*.so.* should be in the main package, and only
> > > %{_libdir}/openais/lib*.so should be in the devel package. This would be
> > > consistent with other packages. If the libs are only needed for the devel
> > > package, then the /etc/ld.so.conf.d/openais-*.conf file should be in the devel
> > > package and the scriptlet calls of ldconfig should be in the devel package
too.
> > > 
> > 
> > I don't understand why lib*.so.* should be in main and lib*.so be in devel?  As
> > an example, consider libevs.
> > The library is libevs.so.1.0.0.  There are two dynamic links libevs.so.1 and
> > libevs.so that point to libevs.so.1.0.0.
> 
> That's normal.
> 
> > With your suggestion this would put the real binary library libevs.so.1.0.0 and
> > one of the links libevs.so.1 into the main package and the link libevs.so into
> > the devel.
> 
> Yes, that is the normal thing to do. The real library libXYZ.so.a.b.c and the
> versioned symlink libXYZ.so.a are usually needed for runtime use, and the
> unversioned symlink libXYZ.so is usually only needed at compile/link time.
> 
> > But in fact they are all development libraries.
> 
> This is what's unusual (i.e. shared libs needed only for development). Is that
> really true? If so, it's correct to put the whole lot in the devel package.
> 

Thanks for all your great comments.  Yes indeed the libraries are only needed
for development so I'll keep them in the devel package.  I'll fix the rest of
the issues you pointed out.

Thanks for all your hard work in reviewing this package and beating it into
submission.  :)

I'll post a new update in a few hours with your suggested changes.

Created attachment 130534 [details]
a patch to modify the uidgid registry to add the core package openais

this should be merged into the setup package.  Phil will you do this or should
I?  Thanks.
-steve

As per Paul's suggestions, I have made the following changes:
* Mon Jun 5 2006 Steven Dake <sdake> - 0.76-1.3
- Allocated uid 102 from setup package for user add operation.
- Added || : to initscript stuff so initscript bugs dont cause RPM transaction
  failures as per Paul's suggestion.
- Added /sbin/services to post requires as per Paul's suggestion.
- Removed ldconfig from the requires post for the main package as per Paul's
  suggestion.
- Changed post devel scriptlet action as per Paul's suggestion.


The new SRPM and specfile can be downloaded from:
Spec URL: http://developer.osdl.org/dev/openais/SRPM/openais.spec
SRPM URL: http://developer.osdl.org/dev/openais/SRPM/openais-0.76-1.3.src.rpm

Thanks again Paul
regards
-steve

Waaaait. gkrellm aside (that's a bug) all system UIDs for core should be < 100.

Created attachment 130541 [details]
moves uid 102 to 39.

Bill,

Thanks for the comment.  I have adjusted the uid to 39 which appears to be one
of the few remaining uids available under 100.	New SRPMS to come shortly.

Created attachment 130542 [details]
last patch had incorrect package descriptor for new uid

* Mon Jun 5 2006 Steven Dake <sdake> - 0.76-1.4
- Moved uid 102 to 39 since uids over 99 are not suitable for core at Bill's
  suggestion.

The new SRPM and specfile can be downloaded from:
Spec URL: http://developer.osdl.org/dev/openais/SRPM/openais.spec
SRPM URL: http://developer.osdl.org/dev/openais/SRPM/openais-0.76-1.4.src.rpm

Thank you for the correction Bill.

Why does this UID have to be static? Is there data, owned by this user, that
needs to be shared over the network?

Should any of the files in this package be owned by this user?

Not knowing the specifics of the package, it's policy for Core that uids for
system packages be allocated statically for consistency.

(In reply to comment #35)
> Not knowing the specifics of the package, it's policy for Core that uids for
> system packages be allocated statically for consistency.

So what's going to happen after the (imminent) exhaustion of available uids?

I'm getting 403 forbidden on the srpm.

Buildroot is still wrong.

The /sbin/service requirement isn't needed as initscripts get pulled in due to
deps in the Exceptions list.

General approval from my end, if it's needed by the current Core cluster stuff.

rpmlint has some things to say:

E: openais non-readable /usr/sbin/ais-keygen 0700
E: openais non-standard-executable-perm /usr/sbin/ais-keygen 0700
W: openais non-standard-dir-in-usr libexec
W: openais incoherent-subsys /etc/rc.d/init.d/openais $prog
W: openais-devel
conffile-without-noreplace-flag /etc/ld.so.conf.d/openais-i686.conf

My review from yesterday got lost with the bugzilla crash:

Review
======

rpmlint output:

E: openais non-readable /usr/sbin/ais-keygen 0700
E: openais non-standard-executable-perm /usr/sbin/ais-keygen 0700
(required permissions)

W: openais non-standard-dir-in-usr libexec
(not *that* non-standard)

W: openais incoherent-subsys /etc/rc.d/init.d/openais $prog
(daemon and package name are incoherent upstream)

W: openais-devel conffile-without-noreplace-flag /etc/ld.so.conf.d/openais-i686.conf
(is anyone *really* going to edit this file anyway?)

I don't believe any of these are blockers, or even need fixing.

- package and spec file naming OK
- package meets guidelines
- license is BSD, matches spec, text included
- spec file written in ENglish and is legible
- sources match upstream
- builds OK in mock for rawhide (i386)
- buildreqs OK
- no locale-specific data
- shared libraries present in -devel package (only needed for devel)
  ldconfig is properly called in %post and %postun for the devel package
- not relocatable
- no directory ownership or permissions issues
- no duplicate files
- %clean section present and correct
- macro usage is consistent
- code, not content
- documentation volume not excessive
- docs don't affect runtime
- header files properly located in -devel package
- static libraries disabled
- no pkgconfig file
- -devel package has fully-versioned dependency on main package
- no libtool archives included
- not a GUI application, so no desktop file needed
- scriptlets are sane

Issues
======

- package is ExclusiveArch: i386 ppc x86_64 ppc64
  Since this covers all current Fedora Core architectures, why is it present?

- please correct confusing 0.76-1.6 changelog entry

Once these are addressed, I'll be in a position where I'd be happy to approve
this package if it was for Fedora Extras, However, I cannot approve Core
packages, so someone else will need to do that.

Post-review, it was noted that the package failed to build on x86_64 due to
"-fPIC" being missing from CFLAGS, This was to be fixed by a patched Makefile.

The last few postings to bugzilla were lost.  Here are some of the last
changelog entries:

* Tue Jun 13 2006 Steven Dake <sdake> - 0.76-1.8
- Add makefile override patch which fixes build with optflags on x86_64 arch.
- Remove -DOPENAIS_LINUX from passed CFLAGS since it now works properly with
  makefile override patch.

* Tue Jun 13 2006 Steven Dake <sdake> - 0.76-1.7
- Remove ExclusiveArch since all Fedora Core 6 arches have been tested.

* Fri Jun 9 2006 Steven Dake <sdake> - 0.76-1.6
- Move condrestart to %%postun instead of %%post.
- Call initscript directly as suggested by Jesse.

* Thu Jun 8 2006 Steven Dake <sdake> - 0.76-1.5
- Changed BuildRoot tag to match convention specified in Fedora Wiki.
- Removed /sbin/service dependency since it is pulled in from init packages.

* Mon Jun 5 2006 Steven Dake <sdake> - 0.76-1.4
- Moved uid 102 to 39 since uids over 99 are not suitable for core at Bill's
  suggestion.

* Mon Jun 5 2006 Steven Dake <sdake> - 0.76-1.3
- Allocated uid 102 from setup package for user add operation.
- Added || : to initscript stuff so initscript bugs dont cause RPM transaction
  failures as per Paul's suggestion.
- Added /sbin/services to post requires as per Paul's suggestion.
- Removed ldconfig from the requires post for the main package as per Paul's
  suggestion.
- Changed post devel scriptlet action as per Paul's suggestion.

* Thu May 31 2006 Steven Dake <sdake> - 0.76-1.2
- Add user account for AIS applications and authentication.
- Move /etc/ld.so.conf/openais-*.conf to devel package since it is
  only needed there.
- Move ldconfig to devel package.
- Execute condrestart on upgrade

* Fri May 25 2006 Steven Dake <sdake> - 0.76-1.1
- Fix unowned dirs problem.
- Correct make with optflags work properly.
- Move plugins from /usr/lib/openais/lcrso to /usr/libexec/lcrso.

The latest SRPM and specfile are located at:
Spec URL: http://developer.osdl.org/dev/openais/SRPM/openais.spec
SRPM URL: http://developer.osdl.org/dev/openais/SRPM/openais-0.76-1.8.src.rpm

Jesse I believe the rpmlint errors were discussed in some of the thread that was
lot in the bz crash but I agree with Paul's analysis in comment #40.

I have tested that the rpm binaries build, install and work as expected on
x86_64 WS4 and i386 FC5.  Version 0.76 has been tested on both ppc and ppc64.  I
don't have other (ppc*) target arches with FC5 or rawhide to test the rpm build
process.

This was added to rawhide.