Bug 491497 - Review Request: dmapd - A server that provides DAAP and DPAP shares
Summary: Review Request: dmapd - A server that provides DAAP and DPAP shares
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Christian Krause
QA Contact: Fedora Extras Quality Assurance
URL: http://www.flyn.org/projects/dmapd/
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-03-22 00:22 UTC by W. Michael Petullo
Modified: 2010-02-19 20:46 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2010-02-19 20:46:16 UTC
Type: ---
Embargoed:
chkr: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description W. Michael Petullo 2009-03-22 00:22:54 UTC
Spec URL: http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.8-1.fc10.src.rpm
Description:
The dmapd project provides a GObject-based, Open Source implementation
of DMAP sharing with the following features:

 o Support for both DAAP and DPAP

 o Support for realtime transcoding of media formats not natively
 supported by clients

 o Support for many metadata formats, such as those associated with Ogg
 Vorbis and MP3 (e.g., ID3)

 o Detection of video streams so that clients may play them as video

 o Use of GStreamer to support a wide range of audio and video CODECs

 o Caching of photograph thumbnails to avoid regenerating them each time
 the server restarts

Comment 1 Fabian Affolter 2009-05-06 10:25:37 UTC
Just some quick comments on your spec file

- Don't mix '$RPM_BUILD_ROOT' and '%{buildroot}'
  http://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS
- There are two '%defattr(-, root, root, -)' entries
- You are working with users/group. Aren't 'Requires(pre): /usr/sbin/useradd  /usr/sbin/groupadd' and 'Requires(postun): /usr/sbin/userdel /usr/sbin/groupdel' missing? 
- Same with the service. 'Requires(post): chkconfig' and 'Requires(preun): chkconfig'
- disttag is not needed in changelog, you can remove '.fc10' there

Comment 2 W. Michael Petullo 2009-05-07 11:53:09 UTC
Spec URL:  http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.10-1.fc10.src.rpm

Comment 3 Juha Tuomala 2009-07-21 16:27:09 UTC
URL in spec should point to: http://www.flyn.org/projects/dmapd/

Comment 4 W. Michael Petullo 2009-07-23 05:06:25 UTC
Spec URL:  http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.14-1.fc11.src.rpm

Comment 5 Fabian Affolter 2009-08-03 10:38:37 UTC
BTW, your spec file is hard to read because it doesn't follow the 'standard'  conventions.  Of course, is your way not wrong, just unusual.  Most packager follows more or less the template that can be created with 'rpmdev-newspec blablabla' (looks a bit like this https://fedoraproject.org/wiki/How_to_create_an_RPM_package#Creating_a_blank_spec_file)

Comment 6 W. Michael Petullo 2009-09-07 00:51:47 UTC
Fabian, are you willing to review the spec file in its current form? The reason I don't use rpmdev-newspec is that my spec files are generated from an XML project description.

Comment 7 Christian Krause 2009-11-10 23:27:02 UTC
Before I do the full review here are some issues which should be fixed before:

1. does not build if dbus-devel is not installed - please add it to the build requires (and probably to the configure script ;-) )

2. lots of warnings when using rpmlint:
rpmlint SPECS/dmapd.spec RPMS/i686/dmapd-* SRPMS/dmapd-0.0.14-1.fc12.src.rpm 
dmapd.i686: W: conffile-without-noreplace-flag /etc/sysconfig/dmapd
dmapd.i686: W: devel-file-in-non-devel-package /usr/lib/dmapd/0.0.14/modules/libdb-gdir.so
dmapd.i686: W: devel-file-in-non-devel-package /usr/include/dmapd-dpap-record.h
dmapd.i686: W: devel-file-in-non-devel-package /usr/include/dmapd-daap-record.h
dmapd.i686: W: devel-file-in-non-devel-package /usr/lib/pkgconfig/dmapd.pc
dmapd.i686: W: non-standard-uid /var/run/dmapd dmapd
dmapd.i686: E: non-standard-dir-perm /var/run/dmapd 0700
dmapd.i686: E: postin-without-ldconfig /usr/lib/libdmapd.so.0.0.0
dmapd.i686: E: postun-without-ldconfig /usr/lib/libdmapd.so.0.0.0
dmapd.i686: W: non-standard-uid /var/cache/dmapd dmapd
dmapd.i686: E: non-standard-dir-perm /var/cache/dmapd 0700
dmapd.i686: W: devel-file-in-non-devel-package /usr/lib/libdmapd.so
dmapd.i686: E: executable-marked-as-config-file /etc/sysconfig/dmapd
dmapd.i686: E: script-without-shebang /etc/sysconfig/dmapd
dmapd.i686: W: percent-in-%pre
dmapd.i686: W: percent-in-%postun
dmapd.i686: W: dangerous-command-in-%postun userdel
3 packages and 1 specfiles checked; 6 errors, 11 warnings.

There are some false positives, but at least the devel-file-in-non-devel-package should be fixed first. Please create either a -devel subpackage or delete the devel files in the %install section if it is not intended to build something based in libdmapd.so.

3. /etc/sysconfig/dmapd should not be executable

4. add ldconfig to %post and %postun

5. I have to second Fabian's request to enhance the formatting of the spec file:
- there are lots of unneeded empty lines
- the order of the main sections is unusual, e.g. %doc is not directly after %files, but somewere later in the file
- the scriptlets in the %preund and %postun sections are squeezed into single lines which makes them hard to read / verify
Probably you can enhance/tweak the tool which generates the spec file a little bit. ;-)

6. the way of creation/deletion of the user and groups is not the official one (and it doesn't work at all)

rpm -U ~rpmbuild/RPMS/i686/dmapd-0.0.14-1.fc12.i686.rpm 
/var/tmp/rpm-tmp.K6nmcB: line 1: fg: no job control
/var/tmp/rpm-tmp.K6nmcB: line 2: fg: no job control
warning: user dmapd does not exist - using root
warning: user dmapd does not exist - using root

rpm -e dmapd
/var/tmp/rpm-tmp.RHEC9n: line 2: fg: no job control
/var/tmp/rpm-tmp.RHEC9n: line 3: fg: no job control
/var/tmp/rpm-tmp.RHEC9n: line 5: fg: no job control
warning: %postun(dmapd-0.0.14-1.fc12.i686) scriptlet failed, exit status 1

Please change it according to http://fedoraproject.org/wiki/Packaging:UsersAndGroups (which is the official specification).

Comment 8 W. Michael Petullo 2009-11-11 05:25:03 UTC
Spec URL:  http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.15-1.fc12.src.rpm

- New upstream version.
- Require dbus-devel to build.
- Properly set permissions of /etc/sysconfig/dmapd.
- Run ldconfig.
- Fix user creation.

Comment 9 Christian Krause 2009-11-11 23:15:22 UTC
The new version 0.0.15 does not compile against the current version of
libdmapsharing because a header file is missing:

Version of libdmapsharing-devel:
libdmapsharing-devel-1.9.0.10-1.fc12.i686

The compile error of dmapd:
[...]
In file included from av-meta-reader.c:21:
av-meta-reader.h:24:33: error: libdmapsharing/dmap.h: No such file or directory
In file included from av-meta-reader.c:21:
[...]

libdmapsharing-devel does not provide this file so far. By looking at the source I'm assuming you've restructured the include files in both the lib and the daemon. ;-)

Please either update the libdmapsharing package as well or keep using 0.0.14 for the review and do both updates later. That's up to you...

Comment 10 W. Michael Petullo 2009-11-13 02:21:17 UTC
I've just queued a libdmapsharing 1.9.0.13 update for F-11, F-12 and devel.

Comment 11 Christian Krause 2009-11-20 22:08:50 UTC
I've tested the newest package with the libdmapshare package from updates-testing:

Please use rpmlint. ;-) It reveals not only minor problems but also major issues:

1. %doc must be inside of a %files section

2. the FAQ file should not be packaged since it doesn't contain anything

3. the package contains a library but doesn't call ldconfig
http://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries

4. the source tarball which is available upstream is not the same as the one in the provided src.rpm:
d1b81cc2a5f3d4bb76366d12914cfcc9  dmapd-0.0.15.tar.gz (upstream)
dc00b094845c758a5e193e4968439de5  SOURCES/dmapd-0.0.15.tar.gz (from src.rpm)

5. dmapd can't be started:

/usr/lib/dmapd/0.0.15/modules/libphoto-meta-reader-graphicsmagick.so: cannot open shared object file: No such file or directory

** ERROR **: Error opening /usr/lib/dmapd/0.0.15/modules/libphoto-meta-reader-graphicsmagick.so
aborting...
Aborted

Comment 12 W. Michael Petullo 2009-11-22 03:25:33 UTC
Spec URL: http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.16-1.fc12.src.rpm

- New upstream version
- Move %doc to %files
- No empty FAQ
- Require GraphicsMagick-devel

Comment 13 Christian Krause 2009-11-22 15:39:42 UTC
I've tested the newest package. It looks better, but there are still a couple of problems:

1. The spec file you provided does not match the spec file inside the src.rpm. Please make sure that you also provide a consistent pair of spec file and src.rpm .

2. According to the used directory and file owners ("dmapd") I assume the service should run as the user dmapd. If the daemon is started via /etc/init.d/dmapd, the service runs as root.

3. The ldconfig call in %post is correct, but the second one in %preun is wrong - it must be in %postun.

4. Here is the current rpmlint output:

rpmlint RPMS/i686/dmapd-*16* SRPMS/dmapd-0.0.16-1.fc12.src.rpm SPECS/dmapd.spec
dmapd.i686: W: conffile-without-noreplace-flag /etc/sysconfig/dmapd
dmapd.i686: W: non-standard-uid /var/db/Storage/Movies dmapd
dmapd.i686: E: non-standard-dir-perm /var/db/Storage/Movies 0700
dmapd.i686: W: non-standard-uid /var/db/Storage dmapd
dmapd.i686: E: non-standard-dir-perm /var/db/Storage 0700
dmapd.i686: W: non-standard-uid /var/run/dmapd dmapd
dmapd.i686: E: non-standard-dir-perm /var/run/dmapd 0700
dmapd.i686: W: non-standard-uid /var/cache/dmapd dmapd
dmapd.i686: E: non-standard-dir-perm /var/cache/dmapd 0700
dmapd.i686: E: zero-length /usr/share/doc/dmapd-0.0.16/NEWS
dmapd.i686: W: non-standard-uid /var/db/Storage/Pictures dmapd
dmapd.i686: E: non-standard-dir-perm /var/db/Storage/Pictures 0700
dmapd.i686: W: non-standard-uid /var/db/Storage/Music dmapd
dmapd.i686: E: non-standard-dir-perm /var/db/Storage/Music 0700
dmapd.i686: E: postun-without-ldconfig /usr/lib/libdmapd.so.0.0.16
dmapd.i686: W: non-standard-dir-in-var db
dmapd-devel.i686: W: no-documentation
4 packages and 1 specfiles checked; 8 errors, 9 warnings.

Please fix: 
dmapd.i686: E: zero-length /usr/share/doc/dmapd-0.0.16/NEWS
dmapd.i686: E: postun-without-ldconfig /usr/lib/libdmapd.so.0.0.16
(see 3.)

Please comment to:
dmapd.i686: W: non-standard-dir-in-var db
Especially: What's the purpose of this directory? Could it be probably put somewhere else?

Comment 14 W. Michael Petullo 2009-11-22 22:06:46 UTC
Spec URL: http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.17-1.fc12.src.rpm

- New upstream version (fixes run as dmapd)
- Fix ldconfig placement
- No empty NEWS
- Move data directory to /var/db/dmapd

I changed /var/db/Storage to /var/db/dmapd. I feel this is the correct place for this data. While most of the filesystem may be mounted read-only, a user might want to add media to this directory (thus /var). It is a database, though I admit not a very sophisticated one (thus /var/db).

Comment 15 Christian Krause 2009-12-03 22:37:10 UTC
Sorry that it took a little bit longer than expected, but here is now the complete review:

* rpmlint: TODO
dmapd.i686: W: conffile-without-noreplace-flag /etc/sysconfig/dmapd
- please consider to use "%config(noreplace)"

dmapd.i686: W: non-standard-uid /var/run/dmapd dmapd
dmapd.i686: E: non-standard-dir-perm /var/run/dmapd 0700
dmapd.i686: W: non-standard-uid /var/cache/dmapd dmapd
dmapd.i686: E: non-standard-dir-perm /var/cache/dmapd 0700

These entries seem to be false positives.

dmapd-devel.i686: W: no-documentation

This is also a false positive.

dmapd.i686: W: non-standard-uid /var/db/dmapd dmapd
dmapd.i686: E: non-standard-dir-perm /var/db/dmapd 0700
dmapd.i686: W: non-standard-uid /var/db/dmapd/Pictures dmapd
dmapd.i686: E: non-standard-dir-perm /var/db/dmapd/Pictures 0700
dmapd.i686: W: non-standard-uid /var/db/dmapd/Movies dmapd
dmapd.i686: E: non-standard-dir-perm /var/db/dmapd/Movies 0700
dmapd.i686: W: non-standard-uid /var/db/dmapd/Music dmapd
dmapd.i686: E: non-standard-dir-perm /var/db/dmapd/Music 0700
dmapd.i686: W: non-standard-dir-in-var db

Regarding these directories I have some doubts. I've read your explanation in 
comment #14, but I think that the chosen directory /var/db/dmapd is quite
arbitrary. I would leave it up completely to the user, where he puts its data and so I'd recommend:
- don't create these directories and don't package them
- put the "--user dmapd" argument into the init script
- comment out the line DMAPDARGS in /etc/sysconfig/dmapd and make sure, that
the daemon complains about a missing option when the user has not adjusted or activated this line

* naming: OK
- name matches upstream
- spec file name matches package name

* Source0: OK
- md5sum: 4ac7dc73d4374a4735a5714ff23aaa26  dmapd-0.0.17.tar.gz
- sources matches upstream
- Source0 tag ok
- spectool -g works

* License: OK
- GPLv2+ acceptable for Fedora
- license matches the actual license: Although the sources uses LGPLv2+ and GPLv2+ it is OK to use soleley GPLv2+ as final license for the binary package.
- license file packaged

* spec file written in English and legible: TODO
- please consider to use the standard order of the sections and the standard
formatting of spec files

* Compilation: OK
- package builds in koji on Rawhide on all archs: OK
- The package can not be built on F11 and F12 since
libdmapsharing is not yet updated to 1.9.0.13 in these releases
- parallel make ok

* build requirements: TODO
- IMHO avahi-devel and dbus-devel can be removed from the build requirements
since all dbus/avahi handling will be done via libdmapsharing. A direct
dependency for runtime or build time to avahi/dbus is not necessary.

* locale handling: OK (n/a)

* ldconfig in %post and %postun: OK

* directory ownership: TODO
Even if there is currently a proposal to change this, currently the official
policy is still to "Require: pkgconfig" in the -devel subpackage if
there are any pkgconfig files shipped.

* files not listed twice: OK

* file permissions: OK
- %defattr used
- actual permissions in packages ok

* rm -rf $RPM_BUILD_ROOT in %install and %clean: OK

* macro usage: TODO
- please substitute "%{PACKAGE_VERSION}" with "%{version}" for consistency
- please use also %{_initdir}

* code vs. content: OK (no content)

* large documentation in subpackage: OK (n/a)

* header files in -devel subpackage: OK

* static libraries in -static package: OK (n/a)

* *.so link in -devel package: OK

* -devel package requires base package using fully versioned dependency: OK

* package must not contain *.la files: OK

* package containing *.pc files must "Requires: pkgconfig": TODO
- please require pkgconfig in -devel subpackage (see above)

* desktop-file: OK (n/a, server application)

* compiler flags: OK (rpmoptflags used)

* debuginfo complete: OK

* functional check: TODO
- /etc/init.d/dmapd start works
- service DAAP service is detected and serving music works
- /etc/init.d/dmapd stop does not work:
root@:~# /etc/init.d/dmapd start
Starting DMAP server:                                      [  OK  ]
root@:~# /etc/init.d/dmapd stop
Shutting down DMAP server:                                 [FAILED]

* packages must not own files/directories already owned by other packages: OK

* all file names UTF-8: OK

* scriptlets: TODO
ldconfig: OK
chkconfig: TODO
http://fedoraproject.org/wiki/Packaging:SysVInitScript:
- "Requires(preun): initscripts" is missing
- "Requires(postun): initscripts" is missing
- scriptles in spec file are ok
user creation/deletion: ok

* sysvinit scripts:
- stop() should return $RETVAL
- script mentions "condrestart", but command won't be accepted - this command is required
- starting the service twice will result twice the number of dmapd processes and strange error messages:
root@:~# /etc/init.d/dmapd start
Starting DMAP server:                                      [  OK  ]
root@:~# 
** (dmapd:12388): WARNING **: Unable to start music sharing server on port 8770, trying any open port

** (dmapd:12388): WARNING **: Unable to notify network of media sharing: Could not add service: Local name collision

** (dmapd:12386): WARNING **: Unable to start music sharing server on port 3689, trying any open port

** (dmapd:12386): WARNING **: Unable to notify network of media sharing: Could not add service: Local name collision

- Please have a look at "http://fedoraproject.org/wiki/Packaging:SysVInitScript" for the basic requirements for initscripts in Fedora (the LSB parts are not necessary). Probably it would be the best to use the example init scripts as a base for dmapd's init script. Most likely lots of the mentioned problems will be fixed then automatically.


Summary:
--------
- use "%config(noreplace)" for /etc/sysconfig/dmapd
- consider not to package /var/db/dmapd at all
- remove unneeded build requirements
- minor macro usage fixes
- consider to reformat the spec file to be more standard conform
- add pkgconfig requirement for -devel subpackage
- add the missing requires for the chkconfig scriptlets
- rework the init scripts

Comment 16 W. Michael Petullo 2009-12-05 02:11:21 UTC
Spec URL: http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.18-1.fc12.src.rpm

This fixes everything except:

1. The "starting the service twice" problem. The master process will have to terminate its children. I will have to fix this in my dmapd source.

2. I still use /var/db/dmapd. I think this is a good default, similar to /var/www and well-reasoned within the LSB definitions of /var and /var/db. My intention is a simple, pre-configured setup.

Comment 17 Christian Krause 2009-12-28 09:39:46 UTC
Sorry for the late answer, the December is always quite a busy month. I've looked at the new package and I've seen the following:

- please use also %{_initdir} in this line:
install -D -m 755 distro/dmapd.fedora %{buildroot}%{_sysconfdir}/rc.d/init.d/dmapd

- rpmlint reveals a new problem:
dmapd.i686: W: incoherent-subsys /etc/rc.d/init.d/dmapd $prog
Technically your solution seems to be OK as well, so I don't know why rpmlint complains here...

It should be sufficient (and also not wrong) to replace 
lockfile=/var/lock/subsys/$prog
with
lockfile=/var/lock/subsys/dmapd

Comment 18 W. Michael Petullo 2010-01-15 03:27:49 UTC
Spec URL: http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.18-2.fc12.src.rpm

Uses %{_initdir} throughout.

Comment 19 Christian Krause 2010-01-28 00:12:26 UTC
(In reply to comment #18)
> Spec URL: http://www.flyn.org/SRPMS/dmapd.spec
> SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.18-2.fc12.src.rpm

Thanks for the new package. What about my second comment regarding the new rpmlint warning:
dmapd.i686: W: incoherent-subsys /etc/rc.d/init.d/dmapd $prog
Please see comment #17 for an idea how to fix it.

I've thought a little bit more about the usage of /var/db/dmapd and I'm really unsure - I'll ask on the fedora-packaging mailing list for clarification...

Comment 20 W. Michael Petullo 2010-01-28 01:12:30 UTC
The rpmlint message is clearly a false positive. I'm not claiming that my code is perfect, but I do think it is bad practice to modify it because rpmlint made an unjustified complaint. To be honest, the right answer would probably be to replace the use of "dmapd" with $prog in all but the prog= declaration.

I would like some more official feedback regarding the /var/db/dmapd issue. As I stated, I am going for ease of install. There is a precedent for this technique (httpd's /var/www comes to mind). Also, the choice of /var is consistent with the FHS. There is precedent for db, but I am not wed to it (i.e., one could argue /var/lib is better, but the FHS is a bit vague on the intent behind this directory). I'd like to find a clear Fedora policy on 1) creating a data directory so that a pre-configured server may be installed 2) /var/db vs. /var/lib, etc.

Comment 21 Christian Krause 2010-01-28 22:49:22 UTC
(In reply to comment #20)
> The rpmlint message is clearly a false positive. I'm not claiming that my code
> is perfect, but I do think it is bad practice to modify it because rpmlint made
> an unjustified complaint. To be honest, the right answer would probably be to
> replace the use of "dmapd" with $prog in all but the prog= declaration.

I agree that it may be arguable whether this warning is important or not. However, the fix I've suggested in comment #17 would be still correct. It would not compromise the readability of the script, but it would make the warning vanish. Having as less warnings as possible will help later during maintenance or auditing. (There are also other warnings of rpmlint like the usage of spaces vs. tabs in the spec file which may be also debatable but which should be still honored.)

Please do this little fix.

> I would like some more official feedback regarding the /var/db/dmapd issue. As
> I stated, I am going for ease of install. There is a precedent for this
> technique (httpd's /var/www comes to mind). Also, the choice of /var is
> consistent with the FHS. There is precedent for db, but I am not wed to it
> (i.e., one could argue /var/lib is better, but the FHS is a bit vague on the
> intent behind this directory). I'd like to find a clear Fedora policy on 1)
> creating a data directory so that a pre-configured server may be installed 2)
> /var/db vs. /var/lib, etc.    

The feedback I got from fedora-packaging indicates, that /var/db/dmapd should not be used. I'd still suggest to not package an directory for the music/video/photo files at all and explicitly require that the user should configure it.

I also believe that we should make it easy for the user, but your proposed solution requires also a standard user to know how he/she can copy files into the directory solely owned by a different user (dmap in this case).

The easiest way would be to check whether the user has configured at least one directory in /etc/sysconfig/dmapd and abort the init script otherwise.


I was going to do a final functionality test, but unfortunately dmapd doesn't start at all:

:~# /etc/init.d/dmapd start
Starting dmapd: /usr/lib/dmapd/0.0.18/modules/libav-meta-reader-gst.so: cannot open shared object file: No such file or directory

** ERROR **: Error opening /usr/lib/dmapd/0.0.18/modules/libav-meta-reader-gst.so
aborting...
/bin/bash: line 1: 19791 Aborted                 /usr/sbin/dmapd --user dmapd -m /var/db/dmapd/Music -m /var/db/dmapd/Movies -p /var/db/dmapd/Pictures
                                                           [FAILED]

When you provide a new package, please give it always a quick smoke test. ;-)

Comment 22 W. Michael Petullo 2010-01-29 03:29:18 UTC
Spec URL: http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.21-1.fc12.src.rpm

- New upstream version (fixes init script and gst.so issue)
- no longer install /etc/sysconfig/dmapd, use /etc/dmapd.conf

I also submitted a bug against rpmlint based on comment #17, bug #559800.

Comment 23 Christian Krause 2010-02-04 23:59:29 UTC
Thanks for the new package.

The remaining rpmlint warnings are all false positives:
dmapd.i586: W: non-standard-uid /var/cache/dmapd dmapd
dmapd.i586: E: non-standard-dir-perm /var/cache/dmapd 0700
dmapd.i586: W: non-standard-uid /var/run/dmapd dmapd
dmapd.i586: E: non-standard-dir-perm /var/run/dmapd 0700

Thanks for removing the default directories, I think that solution with the current /etc/dmapd.conf is much better (probably you could activate the "User=dmap" line per default). ;-)


I've tested your package and I still see some issues when starting/stopping the daemon:

1. when the service is started lots of warnings are printed on the console:
** (dmapd:12480): WARNING **: Unused metadata
** (dmapd:12480): WARNING **: Unused metadata
** (dmapd:12480): WARNING **: Unused metadata
** (dmapd:12480): WARNING **: Unused metadata
Warnings of a daemon should rather be written in a log file or omitted and not printed on the console per default.

2. the daemon can't be stopped:
root@:~# /etc/init.d/dmapd start
root@:~# /etc/init.d/dmapd status
dmapd (pid 12605 12604) is running...
root@:~# /etc/init.d/dmapd stop
Stopping dmapd:                                            [  OK  ]
root@:~# /etc/init.d/dmapd status
dmapd (pid 12605) is running...
root@:~# /etc/init.d/dmapd stop
Stopping dmapd:                                            [FAILED]
root@~# /etc/init.d/dmapd status
dmapd (pid 12605) is running...

After this the daemon can't be started anymore unless the remaining dmapd process is killed.

Comment 24 W. Michael Petullo 2010-02-05 21:44:52 UTC
Spec URL: http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.22-1.fc12.src.rpm

- New upstream version (fixes "service dmapd stop")

The issue regarding the "Unused metadata" message is partially fixed. This particular message has been downgraded to a debug message, so you will not see it unless the environment variable DMAPD_DEBUG is set. On the other hand, different, less common messages still exist. This is more a problem with the present limitations of the dmapd logging system than the package itself. Luckily, I am the upstream maintainer and will fix this soon. However, I would like to get dmapd accepted into Fedora as is because the software works properly despite the logging issue.

Comment 25 Christian Krause 2010-02-17 23:03:31 UTC
(In reply to comment #24)
> Spec URL: http://www.flyn.org/SRPMS/dmapd.spec
> SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.22-1.fc12.src.rpm

Thanks for the new package.

> - New upstream version (fixes "service dmapd stop")

Very good - thank you very much. Starting, Restarting and Stopping of the daemon does now work as expected.

> The issue regarding the "Unused metadata" message is partially fixed. This
> particular message has been downgraded to a debug message, so you will not see
> it unless the environment variable DMAPD_DEBUG is set. On the other hand,
> different, less common messages still exist. This is more a problem with the
> present limitations of the dmapd logging system than the package itself.
> Luckily, I am the upstream maintainer and will fix this soon. However, I would

Yes, I do still see lots of warnings if e.g. the files don't have correct id3 tags etc.

> like to get dmapd accepted into Fedora as is because the software works
> properly despite the logging issue.

Yes, I agree with you - the logging issue will not block the review. I have re-checked all my comments and the package looks now quite good besides one issue I've just discovered (sorry I haven't recognized this earlier):


Please enable the "User" config option by default in /etc/dmapd.conf:
------------------------
# User that dmapd will run as, current user if undefined:
User=dmapd
------------------------
This will ensure that the daemon will run per default as the intended user "dmapd".

Once you show me a new package with this change, the package will be approved.

Comment 26 W. Michael Petullo 2010-02-18 00:36:00 UTC
Spec URL: http://www.flyn.org/SRPMS/dmapd.spec
SRPM URL: http://www.flyn.org/SRPMS/dmapd-0.0.23-1.fc12.src.rpm

New upstream version, set User= in dmapd.conf

Comment 27 Christian Krause 2010-02-18 22:00:38 UTC
Thanks for the new package. 

Please don't forget to upload the 0.0.23 tarball to http://www.flyn.org/projects/dmapd/ too.

All show-stoppers were solved.

-> APPROVED


Here are a couple of items I still don't like but which don't block the review:

- although the spec file is technically correct it would be better for further "maintainability" (also by other maintainers), if the standard order of the sections would by 100% followed

- the same applies for squeezing the scriptlets into a single line (opposite to Fedora's templates)

- all debug, warning and error messages should be written into a log file rather to stdout/stderr

Comment 28 W. Michael Petullo 2010-02-19 02:28:28 UTC
New Package CVS Request
=======================
Package Name: dmapd
Short Description: A server that provides DAAP and DPAP shares
Owners: mikep
Branches: F-11 F-12
InitialCC:

Comment 29 Jason Tibbitts 2010-02-19 18:37:38 UTC
CVS done (by process-cvs-requests.py).

Added an F-13 branch as well.

Comment 30 W. Michael Petullo 2010-02-19 20:46:16 UTC
Thanks to all.


Note You need to log in before you can comment on or make changes to this bug.