Bug 177818
Summary: | Review Request: adplug | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Linus Walleij <triad> |
Component: | Package Review | Assignee: | 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
> $ 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). 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) * 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. Sent question to upstream: http://sourceforge.net/tracker/index.php?func=detail&aid=1423945&group_id=28079&atid=392364 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. 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. 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? 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. sharedstatedir=$prefix/com is GNU standard cf. http://www.gnu.org/prep/standards/html_node/Directory-Variables.html Aha! So when something is in the GNU standard but not the FHS, which one should win in the Fedora world...? 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?? $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). (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. 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. 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. 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. 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. 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. 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. 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) 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... 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 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. 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.) - 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 OK, added --disable-rpath, imported and built successfully for devel, FC-4, FC-5. THANKS JOHN, for making this possible! |