Bug 221906 (gmediaserver)

Not building in mock:

checking for flex... no
checking for lex... no
checking for bison... no
checking for byacc... no
checking for x86_64-redhat-linux-gnu-ranlib... ranlib
checking for perl... perl
checking for perl version greater than or equal to 5.6.0... yes
checking for x86_64-redhat-linux-gnu-pkg-config... no
checking for pkg-config... no
checking upnp/upnp.h usability... yes
checking upnp/upnp.h presence... yes
checking for upnp/upnp.h... yes
checking for UpnpInit in -lupnp... yes
checking magic.h usability... no
checking magic.h presence... no
checking for magic.h... no
configure: error: libmagic magic.h header file not found


Looks like you need BuildRequires for file, gettext, flex, and byacc

I fixed it, but I must fix problems with info (%post and %preun sections) before
I publish second release. I think tommorow I will upload new release.

New URLs:
http://karlik.nonlogic.org/gmediaserv/gmediaserver.spec
http://karlik.nonlogic.org/gmediaserv/gmediaserver-0.12.0-2.src.rpm

I read configure.ac and added more BRs.
I had to fix info, because it did not install correctly.

There is no init file for this server.

I will provide a review, but you will need to provide an init file first.

I had to prepare this program to work with init-script. In package are:
-config-file
-init-script
-logrotate-file
New urls:
http://karlik.nonlogic.org/gmediaserv/gmediaserver.spec
http://karlik.nonlogic.org/gmediaserv/gmediaserver-0.12.0-3.src.rpm

Ok, then, let's get started.  I'll break up my findings into multiple replies. 
It should be a little easier to digest this way.

Obvious problems:
* init file uses daemon function from initscripts but does not require
  initscripts

* package can not assign static uid unless it's registered at this uid:
  /usr/sbin/fedora-groupadd -r gmediaserver -u 105 &>/dev/null || :
  /usr/sbin/useradd -c "gmediaserver" -u 105  \

* scriptlet used chkconfig but does not require it
  Requires(post): /sbin/chkconfig
  Requires(preun): /sbin/chkconfig

  same with /sbin/service, but since you will have to include initscripts anyway 
  you will not need that.

* please create backup patch files, ie:
  %patch0 -p0 -b .mypatch

* please use install to perform this.  specifically, you will want to look at
  the -m (mode), -d (directory), -p (preserve timestamp), and -D (create leading
  components):
  mkdir -p $RPM_BUILD_ROOT%{_initrddir}
  cp %{SOURCE1} $RPM_BUILD_ROOT%{_initrddir}/%{name}
  chmod +x $RPM_BUILD_ROOT%{_initrddir}/%{name}

* %define mediadir /var/gmediaserver
  please read http://www.pathname.com/fhs/pub/fhs-2.3.html and determine the
  proper location... perhaps /var/lib/gmediaserver?

* please use the correct macro for /var.  You can find any macros you need with
  'rpm --showrc | grep /var'.  In this case, it's %{_localstatedir}:
  /var/log/%{name}

* package used logrotate.d directory but does not require logrotate:
  %config(noreplace) %{_sysconfdir}/logrotate.d/%{name}

* rpmlint is confused by your init file.  this is a bug in rpmlint though.
  do you need to allow overrides for some of your variables:
  pidfile=${PIDFILE-/var/run/gmediaserver.pid}
  lockfile=${LOCKFILE-/var/lock/subsys/gmediaserver}
  these should never change and would probably be better static unless you have
  a good reason

* please consider adding the LSB bits (BEGING INIT INFO, END INIT INFO) to your
  init file:
 
http://refspecs.freestandards.org/LSB_2.1.0/LSB-generic/LSB-generic/initscrcomconv.html

* mixing fedora-groupadd / groupadd.  Decide on one or the other.  If you want
  to use fedora-usermgmt, you must also put in the proper requires:
  http://fedoraproject.org/wiki/PackageUserCreation

* chown in scriptlets:
  set the uid/gid/mode in the %files section


If you have questions or comments about any of these, please let me know.
  

(In reply to comment #7)
> Obvious problems:
> * init file uses daemon function from initscripts but does not require
>   initscripts
Added

> * package can not assign static uid unless it's registered at this uid:
>   /usr/sbin/fedora-groupadd -r gmediaserver -u 105 &>/dev/null || :
>   /usr/sbin/useradd -c "gmediaserver" -u 105  \
Fixed
> * scriptlet used chkconfig but does not require it
>   Requires(post): /sbin/chkconfig
>   Requires(preun): /sbin/chkconfig
Added
> * please create backup patch files, ie:
>   %patch0 -p0 -b .mypatch
Updated
> * please use install to perform this.  specifically, you will want to look at
>   the -m (mode), -d (directory), -p (preserve timestamp), and -D (create leading
>   components):
>   mkdir -p $RPM_BUILD_ROOT%{_initrddir}
>   cp %{SOURCE1} $RPM_BUILD_ROOT%{_initrddir}/%{name}
>   chmod +x $RPM_BUILD_ROOT%{_initrddir}/%{name}
Updated
> * %define mediadir /var/gmediaserver
>   please read http://www.pathname.com/fhs/pub/fhs-2.3.html and determine the
>   proper location... perhaps /var/lib/gmediaserver?
Change to /srv/gmediaserver (consulted on IRC - #fedora-devel)
> * please use the correct macro for /var.  You can find any macros you need with
>   'rpm --showrc | grep /var'.  In this case, it's %{_localstatedir}:
>   /var/log/%{name}
Fixed
> * package used logrotate.d directory but does not require logrotate:
>   %config(noreplace) %{_sysconfdir}/logrotate.d/%{name}
Fixed
> * rpmlint is confused by your init file.  this is a bug in rpmlint though.
>   do you need to allow overrides for some of your variables:
>   pidfile=${PIDFILE-/var/run/gmediaserver.pid}
>   lockfile=${LOCKFILE-/var/lock/subsys/gmediaserver}
>   these should never change and would probably be better static unless you have
>   a good reason
Fixed
> * please consider adding the LSB bits (BEGING INIT INFO, END INIT INFO) to your
>   init file:
>  
>
http://refspecs.freestandards.org/LSB_2.1.0/LSB-generic/LSB-generic/initscrcomconv.html
Added
> * mixing fedora-groupadd / groupadd.  Decide on one or the other.  If you want
>   to use fedora-usermgmt, you must also put in the proper requires:
>   http://fedoraproject.org/wiki/PackageUserCreation
change to groupadd
> * chown in scriptlets:
>   set the uid/gid/mode in the %files section
Fixed

New urls:
http://karlik.nonlogic.org/gmediaserv/gmediaserver.spec
http://karlik.nonlogic.org/gmediaserv/gmediaserver-0.12.0-4.src.rpm

(In reply to comment #8)
> > * %define mediadir /var/gmediaserver
> >   please read http://www.pathname.com/fhs/pub/fhs-2.3.html and determine the
> >   proper location... perhaps /var/lib/gmediaserver?
> Change to /srv/gmediaserver (consulted on IRC - #fedora-devel)

Seems reasonable to me.

Init file still needs some work.  Here is the relevent rpmlint output:

E: gmediaserver wrong-line-in-lsb-tag # gmediaserver  Start/stop gmediaserver
service
E: gmediaserver wrong-line-in-lsb-tag #
E: gmediaserver unknow-lsb-tag # chkconfig: - 85 15
E: gmediaserver unknow-lsb-tag # description:   GMediaServer is a UPnP
compatible media server for the GNU system. It is part \
E: gmediaserver wrong-line-in-lsb-tag #
E: gmediaserver unknow-lsb-tag # processname: gmediaserver
E: gmediaserver unknow-lsb-tag # config: /etc/sysconfig/gmediaserver
E: gmediaserver unknow-lsb-tag # pidfile: /var/run/gmediaserver.pid
E: gmediaserver missing-mandatory-lsb-tag Provides
E: gmediaserver missing-mandatory-lsb-tag Description
E: gmediaserver missing-mandatory-lsb-tag Short-Description

And here is an example init with the LSB info:

#!/bin/bash
#
# Startup script for the ntop program

# chkconfig: - 81 19
# description: A network traffic probe similar to the UNIX top command
# processname: ntop
# pidfile: /var/run/ntop.pid
# config: /etc/ntop.conf
#
### BEGIN INIT INFO
# Provides:          ntop
# Required-Start:    $local_fs $network $syslog
# Should-Start:
# Required-Stop:
# Default-Start:     3 4 5
# Default-Stop:      0 1 2 6
# Short-Description: Start ntop daemon
# Description:       Start ntop, a network traffic probe similar to the UNIX \
#                    top command
### END INIT INFO



E: gmediaserver executable-marked-as-config-file /etc/sysconfig/gmediaserver
E: gmediaserver script-without-shebang /etc/sysconfig/gmediaserver

this should be installed mode 644

E: gmediaserver executable-marked-as-config-file /etc/logrotate.d/gmediaserver
W: gmediaserver spurious-executable-perm /etc/logrotate.d/gmediaserver

this should be installed mode 644

> install -D -m755 %{SOURCE1} $RPM_BUILD_ROOT%{_initrddir}/%{name}
> cp %{SOURCE1} $RPM_BUILD_ROOT%{_initrddir}/%{name}
> chmod +x $RPM_BUILD_ROOT%{_initrddir}/%{name}

the install then cp is redundant.
use -p to preserve the timestamp.  If you want, you can use 'cp -p' or more
efficiently:
install -D -m 0755 -p %{SOURCE} $RPM_BUILD_ROOT%{_initrddir}/%{name}

> install -D $RPM_BUILD_ROOT%{_bindir}/%{name} 
$RPM_BUILD_ROOT%{_sbindir}/%{name}
> rm $RPM_BUILD_ROOT%{_bindir}/%{name}

just use 'mv' - it will keep the timestamp, mode, ownership, etc of the original
file
you might want a comment here too, explaining that the make install put the file
in /usr/bin but for a server it should be /usr/sbin/(In reply to comment #8)

Please check that /var/log/gmediaserver is writable by the daemon.

http://karlik.nonlogic.org/gmediaserv/gmediaserver.spec
http://karlik.nonlogic.org/gmediaserv/gmediaserver-0.12.0-5.src.rpm

The default log-file was not writable after reinstalling (the dynamic UID/GID),
now it is in %ghost, so after remove package the log will be deleted.
I think it is better idea than:
[ -f "path_to_log_file"] && chown %{name}:%{name} path_to_log_file
and I think now it is more safely.

Sorry, I've been awfully busy and haven't been able to finish this review.  It
will still be a couple of day yet.

I did a quick look over the spec file and things look much better.  There was a
couple of things that stood out.

1) "%patch0 -p0 -b .mypatch" - when I said ".mypatch" I meant to personalize it.
 I suspect in your case it would be something like "%patch0 -p0 -b .infofix".  I
see you took me literally.

2) The %preun and %postun are using two different styles of testing $1.  This
makes it hard to read.  Suggest that you adopt the style of %preun.  You are
also mixing different styles of redirection.  This makes it a little hard to
follow, but it not a blocker.

3) rpmlint will always complain with the userdel/groupdel in the scriptlets. 
Let me check and see what the current recommended way to handle this is.  A lot
of people suggest leaving the user on the system once it's created.  Since it's
going to own the media, we must be careful that reinstallation get the same id
or there will be a problem.  This might be a candidate for static uid, but let
me check.

(In reply to comment #12)
> 3) rpmlint will always complain with the userdel/groupdel in the scriptlets. 
> Let me check and see what the current recommended way to handle this is.  A lot
> of people suggest leaving the user on the system once it's created.  Since it's
> going to own the media, we must be careful that reinstallation get the same id
> or there will be a problem.  This might be a candidate for static uid, but let
> me check.

If the package's own user/group will be used for media files that will be left
on the system if the package is removed, it's certainly best not to delete the
user/group at all. If the package is subsequently reinstalled, useradd/groupadd
will fail but it doesn't matter because the user/group will already be there
with the correct uid/gid respectively (just be sure that the useradd/groupadd
commands don't produce spurious error messages or cause the scriptlets to fail
in this situation).

Well...IMO there are some ways resolve this problem:
1. Do not change it, the owner of /srv/gmediaserver will be changing all time
when the package user will be having other uid/gid and the files are often 644,
so the program can read it. I think it is safe, because if the directory will
not be deleted unless it is empty.
2. Delete from spec:
"%postun
if [ $1 = 0 ]; then
    /usr/sbin/userdel  gmediaserver &>/dev/null || :
    /usr/sbin/groupdel gmediaserver &>/dev/null || :
fi".
It can be good idea, but I think the packages which does not remove users make a
mess. If the all servers do not remove it, after some "testing" instalations
(different packages) in system will be a lot of needless users. On the other
hand there is not a problem with owner of log-file (in gmediaserver) and it is
very safe for the media files.
3. The static uid/gid solve all problems, but I really do not know if the make
static uid/gid is good idea if there are another solution.

I made the spec more clearly, but I make next release when I will know what I
should do with the gmediaserver-package-user.

In the absence of option 3 (static UID/GID), I think option 2 (not removing the
user/group) is much better. It may be "messy" in terms of leaving accounts on
systems but IMHO this is not as "messy" as leaving unowned files on those
systems (which would happen if the user/group were deleted).

There is actually a pseudo-static UID/GID mechanism available in Fedora
(http://fedoraproject.org/wiki/PackageUserCreation) but use of it is contentious
to say the least and the possibility of expanding the static uid/gid range
beyond the current 0-100 was favourably received when the latest flamewar on
this subject erupted recently (see
http://www.redhat.com/archives/fedora-devel-list/2007-March/msg00647.html and
the "discussion" on fedora-maintainers linked from there).

(In reply to comment #15)
> In the absence of option 3 (static UID/GID), I think option 2 (not removing the
> user/group) is much better. It may be "messy" in terms of leaving accounts on
> systems but IMHO this is not as "messy" as leaving unowned files on those
> systems (which would happen if the user/group were deleted).

> There is actually a pseudo-static UID/GID mechanism available in Fedora
> (http://fedoraproject.org/wiki/PackageUserCreation) but use of it is contentious

I agree with Paul here.  This is messy, but ATM there is not a good way to
handle this except with fedora-usermgmt as noted above.  However, I would not
recommend fedora-usermgmt (although I use it on my packages) until the
controversy regarding it's use is ironed out.

So for now, let's go with the persistent user/group.

(In reply to comment #14)
> Well...IMO there are some ways resolve this problem:
> 1. Do not change it, the owner of /srv/gmediaserver will be changing all time
> when the package user will be having other uid/gid and the files are often 644,
> so the program can read it. I think it is safe, because if the directory will
> not be deleted unless it is empty.

But there will be potentially unowned files depending on how the media server is
used.  You're lucky in the regard that the media server doesn't actually have to
manage the files, only read them - otherwise it would be more sticky.

"So for now, let's go with the persistent user/group."
I really do not understand it. Should I use "fedora-usermgmt" with uid? If yes,
which number can I use as uid/gid? I am testing fedora-useradd, but I do not
uderstand how it works.

I mean you can add the user/group dynamically, then never remove it.  That will
assure that it uses the same uid/gid.

None of these solutions are ideal, but the current state of affairs doesn't
offer much better until Fesco and/or FPC makes some decisions on this matter.

Using fedora-usermgmt will not necessarily buy you anything in this case unless
you are also using the static id registry.

Well...I hope that there is finish release:
http://karlik.nonlogic.org/gmediaserv/gmediaserver.spec
http://karlik.nonlogic.org/gmediaserv/gmediaserver-0.12.0-6.src.rpm

Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Rpmlint output:
       E: gmediaserver non-standard-uid /srv/gmediaserver gmediaserver
       E: gmediaserver non-standard-gid /srv/gmediaserver gmediaserver
       E: gmediaserver non-standard-uid /var/log/gmediaserver/gmediaserver.log
gmediaserver
       E: gmediaserver non-standard-gid /var/log/gmediaserver/gmediaserver.log
gmediaserver
       E: gmediaserver non-standard-uid /var/log/gmediaserver gmediaserver
       E: gmediaserver non-standard-gid /var/log/gmediaserver gmediaserver

      These are acceptable as the package will own the log and media directories.
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must matches the base package %{name}, in the format
%{name}.spec.
 [x] Package meets the  Packaging Guidelines.
 [x] Package is licensed with an open-source compatible license and meet other
legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [x] Spec file is written in American English.
 [x] Spec file for the package is legible.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
     MD5SUM this package    : 7f99a9b2e32c41acf7e71eb0bc0840e9
     MD5SUM upstream package: 7f99a9b2e32c41acf7e71eb0bc0840e9
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on: FC6/i386
 [x] Package is not known to require ExcludeArch, OR:
     Arches excluded:
     Why:
 [!] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packaging Guidelines.
     Build is not picking up the uuid libraries.  These are not the ossp-uuid
libraries that you added to the BR, but the e2fsprogs uuid libraries.  Please
remove uuid-devel from the BR and add e2pfsprogs-devel.  BR also does not
contain pkgconfig which is required for the uuid detection.
 [x] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package is not relocatable.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: FC6/i386
 [-] Package should compile and build into binary rpms on all supported
architectures.
     Tested on:
 [?] Package functions as described.
     Unable to test as I don't have any upnp devices.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files are correct.
 [x] File based requires are sane.
 [x] Latest version is packaged.

=== Issues ===
1.  Package source file timestamp does not match upstream.  Consider downloading
with "wget -N".
2.  Missing/incorrect BR noted above.
3.  init file line wraps on line 4-5, please pick a better break point
4.  init file / LSB info requires $syslog - not needed since it does not use syslog

=== Final Notes ===
1.  I was only able to start/stop program and verify that it did run.  I don't
have a upnp media device to test with.  I also noted that there was no
information sent to the log files.  If this seems to be a problem, please look
into it.


Go ahead and fix this up and I'll approve the package.

I am sorry, I was very bussy last time, so I can not fix/update package.

(In reply to comment #20)
>  [!] All build dependencies are listed in BuildRequires, except for any that are
> listed in the exceptions section of Packaging Guidelines.
>      Build is not picking up the uuid libraries.  These are not the ossp-uuid
> libraries that you added to the BR, but the e2fsprogs uuid libraries.  Please
> remove uuid-devel from the BR and add e2pfsprogs-devel.  BR also does not
> contain pkgconfig which is required for the uuid detection.
I wrote BRs based on configure. Now it is fixed.
> 1.  Package source file timestamp does not match upstream.  Consider downloading
> with "wget -N".
Update sources (download by wget -cN ....)
> 2.  Missing/incorrect BR noted above.
Fixed
> 3.  init file line wraps on line 4-5, please pick a better break point
I set break point in description between sentences.
> 4.  init file / LSB info requires $syslog - not needed since it does not use
syslog
Deleted

http://karlik.nonlogic.org/gmediaserv/gmediaserver.spec
http://karlik.nonlogic.org/gmediaserv/gmediaserver-0.12.0-7.src.rpm

(In reply to comment #21)
> I wrote BRs based on configure. Now it is fixed.

Yeah, it took me a while to figure out why configure wasn't picking it up.

Most everything is fixed at this point, so this package is
================
*** APPROVED ***
================ 

One last thing that I notice that you probably want to fix before your first build:

all the scriptlet lines should end with "|| :" otherwise spurious errors on any
of the commands will cause scriptlet errors and package
install/upgrade/uninstall failures.

Thanks for Your review (it taught me a lot of new things). Now, I can add
fedora-cvs comment.

Yes, you can find the information on how to request your packages CVS module
creation here:
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure?action=show&redirect=CVSAdminProcedure

New Package CVS Request
=======================
Package Name: gmediaserver
Short Description: UPnP compatible media server for the GNU system
Owners: karlikt
Branches: FC-6
InitialCC: 

Built correct on buildsys, so I can close :)