Bug 370561 - Review Request: bmpx - Beep Media Player eXperimental
Review Request: bmpx - Beep Media Player eXperimental
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-07 18:10 EST by Alexander Kahl
Modified: 2007-11-30 17:12 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-11-23 04:40:19 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Alexander Kahl 2007-11-07 18:10:28 EST
Spec URL: http://akahl.fedorapeople.org/bmpx.spec
SRPM URL: http://akahl.fedorapeople.org/bmpx-0.40.13-1.src.rpm
Description: BMPx is an audio player that features support for specifications like XDS DnD, XSPF and DBus. BMPx is highly interoperable and integrates well with other applications and a variety of desktop environments.

--

The original spec file was committed by Matthias Saou who abandoned it, see Bug 187351. I've updated to the latest release, did a cleanup and made the following changes:

- takeover of Matthias' work
- update to 0.40.13
- updated dependencies
- updated files
- removed patch
- added firefox plugin, bits adapted from beagle's spec
- removed useless mime database update
- adapted latest macros
- added modplug support

Firefox plugin installation/uninstallation is tested and works well. Same goes for the player itself. The only thing that fails to work is the HAL back end, at least for me it has been broken for several versions and my own debugging investigations have not revealed anything useful yet, any help is appreciated here. I'm going to contact upstream about this as I was unable to find a bug report on it so far.
Comment 1 Alexander Kahl 2007-11-08 14:32:35 EST
Updated Spec URL: http://akahl.fedorapeople.org/bmpx.spec
Updated SRPM URL: http://akahl.fedorapeople.org/bmpx-0.40.13-2.fc8.src.rpm

Changes:
- added missing dist macro to revision
Comment 2 Mamoru TASAKA 2007-11-15 09:37:46 EST
Well,

! License is actually strict GPLv2.

* First triggerscripts
  - Personally, I don't like triggerscripts.
    However, even if we ignore what I feel, current triggerscripts
    contains some problems.

  1. /usr/share/doc/rpm-4.4.2.2/triggers says the order of the scriptlets
     when one package is upgrade is:
--------------------------------------------------------
  new-%pre      for new version of package being installed
  ...           (all new files are installed)
  new-%post     for new version of package being installed

  any-%triggerin (%triggerin from other packages set off by new install)
  new-%triggerin
  old-%triggerun
  any-%triggerun (%triggerun from other packages set off by old uninstall)

  old-%preun    for old version of package being removed
  ...           (all old files are removed)
  old-%postun   for old version of package being removed

  old-%triggerpostun
  any-%triggerpostun (%triggerpostun from other packages set off by old un
                install)
--------------------------------------------------------
    So, (if I understand this correctly) when firefox is upgraded,
    - First (any)-%triggerin of bmpx against "new firefox" is executed.
      When this %triggerin ends, the contents of 
      %{_libdir}/%{name}/%{name}-plugin-path points to new
      firefox directory.
    - Next (any)-%triggerun of bmpx against "old firefox" is executed.
      At this stage, the files under the directory written in
      %{name}-plugin-path is removed, so as the result bmpx extension
      for "new" firefox is removed.

    ! By the way, if I am correct new install of bmpx should call
      %triggerin scripts of bmpx itself (new-%triggerin).

  2. Usually these method leaves unowned files.
--------------------------------------------------------
$ LANG=C rpm -qf
/usr/lib/firefox-2.0.0.9/extensions/{bc3572da-daf9-435d-a8a6-33cc20fe4533}
file /usr/lib/firefox-2.0.0.9/extensions/{bc3572da-daf9-435d-a8a6-33cc20fe4533}
is not owned by any package
--------------------------------------------------------
     Moreover, %{_libdir}/%{name}/%{name}-plugin-path is not owned
     either.

     I think the proper way is to rebuild bmpx every time firefox
     is upgraded, or to separate firefox extension related files
     from bmpx (and submit a new review request for bmpx-mozextension,
     for example) if you don't want to rebuild whole bmpx.

- For generic issues:
* koji build
  - -2 does not build.
    http://koji.fedoraproject.org/koji/taskinfo?taskID=236212
    Note: basically, %_iconsdir is not defined.
----------------------------------------------------------
[tasaka1@localhost ~]$ grep iconsdir /etc/rpm/macros.*
.........
/etc/rpm/macros.jpackage:%_iconsdir     %{_datadir}/icons
.........
[tasaka1@localhost ~]$ rpm -qf /etc/rpm/macros.jpackage
jpackage-utils-1.7.3-1jpp.3.fc8
----------------------------------------------------------

* Timestamps
  - Please use "-p" option when you use "cp" or "install" commands
  - Also, try to add 'INSTALL="install -p"' option to "make install"
    if this keeps timestamps on name png file, header files or so
    (I guess yes).

* Exclude
  - It seems many .la files are installed under %_libdir/bmpx/plugins/taglib.
    Check if this is correct.
  ! Note:
    I prefer not to use %exclude but really to "remove" unneeded files
    by the time %install finishes.

* %post scriptlet
----------------------------------------------------------
if [ -x %{_bindir}/gtk-update-icon-cache ]; then
	update-desktop-database &>/dev/null || :
fi
----------------------------------------------------------
  - Perhaps this is some copy/paste mistake.

* Files entry
  - Are Makefile.* under %_defaultdocdir really needed?

* -devel package?
  - Well, why is this -devel package needed?
    bmpx does not contain any libraries in ldconfig default paths,
    and -devel package does not contain any symlinks for libraries.

    As far as I see this package, -devel package is completely unneeded.
Comment 3 Alexander Kahl 2007-11-18 18:14:45 EST
Updated SPEC URL: http://akahl.fedorapeople.org/bmpx.spec
Updated SRPM URL: http://akahl.fedorapeople.org/bmpx-0.40.13-3.fc8.src.rpm

Changes:
- split up firefox extension
- removed trigger scripts
- install and handle documentation properly
- replaced non-standard icon macros
- remove libtool archives instead of excluding them
- extended copy and install commands with timestamp preservation
- rectified desktop database and icon cache update handling
Comment 4 Alexander Kahl 2007-11-18 18:29:11 EST
(In reply to comment #2)
> Well,
> 
> ! License is actually strict GPLv2.
Yes - did I miss anything here?

> * First triggerscripts
>   - Personally, I don't like triggerscripts.
>     However, even if we ignore what I feel, current triggerscripts
>     contains some problems.
[...]
I've considered this a lot after reading your remarks about triggerscripts and
think you are right, if there's any chance to get around triggerscripts one is
usually right to choose that way, they're easily likely to break things and not
easy to grasp in the first place.
 
>      I think the proper way is to rebuild bmpx every time firefox
>      is upgraded, or to separate firefox extension related files
>      from bmpx (and submit a new review request for bmpx-mozextension,
>      for example) if you don't want to rebuild whole bmpx.
The updated spec above creates such an extension, I've nailed the Firefox
compatible version to 2.0.0.9 however I think on the long run we'd be better off
to adapt the current Firefox plugin handling in a version independent directory
for extensions, i.e. we have
/usr/lib[64]/mozilla/plugins for plugins and should use
/usr/lib[64]/mozilla/extensions for extensions.

Do you think I should file a bug report for this issue?


> - For generic issues:
> * koji build
>   - -2 does not build.
>     http://koji.fedoraproject.org/koji/taskinfo?taskID=236212
>     Note: basically, %_iconsdir is not defined.
[...]
Fixed in -3
 
> * Timestamps
>   - Please use "-p" option when you use "cp" or "install" commands
>   - Also, try to add 'INSTALL="install -p"' option to "make install"
>     if this keeps timestamps on name png file, header files or so
>     (I guess yes).
Fixed in -3

> * Exclude
>   - It seems many .la files are installed under %_libdir/bmpx/plugins/taglib.
>     Check if this is correct.
>   ! Note:
>     I prefer not to use %exclude but really to "remove" unneeded files
>     by the time %install finishes.
Fixed in -3

> * %post scriptlet
> ----------------------------------------------------------
> if [ -x %{_bindir}/gtk-update-icon-cache ]; then
> 	update-desktop-database &>/dev/null || :
> fi
> ----------------------------------------------------------
>   - Perhaps this is some copy/paste mistake.
You are right it is a copy/paste mistake, also fixed in -3

> * Files entry
>   - Are Makefile.* under %_defaultdocdir really needed?
No, my fault, sorry! Fixed in -3

> * -devel package?
>   - Well, why is this -devel package needed?
>     bmpx does not contain any libraries in ldconfig default paths,
>     and -devel package does not contain any symlinks for libraries.
> 
>     As far as I see this package, -devel package is completely unneeded.
My opinion differs here. While the -devel subpackge contains no libraries so
far, all files contained within, i.e. header files and the pkgfile are only
useful for bmpx plugin developers and totally useless for pure end users.
However so far I've been unable to find any clarifying guidelines about -devel
subpackages' contents, if you don't know any neither we should use our common
sense here.
Comment 5 Mamoru TASAKA 2007-11-19 08:03:29 EST
Before checking -3:

(In reply to comment #4)
> (In reply to comment #2)
> > Well,
> > 
> > ! License is actually strict GPLv2.
> Yes - did I miss anything here?
  No.. it is all right.

> >      I think the proper way is to rebuild bmpx every time firefox
> >      is upgraded, or to separate firefox extension related files
> >      from bmpx (and submit a new review request for bmpx-mozextension,
> >      for example) if you don't want to rebuild whole bmpx.
> The updated spec above creates such an extension, I've nailed the Firefox
> compatible version to 2.0.0.9 however I think on the long run we'd be better off
> to adapt the current Firefox plugin handling in a version independent directory
> for extensions, i.e. we have
> /usr/lib[64]/mozilla/plugins for plugins and should use
> /usr/lib[64]/mozilla/extensions for extensions.
> 
> Do you think I should file a bug report for this issue?
  If firefox comes to handle such directories it is preferable for
  everyone, so you can file against firefox if you want.
  


> > * -devel package?
> >   - Well, why is this -devel package needed?
> >     bmpx does not contain any libraries in ldconfig default paths,
> >     and -devel package does not contain any symlinks for libraries.
> > 
> >     As far as I see this package, -devel package is completely unneeded.
> My opinion differs here. While the -devel subpackge contains no libraries so
> far, all files contained within, i.e. header files and the pkgfile are only
> useful for bmpx plugin developers and totally useless for pure end users.
  Okay, this is a valid reason.
Comment 6 Mamoru TASAKA 2007-11-19 08:56:41 EST
Well, then for 0.40.13-3:

* Dependency for -devel package
  - First on %_libdir/pkgconfig/bmp-2.0.pc:
------------------------------------------------------
Requires: dbus-1 dbus-glib-1 glib-2.0
------------------------------------------------------
    This means that bmp-2.0.pc requires dbus-1.pc, dbus-glib-1.pc
    and glib-2.0.pc, so "Requires: dbus-glib-devel" is needed
    for bmpx-devel
    - both dbus-devel, glib2-devel is required by dbus-glib-devel.

  - Then for header files:
------------------------------------------------------
$ grep -h 'include ' `rpm -ql bmpx-devel | grep -v /usr/share/doc` | sort | uniq
#  include <config.h>
#include "bmp/types/types-basic.hh"
#include "bmp/types/types-database.hh"
#include "bmp/types/types-library.hh"
#include "bmp/types/types-ui.hh"
#include "minisoup.hh"
#include <boost/cstdint.hpp>
#include <boost/format.hpp>
#include <boost/optional.hpp>
#include <boost/shared_ptr.hpp>
#include <boost/tuple/tuple.hpp>
#include <boost/tuple/tuple_comparison.hpp>
#include <boost/variant.hpp>
#include <ctime>
#include <glibmm.h>
#include <glibmm/ustring.h>
#include <glibmm/ustring.hh>
#include <gtkmm.h>
#include <gtkmm/uimanager.h>
#include <map>
#include <queue>
#include <set>
#include <string>
#include <vector>
--------------------------------------------------------
    You have to add the corresponding devel packages as "Requires"
    to bmpx-devel.
    (For example, #include <boost/cstdint.hpp> means bmpx-devel should
     have "Requires: boost-devel")
Comment 7 Alexander Kahl 2007-11-20 11:38:27 EST
Updated Spec URL: http://akahl.fedorapeople.org/bmpx.spec
Updated SRPM URL: http://akahl.fedorapeople.org/bmpx-0.40.13-4.fc8.src.rpm

Changes:
- install missing minisoup header file
- updated -devel dependencies
Comment 8 Alexander Kahl 2007-11-20 11:43:38 EST
(In reply to comment #5)
>   If firefox comes to handle such directories it is preferable for
>   everyone, so you can file against firefox if you want.
Bug 392211

(In reply to comment #6)
> Well, then for 0.40.13-3:
> [...]
>     You have to add the corresponding devel packages as "Requires"
>     to bmpx-devel.
>     (For example, #include <boost/cstdint.hpp> means bmpx-devel should
>      have "Requires: boost-devel")

Done. It seems like their install target misses the file minisoup.hh referenced
locally in types/types-network.hh so I added it to the package and also updated
dependencies (libsoup-devel) accordingly.
Comment 9 Mamoru TASAKA 2007-11-21 04:31:32 EST
(In reply to comment #8)
> (In reply to comment #6)
> > Well, then for 0.40.13-3:
> > [...]
> >     You have to add the corresponding devel packages as "Requires"
> >     to bmpx-devel.
> >     (For example, #include <boost/cstdint.hpp> means bmpx-devel should
> >      have "Requires: boost-devel")
> 
> Done. It seems like their install target misses the file minisoup.hh referenced
> locally in types/types-network.hh 

Actually.

----------------------------------------------------------------
     This package (bmpx) is APPROVED by me
----------------------------------------------------------------
Comment 10 Alexander Kahl 2007-11-21 09:25:00 EST
New Package CVS Request
=======================
Package Name:      bmpx
Short Description: Beep Media Player eXperimental
Owners:            akahl
Branches:          F-8
InitialCC: 
Cvsextras Commits: yes
Comment 11 Kevin Fenzi 2007-11-22 14:43:09 EST
cvs done.
Comment 12 Alexander Kahl 2007-11-23 04:40:19 EST
All builds successful.

Thank you Mamoru!

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