Bug 177818

Summary: Review Request: adplug
Product: [Fedora] Fedora Reporter: Linus Walleij <triad>
Component: Package ReviewAssignee: John Mahowald <jpmahowald>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhide   
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-05-16 07:15:01 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 163779, 177865, 178360    

Description Linus Walleij 2006-01-14 20:44:03 UTC
Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/adplug.spec
SRPM Name or Url: http://www.df.lth.se/~triad/krad/fc/adplug-1.5.1-2.20060101cvs.src.rpm

Description:

AdPlug is a free software, cross-platform, hardware independent AdLib
sound player library, mainly written in C++ and released under the
LGPL. AdPlug plays sound data, originally created for the AdLib (OPL2)
audio board, directly from its original format on top of an OPL2
emulator or by using the real hardware. No OPL chip is required for
playback. It supports various audio formats from MS-DOS AdLib trackers.

This is a CVS checkout, because only the CVS version supports the 
changes in the version of libbinio (by the same author) stored in
Fedora Extras. Since we (FE) initiated these changes in libbinio, we
should eat our own dogfood :-)

Comment 1 Michael Schwendt 2006-01-15 16:48:06 UTC
> $ rpmlint adplug-1.5.1-2.20060101cvs.src.rpm 
> W: adplug summary-ended-with-dot A software library for AdLib
> (OPL2) emulation.

Headline-style "Software library for AdLib (OPL2) emulation" commonly
considered better taste.

* %postun script is broken during package removal:

/sbin/ldconfig: relative path `0' used to build cache
error: %postun(adplug-1.5.1-2.20060101cvs.i386) scriptlet failed, exit status 1

Avoid '#' prefixed comments in your spec file below scriptlet sections.
The spec parser inserts them into your scriptlets, which makes them fail
badly when your scriptlet interpreter is not a shell, but /sbin/ldconfig.
See "rpm --query --scripts adplug".

* adplug-devel is missing "Requires: libbinio-devel".
  See headers which included binio.h and the dependency in the
  pkgconfig file.

* Making good use of "make test" either in %check section or not
  could turn out to be useful (adplug is not fully portable yet).

> # Remove doc "dir"
> rm -rf $RPM_BUILD_ROOT%{_infodir}/dir

/usr/share/info/dir is an index file, not a directory. Simple "rm -f"
ought to be enough.

Else it looks good. xmms-adplug is playing fine here (although I had
to tell it where to find libbinio headers).


Comment 2 Linus Walleij 2006-01-15 19:43:33 UTC
Thanks Michael, excellent work as usual. Here is a fixed SRPM which
hopefully works.

Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/adplug.spec
SRPM Name or Url:
http://www.df.lth.se/~triad/krad/fc/adplug-1.5.1-3.20060101cvs.src.rpm

(Adplay and xmms-adplug are hopefully next BTW)

Comment 3 Michael Schwendt 2006-02-04 01:08:03 UTC
* spec looks good
* dependencies 177865 and 178360 build with this
* basic run-time testing: works for me
* sources match upstream

However:

I'm unsure with regard to the stuff it does when running "adplugdb"
with option "-s".

Currently, it cannot create /usr/share/adplug/adplug.db because the
directory is missing. And it ought not try writing to a file there
anyway, as according to the FHS /usr/share is supposed to be for
read-only and platform-independent data.

Which leads to a question: Is adplug.db a platform-independent format?
The default configuration of adplugdb (and the manual page) would need
a correction to use directory /var/lib/adplug instead
(or var/lib/misc/adplug.db if only that single file is accessed in there).

Which leads to another question: If %dir /var/lib/adplug gets included,
what to do with /var/lib/adplug/adplug.db during package removal?
If %ghost'ed, user would lose the database file. If not %ghost'ed,
the directory could not be removed. I prefer the latter, since I
don't like removing user-created data files.

In either case, /usr/share/adplug{/adplug.db} as a default is questionable.


Comment 4 Linus Walleij 2006-02-04 08:42:33 UTC
Sent question to upstream:
http://sourceforge.net/tracker/index.php?func=detail&aid=1423945&group_id=28079&atid=392364

Comment 5 Linus Walleij 2006-02-16 21:51:14 UTC
Copied in from upstream:

Date: 2006-02-04 13:51
Sender: dynamite  Project Admin
Logged In: YES 
user_id=31101

adplug.db is a platform-independent file, even though it's
binary.

Although, indeed, it should maybe be placed into
/var/lib/adplug, as the super-user is able to modify it,
using adplugdb.

The policy for AdPlug frontends is to always merge the
system-wide database with the database in the user's home,
giving preference to user-made changes.

adplugdb is used by users to change their home databases.
The superuser can use it (with the -s option) to change the
system-wide database.

The system-wide database should provide a default source for
common database entries, any user should want to have in
their database, so it is probably very seldomly written to.

What does the FHS say to such files? Do they go to /var/lib?
If so, i will change it in AdPlug's CVS.

Comment 6 Michael Schwendt 2006-02-24 18:46:01 UTC
It's a matter of design. As /usr/share may be read-only, nobody could
create or modify the database file. Only the superuser of the master
server could. If the goal is to make it possible to distribute a shared
database file master copy to multiple machines in a network, /usr/share
would be right. Which solution to choose depends on where the global
database file comes from and how it is accessed at run-time. If it will
turn out to be something which is downloaded or updated at run-time,
/usr/share won't work on the individual machines. If it is something
which can be wrapped up in the RPM package as a default database, too,
/usr/share would work. Particularly, since users can override the db
contents from within their home dir.

Comment 7 Linus Walleij 2006-02-25 22:08:46 UTC
Updtream have agreed to change the path, but has this strange phenomenon
with autoconf, any ideas to help them understand what autoconf seem to
want to do?

----------------------------------

Date: 2006-02-18 00:29
Sender: dynamiteProject Admin
Logged In: YES 
user_id=31101

I agree with you and like to correct it, although the
following is strange: Autoconf has a set of variables to
refer to default installation directories, which i've been
using.

In order to correct this bug, i changed the default lookup
path for the adplug.db file from $(datadir)/adplug, which
resolves to /usr/local/share/adplug, to
$(sharedstatedir)/adplug, of which the Autoconf docs say for
$(sharedstatedir) that it is "The directory for installing
modifiable architecture-independent data", so exactly what
i'm looking for. This variable resolves to /usr/local/com,
which is a path i never heard of and didn't exist in my
system before.

Question is now if i should just set it to $(sharedstatedir)
and let the distribution maintainers (i.e. you) re-configure
the right path for their installation at build time.
According to the Autoconf docs, this is the right way to go.

I really wonder though what this /com directory is. I
don't see anything useful on google. Do you have any idea?

Comment 8 Linus Walleij 2006-02-25 22:35:34 UTC
This was one weird thing, but looking into the M4 sources
for autoconf you see it is defined like this:

AC_SUBST([sharedstatedir], ['${prefix}/com'])

When it installs for real it will go into /com
actually, and that is NOT in the Filesystem
Hierarchy Standard, nor on any system I have ever
seen (logged into a Solaris and a ULTRIX system
to verify this) it might be in the Modified
Directory Structure 
http://markhobley.yi.org/standards/mdirs/index.html
but I cannot access that right now.

I think this comes from the fact that some sysadmins
like to mount /usr/local on a network share, so when
you just compile and install a program it will
install in a location where all computers (running
same OS/distro) have access to the locally compiled
programs. Then a shared database across all computers
mounting /usr/local (or even NFS-mounting /com) will
make sense. (This is just an educated guess.)

/var however, is guaranteed to be on the actual 
local computer only. That is why there exist such
sick things as proprietary software actually 
installing itself into /var.

Even if /com is probably used by some
people in this world, I hardly believe it will be 
very many.

What you probably rather want to use is 
$(localstatedir) definied as:

AC_SUBST([localstatedir],  ['${prefix}/var'])

which will expand to "/usr/local/var" on local build but
when built for installation into root (--prefix=/) will
expand to /var of course.

Comment 9 Ralf Corsepius 2006-02-26 05:38:06 UTC
sharedstatedir=$prefix/com is GNU standard

cf. http://www.gnu.org/prep/standards/html_node/Directory-Variables.html

Comment 10 Linus Walleij 2006-02-26 18:48:34 UTC
Aha! So when something is in the GNU standard but not the FHS,
which one should win in the Fedora world...?

Comment 11 Linus Walleij 2006-02-26 20:12:52 UTC
Thinking about it, if upstream goes for the GNU standard, what would
we do? This package certainly cannot own /com, it's in the freaking
root dir! The only reasonable package to own /com would be "filesystem"
and the question is if filesystem would include it or only implement
FHS stuff.

Has anyone ever, ever seen a package use $prefix/com??

Comment 12 Michael Schwendt 2006-02-27 15:25:39 UTC
$prefix expands to /usr/local (or /usr) by default in our environment
and most distributions, so $prefix/com won't be in the root directory.
However, %{_sharedstatedir} in RPM macros expands to %{_prefix}/com
(and RPM %{_localstatedir} expands to just /var not %{_prefix}/var).

Comment 13 Ralf Corsepius 2006-02-27 16:34:51 UTC
(In reply to comment #10)
> Aha! So when something is in the GNU standard but not the FHS,
> which one should win in the Fedora world...?
None. Both standards cover different aspects. The GNU standards aim at
configuration portability, the FHS is the smallest common denominator that many
*nix vendors (almost) agree to. 

So, in this case it's a matter of "switching on brains" and "taking the plunge"
of drawing responsible decisions.

As far as, sharedstatedir is concerned, this is rarely used, because it's beyond
the scope of most packages, and beyond the reponsibility of vendors ("shared
statedir", architecture independent data, it is supposed to be shared between
different architectures and OSes). Implementing it definitely is a very tough
task, and therefore probably not covered by the FHS.








Comment 14 Linus Walleij 2006-02-28 17:40:52 UTC
OK upstream has opted for $(sharedstatedir) i.e. /usr/local/com so
I have opened bug 183370 to address this in filesystem. Until 
filesystem fix/rejects this I will leave /usr/local/com unowned in
the package.

Comment 15 Linus Walleij 2006-02-28 22:31:36 UTC
Here:

Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/adplug.spec
SRPM Name or Url:
http://www.df.lth.se/~triad/krad/fc/adplug-1.5.1-4.20060228cvs.src.rpm

This incorporates the move of adplugdb into $(sharedstatedir)/adplug
on my local build it resolves into /usr/com/adplug which rpmlint naturally
complains about but otherwise it's clean I think.

Comment 16 Linus Walleij 2006-03-04 22:04:34 UTC
Updated this once again:

Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/adplug.spec
SRPM Name or Url:
http://www.df.lth.se/~triad/krad/fc/adplug-1.5.1-5.20060228cvs.src.rpm

It now includes the latest adplug.db file from the adplug page as a
%config(noreplace) file since I believe anyone using adplug will probably
want this file too, and few will be bothered to download it on their
own.

Comment 17 Michael Schwendt 2006-03-16 11:45:28 UTC
Withdrawing my offer to review this.

As this would be the first package to use /usr/com which is not even
implemented by the FC filesystem, this should be discussed for a proper
packaging policy first.


Comment 18 Linus Walleij 2006-03-16 16:48:07 UTC
I'm putting in an explicit dependency on bug 183370 so we don't go
any further until the /usr/com issue has been resolved/rejected/third solution
by the filesystem package.

Comment 19 Linus Walleij 2006-03-23 22:58:36 UTC
Updated again with non-offending paths for database:

Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/adplug.spec
SRPM Name or Url:
http://www.df.lth.se/~triad/krad/fc/adplug-1.5.1-6.20060323cvs.src.rpm

These move the troublesome database into /var/adplug and it works
just fine. While not a preferable solution at large it doesn't hurt
this package, and I believe it is OK really.

Comment 20 Michael Schwendt 2006-03-24 14:22:46 UTC
FHS says:

Applications must generally not add directories to the top level of
/var. Such directories should only be added if they have some
system-wide implication, and in consultation with the FHS mailing list.

So, has this place been officially sanctioned? (i.e. as being
the official way to implement it in Fedora)

Comment 21 Linus Walleij 2006-03-24 20:16:16 UTC
No, hm, perhaps as this is just a single file we could put it right
into /var. Requires some patching but that'd be OK to do I guess.
I'll try it...

Comment 22 Linus Walleij 2006-03-31 20:50:16 UTC
Updated again with (this time hopefully) non-offending path for the database:

Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/adplug.spec
SRPM Name or Url:
http://www.df.lth.se/~triad/krad/fc/adplug-1.5.1-7.20060323cvs.src.rpm

Now it is directly in /var/lib/adplug.db


Comment 23 Linus Walleij 2006-04-06 19:15:49 UTC
Finally realized that /var/lib/adplug/adplug.db is the best place
to have is so as to avoid unecessary patching of upstream. This
was suggested earlier as well.

Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/adplug.spec
SRPM Name or Url:
http://www.df.lth.se/~triad/krad/fc/adplug-1.5.1-8.20060323cvs.src.rpm

I will remove dependencies on filesystem issues since this no
longer blocks this bug.

Comment 24 Linus Walleij 2006-05-06 19:50:35 UTC
Upstream finally released a version including our fixes:

Spec Name or Url: http://www.df.lth.se/~triad/krad/fc/adplug.spec
SRPM Name or Url: http://www.df.lth.se/~triad/krad/fc/adplug-2.0-1.src.rpm

(Bug 6 months+ hrm... Need to think of this. Perhaps nobody wants this.)

Comment 25 John Mahowald 2006-05-14 20:49:58 UTC
- rpmlint checks return:
E: adplug binary-or-shlib-defines-rpath /usr/bin/adplugdb ['/usr/lib64']

try passing --disable-rpath to configure


- package meets naming guidelines
- package meets packaging guidelines
- license (LGPL) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on FC5 (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

- devel package ok
- no .la files
- post/postun ldconfig ok
- devel requires base package n-v-r 

- db in /var/lib/adplug 

APPROVED

Comment 26 Linus Walleij 2006-05-16 07:13:35 UTC
OK, added --disable-rpath, imported and built successfully for 
devel, FC-4, FC-5.

THANKS JOHN, for making this possible!