Bug 370561

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

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.

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

(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.

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.

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")

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

(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.

(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
----------------------------------------------------------------

New Package CVS Request
=======================
Package Name:      bmpx
Short Description: Beep Media Player eXperimental
Owners:            akahl
Branches:          F-8
InitialCC: 
Cvsextras Commits: yes

cvs done.

All builds successful.

Thank you Mamoru!