Bug 583327 - Review Request: clementine - A music player and library organizer
Summary: Review Request: clementine - A music player and library organizer
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mattias Ellert
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 577601 581220 614692
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-04-17 21:21 UTC by Orcan Ogetbil
Modified: 2010-08-27 06:50 UTC (History)
5 users (show)

Fixed In Version: clementine-0.4.2-9.fc13
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-08-27 06:49:16 UTC
Type: ---
Embargoed:
mattias.ellert: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Orcan Ogetbil 2010-04-17 21:21:31 UTC
Spec URL: http://6mata.com:8014/review/clementine.spec
SRPM URL: http://6mata.com:8014/review/clementine-0.2-2.fc12.src.rpm
Description: 
Clementine is a modern music player and library organiser.
It is largely a port of Amarok 1.4, with some features rewritten to take
advantage of Qt4.


---
This is an initial attempt to build clementine on Fedora. It builds and runs fine. However, the way we patch out the bundled libraries and use the system libraries might change, depending on #581220, #582864, #577601.

Comment 1 Orcan Ogetbil 2010-05-07 08:31:53 UTC
New vercione! runs and works magnifico.

Spec URL: http://6mata.com:8014/review/clementine.spec
SRPM URL: http://6mata.com:8014/review/clementine-0.3-1.fc12.src.rpm

However, the package is so not ready with linking to qxt etc. I'll have time to sort things out next week. qtsingleapplication is not done with the review, we are not in a hurry.

Comment 2 Orcan Ogetbil 2010-05-09 00:23:08 UTC
Alright. I had a discussion with a clementine developer. 
   http://code.google.com/p/clementine-player/issues/detail?id=291

What he says is they made such API breaking modifications in qxt and qtsingleapplication that are not upstreamable. So far I have been using clementine-0.2 built against our own qxt and own-to-be qtsingleapplication and I didn't see any regressions. That is perhaps because I didn't try to use the functionality that is provided by these modified libraries. Or perhaps the modifications are made after 0.2 releasei I didn't have much time to play around with 0.3, I just verified it can do the basic things.

So, in particular, quoting the developer:

* qxt: changes are to add support for media keys (play, stop, etc.) in the global shortcut library.  I've included a private Qt header in there too, so these probably wouldn't get accepted upstream.

* qtsingleapplication: changes are twofold: there's an obscure compilation fix for cross-compilation with mingw in release mode, which we could send upstream, but the other one is a compatibility-breaking API change.

* universalchardet (new bundled library with 0.3): modified to remove its dependencies on Mozilla. It's not currently provided separately so the only solution is to bundle the source with clementine.

qtlsingleapplication and universalchardet are small libraries, so we won't waste too much space by keeping them bundled. qxt is large originally, but clementine only bundles a relatively small portion of it.

What do you folks need? Can we make an exception to allow these modified libraries in clementine?

Comment 3 Orcan Ogetbil 2010-05-09 00:24:48 UTC
> What do you folks need?

s/need/think/

Comment 4 Orcan Ogetbil 2010-06-03 02:42:45 UTC
Hi,
So what is the next step? Do I need to ask FESCO about the inclusion of these modified libraries in clementine?

Comment 5 Orcan Ogetbil 2010-07-17 17:18:02 UTC
After a lot of discussion with upstream, we got the 3rd party software patches backwards compatible except one. We now have command line options for cmake to link to the system libraries.

The one exception is qxt. I wrote a patch to use the system qxt. However, this means that the multimedia buttons on the keyboards will not work. Still much better than what we had previously.

Note that this package Buldrequires qtsingleapplication and qtiocompressor which are in updates-testing for the time being. It also requires a not-yet-built package: libprojectM. The current libprojectM package has a bug in it that causes clementine to crash. I notified its maintainer, and sent a patch.

SPEC: http://oget.fedorapeople.org/review/clementine.spec
SRPM: http://oget.fedorapeople.org/review/clementine-0.4.2-1.fc13.src.rpm

Comment 6 Orcan Ogetbil 2010-07-18 02:44:40 UTC
SPEC: http://oget.fedorapeople.org/review/clementine.spec
SRPM: http://oget.fedorapeople.org/review/clementine-0.4.2-2.fc13.src.rpm

This update fixes a segfault because of missing font paths when linked to the system projectM.

Comment 7 Orcan Ogetbil 2010-07-18 19:18:11 UTC
SPEC: http://oget.fedorapeople.org/review/clementine.spec
SRPM: http://oget.fedorapeople.org/review/clementine-0.4.2-3.fc13.src.rpm

Better, more portable patch for splitting qxt. Patch sent upstream.

Since all the dependencies are built and are in testing now, this package is ready to review.

Comment 8 Mattias Ellert 2010-07-20 14:51:53 UTC
Fedora Review clementine 2010-07-20

rpmlint output:

$ rpmlint clementine-0.4.2-3.fc14.src.rpm \
          clementine-0.4.2-3.fc14.x86_64.rpm \
          clementine-debuginfo-0.4.2-3.fc14.x86_64.rpm 
clementine.src: W: invalid-url Source0: http://clementine-player.googlecode.com/files/clementine-0.4.2.tar.gz HTTP Error 404: Not Found
clementine.x86_64: W: no-manual-page-for-binary clementine
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

Googlecode is infamous for triggering false 404 warnings in rpmlint.
Not quite sure why - wget seems to work without problem every time I try.


+ Package is named according to guidelines
+ Specfile is named after the package

- Guidelines say cmake projects should use "make VERBOSE=1":
  https://fedoraproject.org/wiki/Packaging/cmake

+ Package License tag GPLv3 and GPLv2+ is a Fedora approved license

- All the source files say "or (at your option) any later version", so it
  would make sense to say GPLv3+ and GPLv2+

The universalchardet (bundled code) is MPLv1.1 or GPLv2+ or LGPLv2+.
I guess we can choose to use the GPLv2+ in this case, which is alredy
covered in the tag.

+ License file (COPYING) is included as %doc (this is the GPLv3 text)
+ Specfile is written in legible English
+ Source matches upstream:

$ md5sum srpm/clementine-0.4.2.tar.gz clementine-0.4.2.tar.gz 
c6819b0d2a8324f1d686fb5a3b1d287b  srpm/clementine-0.4.2.tar.gz
c6819b0d2a8324f1d686fb5a3b1d287b  clementine-0.4.2.tar.gz

+ Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=2329614

- libprojectM-devel is listed twice as BuildRequires (once with a
  version and once without)


The build log says:
-- Building engines: gst
-- Skipping engines: vlc xine qt-phonon

So you are currently not building the xine engine - but you have
xine-lib-devel as a BuildRequires - is it needed in this case?

If you try to enable more engines you get a big warning:

> The following engines are NOT supported by clementine developers:
>  xine qt-phonon
>
> Don't post any bugs if you use them, fix them yourself!

So I guess you have a reason for not building them for Fedora.

Some other BuildRequires seems redundant too - did you remove those
that are only required to build the bundled libraries you do not build
anymore? E.g. glew-devel seems to only be used inside the libprojectM
code.


+ No locale files installed. (The translation files are mangled into
  the main binary by Qt.)
+ No shared libraries


This package must have been a nightmare w.r.t. bundled libraries. You
have done an excellent job unbundling the source. Very good job and
congratualations.


What remains is the universalchardet code. This is tricky. Did you
discuss this with some Fedora packaging people?

Googling a bit shows that this code is used by quite many projects,
and they all bundle it. It would be interesting to know how many
packages that exist in Fedora and bundle the code. I don't know any
reasonably fast way to figure that out though.

By doing a repoquery I found one package that installs the
universalchardet headers. In that package (codeblocks-devel) the code
is not compiled into a separate library, but bundled with a lot of
other stuff into a big library that has very many dependencies - so it
is not really a good option to use if you only want to use the
universalchardet. Installing the headerfiles for a bundled library the
way codeblocks-devel does seems wrong anyway.

In a perfect world, I think this would be best compiled as a shared
library in a separate package, having a common maintained codebase for
all users of the code. But you might have some arguments for treating
this case special.


? How do you ensure ownership of /usr/share/icons/hicolor/64x64/apps?

+ No duplicate files
+ Permissions are sane and %file has %defattr
+ Macros are used consistently in the Specfile
+ %doc is not runtime essential
+ No headerfiles
+ No static libraries
+ No libtool archive
+ .desktop file verified using desktop-file-validate
+ Package does not own other's directories
+ Installed files have valid UTF8 filenames

Comment 9 Mattias Ellert 2010-07-21 05:10:22 UTC
(In reply to comment #8)
> In a perfect world, I think this would be best compiled as a shared
> library in a separate package, having a common maintained codebase for
> all users of the code. But you might have some arguments for treating
> this case special.

Rather than creating a new source package, it might be a good idea to talk to the maintainers of the xulrunner package and ask them if that package could provide such a library.

Comment 10 Orcan Ogetbil 2010-07-21 05:12:17 UTC
(In reply to comment #8)
> Fedora Review clementine 2010-07-20
> 
Thanks a lot for the review.

> - Guidelines say cmake projects should use "make VERBOSE=1":
>   https://fedoraproject.org/wiki/Packaging/cmake
> 

That makes the build logs, as one can guess, verbose. clementine's logs were verbose by default, but I added it, just in case things change in the future.

> - All the source files say "or (at your option) any later version", so it
>   would make sense to say GPLv3+ and GPLv2+
> 

I changed it to GPLv3+ and GPLv2+

> - libprojectM-devel is listed twice as BuildRequires (once with a
>   version and once without)
> 

My sloppiness. Removed.

> So you are currently not building the xine engine - but you have
> xine-lib-devel as a BuildRequires - is it needed in this case?
> 

Nope, it is a leftover from my experiments. 

> If you try to enable more engines you get a big warning:
> 
> > The following engines are NOT supported by clementine developers:
> >  xine qt-phonon
> >
> > Don't post any bugs if you use them, fix them yourself!
> 
> So I guess you have a reason for not building them for Fedora.
> 

If I remember correctly, they use different engines on different OS'. gst was the default, and it works, so I kept it. :)

> Some other BuildRequires seems redundant too - did you remove those
> that are only required to build the bundled libraries you do not build
> anymore? E.g. glew-devel seems to only be used inside the libprojectM
> code.
> 

I think glew-devel is left from the days I was compiling the bundled libprojectM. I think that's the last one though.

> This package must have been a nightmare w.r.t. bundled libraries. You
> have done an excellent job unbundling the source. Very good job and
> congratualations.
> 

Yeah, it was a lot of work, constantly bugging the developers, trying to reason with other upstreams to have the patches accepted, etc. I am sure the review took you some time too. Thanks again for your patience.

> 
> What remains is the universalchardet code. This is tricky. Did you
> discuss this with some Fedora packaging people?
> 
> Googling a bit shows that this code is used by quite many projects,
> and they all bundle it. It would be interesting to know how many
> packages that exist in Fedora and bundle the code. I don't know any
> reasonably fast way to figure that out though.
> 
> By doing a repoquery I found one package that installs the
> universalchardet headers. In that package (codeblocks-devel) the code
> is not compiled into a separate library, but bundled with a lot of
> other stuff into a big library that has very many dependencies - so it
> is not really a good option to use if you only want to use the
> universalchardet. Installing the headerfiles for a bundled library the
> way codeblocks-devel does seems wrong anyway.
> 
> In a perfect world, I think this would be best compiled as a shared
> library in a separate package, having a common maintained codebase for
> all users of the code. But you might have some arguments for treating
> this case special.
> 

This library does not have a standalone release by itself. Hence there are no packages for it. People keep copying its code into their projects. I talked to a few people on fedora-devel on IRC. They say xulrunner, gnash etc, everybody is sweeping it under the carpet. We can always switch over if it becomes available as a package.

> 
> ? How do you ensure ownership of /usr/share/icons/hicolor/64x64/apps?
> 

By adding the relevant Requires :)

Here is my update:
SPEC: http://oget.fedorapeople.org/review/clementine.spec
SRPM: http://oget.fedorapeople.org/review/clementine-0.4.2-4.fc13.src.rpm

Changelog: 0.4.2-4
- Use: make VERBOSE=1
- License is GPLv3+ and GPLv2+
- Put BRs in alphabetical order
- Remove redundant BRs: glew-devel, xine-lib-devel, and
  the extra libprojectM-devel
- Add R: hicolor-icon-theme

Comment 11 Mattias Ellert 2010-07-21 06:04:10 UTC
(In reply to comment #10)
> This library does not have a standalone release by itself. Hence there are no
> packages for it. People keep copying its code into their projects. I talked to
> a few people on fedora-devel on IRC. They say xulrunner, gnash etc, everybody
> is sweeping it under the carpet. We can always switch over if it becomes
> available as a package.

I understand your point, and I will not block the review waiting for a universalchardet library to appear. As I said in my previous comment, I think the closest thing you could get to an upstream in Fedora is the xulrunner package (since this contains the full code tree from mozilla and not just a copy of this class). So any solution to the problem with bundled universalchardet code should at least involve the xulrunner maintainers.

But as I said, I will not block the review over this issue, so ...

... package approved.

Comment 12 Orcan Ogetbil 2010-07-21 14:17:59 UTC
Thanks Mattias, I will definitey contact xulrunner maintainers about this.

New Package CVS Request
=======================
Package Name: clementine
Short Description: A music player and library organizer
Owners: oget
Branches: F-12 F-13
InitialCC:

Comment 13 Kevin Fenzi 2010-07-23 21:32:24 UTC
CVS done (by process-cvs-requests.py).

Comment 14 Kalev Lember 2010-07-23 22:05:18 UTC
(In reply to comment #10)
> (In reply to comment #8)
> > - Guidelines say cmake projects should use "make VERBOSE=1":
> >   https://fedoraproject.org/wiki/Packaging/cmake
> > 
> 
> That makes the build logs, as one can guess, verbose. clementine's logs were
> verbose by default, but I added it, just in case things change in the future.

The reason why clementine's logs were already verbose is that the %cmake macro has -DCMAKE_VERBOSE_MAKEFILE=ON switch. When the guidelines were first approved %cmake macro didn't have this switch; it is something that was added later but it looks like people have forgotten to update the guidelines.

The spec file doesn't seem to run icon cache scriplets, but I think it should as it's putting files into %{_datadir}/icons/hicolor/. Also, one of the icons is named application-x-clementine.png which looks like it's an icon for application/x-clementine mime type. I didn't check the .desktop file if it really has mimetype entry, but if it does, then you should also add desktop-database updating scriplets.

Comment 15 Orcan Ogetbil 2010-07-23 22:11:00 UTC
ouch. 

I'll add the scriptlets. Thanks for pointing them out. And yes, the .desktop file has mimetypes

Comment 16 Fedora Update System 2010-07-25 14:08:26 UTC
clementine-0.4.2-5.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/clementine-0.4.2-5.fc13

Comment 17 Fedora Update System 2010-07-25 14:09:14 UTC
clementine-0.4.2-5.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/clementine-0.4.2-5.fc12

Comment 18 Fedora Update System 2010-07-27 02:28:06 UTC
clementine-0.4.2-5.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update clementine'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/clementine-0.4.2-5.fc12

Comment 19 Fedora Update System 2010-07-27 02:55:45 UTC
clementine-0.4.2-5.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update clementine'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/clementine-0.4.2-5.fc13

Comment 20 Fedora Update System 2010-08-06 20:55:08 UTC
clementine-0.4.2-7.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update clementine'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/clementine-0.4.2-7.fc13

Comment 21 Fedora Update System 2010-08-06 20:55:33 UTC
clementine-0.4.2-7.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update clementine'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/clementine-0.4.2-7.fc12

Comment 22 Fedora Update System 2010-08-07 23:21:02 UTC
clementine-0.4.2-8.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update clementine'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/clementine-0.4.2-8.fc12

Comment 23 Fedora Update System 2010-08-07 23:25:03 UTC
clementine-0.4.2-8.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update clementine'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/clementine-0.4.2-8.fc13

Comment 24 Fedora Update System 2010-08-27 06:49:02 UTC
clementine-0.4.2-9.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 25 Fedora Update System 2010-08-27 06:50:34 UTC
clementine-0.4.2-9.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.


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