Bug 187351

I'm trying to test it but i can' achieve to fill in the music library. I select
a directory containing ogg files (maybe all not tagged correctly) and try to add
these. The system seems to look at he files and the end add nothing.
Another thing, when you insert an audio cd and you try to read the files in, you
can't see the gnome-mounted cd.

Yeah, there are definitely issues with file adding, like playlist icons not
working, while right click is, or errors and playback stop when .jpg files are
found...
The program is still maturing, working "enough" for many people, and could
possibly benefit from a wider testing if it was available in Extras, so this
should be a review about the package more than about the bugs remaining in the
software... which should go here : http://bugs.beep-media-player.org/

Review for release 1.fc5:
* RPM name is OK
* Source bmpx-0.14.3.tar.bz2 is the same as upstream
* Builds fine in mock
* rpmlint of bmpx-devel looks OK
* rpmlint of bmpx looks OK
* File list of bmpx-devel looks OK
* INSERT RESULT OF RUN TEST

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* Spec file: some paths are not replaced with RPM macros
  (wiki: QAChecklist item 7)
* The BuildRoot must be cleaned at the beginning of %install
* BuildRequires: gettext is missing (required by the %find_lang macro)
* The package should contain the text of the license
  (wiki: Packaging/ReviewGuidelines)
* Desktop file: vendor should be fedora
  (wiki: PackagingGuidelines#desktop)
* Desktop file: the Categories tag should contain X-Fedora
  (wiki: PackagingGuidelines#desktop)
* Scriptlets: missing update-desktop-database
  (wiki: ScriptletSnippets)

Minor:
* Duplicate BuildRequires: dbus-devel (by hal-devel), gtk2-devel (by
libglade2-devel)

About the needs work :
- That build root is plain silly with the chroot builds we have nowadays, and is
the "preferred" according to the wiki. It makes mach builds fail, so it's not
the one I use.
- Please detail what paths aren't replaced, as I can't identify any at a first
glance.
- %{__rm} -rf %{buildroot} is already present right after %install...
- If you look closer, gettext-devel is build required already...
- COPYING added... it was missing from the tarballs some time ago IIRC.
- I understand the guidelines as "if there is no desktop files, then include one
and install it this way", but since the included desktop file is fine IMHO, I
didn't touch it. If my interpretation is wrong, alright, but the Wiki section
will need some more details.
- update-desktop-database calls will be added, good catch!

Updated package 0.14.3-2 available now.

Yeah, my fedora-qa script tends to give a lot of false positives, so it should
be double-checked by human eye. It's much worse that rpmlint in this regard.
I believe it can be useful though.

I've fixed the problem detecting gettext-devel. The "absolute path" check greps
for /usr and /var in the spec file, but I can't find that in the spec file from
svn. You'll find the updated script at the usual place.

Yes you are right. I had to put my human eye but i had not enough time so i
apologize. I will look more carefully to this next time.
(In reply to comment #4)
> About the needs work :
> - That build root is plain silly with the chroot builds we have nowadays, and is
> the "preferred" according to the wiki. It makes mach builds fail, so it's not
> the one I use.

Ok

> - Please detail what paths aren't replaced, as I can't identify any at a first
> glance.

You had already modified their : these was the lines about gtk-update-icon-cache

> - %{__rm} -rf %{buildroot} is already present right after %install...

you are right.

> - If you look closer, gettext-devel is build required already...

you are right also

> - COPYING added... it was missing from the tarballs some time ago IIRC.

Ok

> - I understand the guidelines as "if there is no desktop files, then include one
> and install it this way", but since the included desktop file is fine IMHO, I
> didn't touch it. If my interpretation is wrong, alright, but the Wiki section
> will need some more details.

I agree that the wiki section is not very clear about what to if a desktop file
is already included in the source file but when i had this question i modified
myself the desktop file to be according to the wiki. Maybe someone else could
give more details about this or i will ask the question to extras list to make
clarification in this wiki section

> - update-desktop-database calls will be added, good catch!
> 
> Updated package 0.14.3-2 available now.

I will see the new version when i will be bak home

(In reply to comment #4)
> About the needs work :
> - That build root is plain silly with the chroot builds we have nowadays,
What? The purpose is to protect users rebuilding the package. chroot's are
completely irrelevant wrt. this.

> It makes mach builds fail, so it's not the one I use.
Irrelevant for FE. FE's buildsystem and normal user environments count.
What you say only meany a bug in mach.

So let's just say that this (silly IMHO) %(id etc.) suffix to the build root is
the "preferred" according to the wiki, meaning it is encouraged to use it, but
not mandatory. I don't like it, so I don't use it.
BTW, it's totally useless when building with mock, which is the Extras way of
building packages :-)

Review for release 2:
* RPM name is OK
* Source bmpx-0.14.3.tar.bz2 is the same as upstream
* Builds fine in mock
* rpmlint of bmpx-devel looks OK
* rpmlint of bmpx looks OK
* File list of bmpx-devel looks OK
* File list of bmpx looks OK

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* Desktop file: vendor should be fedora
  (wiki: PackagingGuidelines#desktop)
* Desktop file: the Categories tag should contain X-Fedora
  (wiki: PackagingGuidelines#desktop)

Minor:
* Duplicate BuildRequires: dbus-devel (by hal-devel), gtk2-devel (by
libglade2-devel)
* Maybe you could add %{?dist} in the release. It could be usefull for next fedora
version (ie fc6)

$ rpmqfcheck.pl /home/qa/tmp/rpm/RPMS/bmpx-0.14.3-1.fc5.i386.rpm
Orphaned dir: /usr/share/dbus-1
Orphaned dir: /usr/share/locale/af
Orphaned dir: /usr/share/locale/af/LC_MESSAGES
Orphaned dir: /usr/share/locale/et

> $ rpmqfcheck.pl /home/qa/tmp/rpm/RPMS/bmpx-0.14.3-1.fc5.i386.rpm
> Orphaned dir: /usr/share/dbus-1
> Orphaned dir: /usr/share/locale/af
> Orphaned dir: /usr/share/locale/af/LC_MESSAGES
> Orphaned dir: /usr/share/locale/et

Since /usr/share/dbus-1/services/ is owned by dbus, I think you've found a bug
in dbus, which should own the /usr/share/dbus-1/ directory.
The af and et directories should be owned by glibc-common if I'm not mistaken,
as many other translations are already there too.

Did you file these bugs already, or do you want me to do it?

I haven't filed them. Btw, glibc-common does not include plain "af"
and "et" locales at all.

Looking further at all the other directories under /usr/share/locale/, I'm
really not sure what the consensus is, as MANY aren't owned by any package.
Probably a subject for the packaging-list before any bug gets filed.

IMO %find_lang or the glibc-common are things that should be looked into,
they're things that could provide a centralized fix for unowned locale dirs.

I'm not sure what to do here... bmpx has evolved quite fast. It's still somewhat
buggy from my experience, and the current 0.20.2 release optionally uses faad2
(MP4/AAC decoder...) to read some tags, so it cannot be shipped full-featured in
Extras.

This does seem wrong though, since using gstreamer for all the tags would be the
proper thing to do... *sigh*

It also has a dependency on a version of libnotify that isn't even in FC
development yet (see bug #191731).

Does it really require faad2 for reading the tags, or would libmp4v2 (bug
191036) be enough?

Thanks for the pointer. Indeed, only libmp4v2 should be enough. Still the
libnotify requirement left... then wait for legal... then continue with the
submission. Might be a while...

Well, I've clearly lost interest in maintaing bmpx for Extras : My perception is
that upstream doesn't really know what they want (they went for an xmms fork to
a Rhythmbox-like player written in C++...), and seem more interested in working
on beta releases with plenty of new features than releasing stable and solid
versions of their player.

I'll withdraw this submission and welcome anyone to re-submit bmpx if they're
interested in maintaining it.

That's a shame. bmpx has the nicest Last.fm client for Linux I've seen so far.
Are you still going to maintain it in your own repository?