Bug 754698 (idjc) - Review Request: idjc - DJ application with streaming capabilities
Summary: Review Request: idjc - DJ application with streaming capabilities
Keywords:
Status: CLOSED ERRATA
Alias: idjc
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ismael Olea
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 704484 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-11-17 12:24 UTC by Nikos Roussos
Modified: 2012-01-04 01:57 UTC (History)
4 users (show)

Fixed In Version: idjc-0.8.6-4.fc16
Clone Of:
Environment:
Last Closed: 2011-12-07 21:22:04 UTC
Type: ---
Embargoed:
ismael: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Nikos Roussos 2011-11-17 12:24:03 UTC
Spec URL: http://comzeradd.fedorapeople.org/specs/idjc.spec
SRPM URL:
http://repos.fedorapeople.org/repos/comzeradd/autoverse/fedora-16/SRPMS/idjc-0.8.5-1.fc16.src.rpm
Description: It's a two panel DJ application, with automatic cross-fading,
Icecast streaming and it uses jack as a back-end.

rpmlint idjc-0.8.6-1.fc16.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 1 Nikos Roussos 2011-11-17 12:24:25 UTC
*** Bug 704484 has been marked as a duplicate of this bug. ***

Comment 2 Ismael Olea 2011-11-21 14:22:45 UTC
* The SPEC file and the included in the SRC are not the same

* License:        GPLv2

License should be GPLv2+ as you can check in COPYING: 

    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
    the Free Software Foundation; either version 2 of the License, or
    (at your option) _any later_ version.

* Are you a Fedora contributor yet?

Comment 3 Nikos Roussos 2011-11-22 15:25:52 UTC
Sorry about that, I accidently linked the old version

Here are new files. Fixed the LICENSE issue
spec: http://comzeradd.fedorapeople.org/specs/idjc.spec
srpm: http://repos.fedorapeople.org/repos/comzeradd/autoverse/fedora-16/SRPMS/idjc-0.8.6-2.fc16.src.rpm

Yes I'm a Fedora Package Maintainer. A fairly new one :)
https://admin.fedoraproject.org/updates/user/comzeradd

Comment 4 Ismael Olea 2011-11-22 23:48:34 UTC
(In reply to comment #3)

> http://repos.fedorapeople.org/repos/comzeradd/autoverse/fedora-16/SRPMS/idjc-0.8.6-2.fc16.src.rpm

fine
 
> Yes I'm a Fedora Package Maintainer. A fairly new one :)
> https://admin.fedoraproject.org/updates/user/comzeradd

That's nice :-)

Some suggestions:

* please extend the description to be more informative; now is too short; in this sense, maybe could be significative to explicit which formats are supported, expecially bc the Fedora packaging doesn't support some popular ones (correct me if I'm wrong)

* accordingly, IMHO would be appropiated to add a README.Fedora with the format support information explained

* I see idjc runs over JACK, AFAIK pulseaudio is the standard sound server in Fedora (I really only use GNOME so I ignore the details with the other desktop), so, is there any practical way to add the JACK integration with Pulseaudio? Maybe something launched by a shell script or equivalent... or just adding pulseaudio-module-jack as Requires: tag 

* Have you considered to include the html documentation?

* Should be important to add a Requires: tag for icecast?

Checking list:

* Group: tag is wrong, use Applications/Multimedia

* the idjc.desktop file should include at least the AudioVideo category

* Why you remove the Application category from idjc.desktop?




PD: Completely out of this review, maybe you'll like to contrib a full featured idjc to RPM Fusion (something like idjc-freeworld)

Comment 5 Christoph Wickert 2011-11-23 00:10:53 UTC
(In reply to comment #4)
> * Why you remove the Application category from idjc.desktop?

Because it is no longer a valid category form http://standards.freedesktop.org/menu-spec/latest/apa.html and desktop-file-[install|validate] will complain.

Comment 6 Nikos Roussos 2011-11-28 10:28:14 UTC
> * please extend the description to be more informative; now is too short; in
> this sense, maybe could be significative to explicit which formats are
> supported, expecially bc the Fedora packaging doesn't support some popular ones
> (correct me if I'm wrong)

You are right about this. I added a more detailed description. I don't see necessary to explicitly point all the supported formats, as it actual supports almost every free major audio format.

> * accordingly, IMHO would be appropiated to add a README.Fedora with the format support information explained

I think that upstream documentation already covers that.

> * I see idjc runs over JACK, AFAIK pulseaudio is the standard sound server in
> Fedora (I really only use GNOME so I ignore the details with the other
> desktop), so, is there any practical way to add the JACK integration with
> Pulseaudio? Maybe something launched by a shell script or equivalent... or just
> adding pulseaudio-module-jack as Requires: tag

Good idea! I could add pulseaudio-module-jack as dependency. It may not be necessary for the application to run, but it make the life of a Fedorian much more easier if he wishes to integrate it with other Pulseaudio apps.

> * Have you considered to include the html documentation?

done :)

> * Should be important to add a Requires: tag for icecast?

I don't think so. IDJC is just the client. Icecast could be on a different machine. Certainly not a dependency.


> Checking list:
> 
> * Group: tag is wrong, use Applications/Multimedia
> 
> * the idjc.desktop file should include at least the AudioVideo category

Being a streaming application I find the Applications/Internet category more appropriate.

> * Why you remove the Application category from idjc.desktop?

What Christoph said. It's no longer a valid category.

> PD: Completely out of this review, maybe you'll like to contrib a full featured idjc to RPM Fusion (something like idjc-freeworld)

Yeap. I plan to do so :)


New spec: http://comzeradd.fedorapeople.org/specs/idjc.spec
New SRPM: http://repos.fedorapeople.org/repos/comzeradd/autoverse/fedora-16/SRPMS/idjc-0.8.6-3.fc16.src.rpm

Comment 7 Ismael Olea 2011-11-28 12:42:14 UTC
(In reply to comment #6)
> You are right about this. I added a more detailed description. I don't see
> necessary to explicitly point all the supported formats, as it actual supports
> almost every free major audio format.

Description is now fine.

> 
> > * accordingly, IMHO would be appropiated to add a README.Fedora with the format support information explained
> 
> I think that upstream documentation already covers that.

Let me disagree on that. But my point is about to explicit the relevant differences on supported formats here. Just a very brief informational text like: «Due to the Fedora limitations on patents covering some popular formats, idjc Fedora package is restricted to support OGG, FLAC, SPEEX formats. Support of MP3, WMA is available at upstream sources». 


> Good idea! I could add pulseaudio-module-jack as dependency. It may not be
> necessary for the application to run, but it make the life of a Fedorian much
> more easier if he wishes to integrate it with other Pulseaudio apps.

Fine

 
> > * Have you considered to include the html documentation?
> 
> done :)

Good.

Please remove the Makefile* files such they are unnecessary


> > * Should be important to add a Requires: tag for icecast?
> 
> I don't think so. IDJC is just the client. Icecast could be on a different
> machine. Certainly not a dependency.

perfect

 
> > Checking list:
> > 
> > * Group: tag is wrong, use Applications/Multimedia

> Being a streaming application I find the Applications/Internet category more
> appropriate.

Well, Icecast itself has been tagged «Applications/Multimedia» in Fedora. Check for yourself: http://pkgs.fedoraproject.org/gitweb/?p=icecast.git;a=blob_plain;f=icecast.spec;hb=b4d27c3ef96f50ad07499fcc909a345489a2b436  :-)

please fix

> > 
> > * the idjc.desktop file should include at least the AudioVideo category

please fix this too; add any other category if needed at your consideration


> > * Why you remove the Application category from idjc.desktop?
> 
> What Christoph said. It's no longer a valid category.

fine


Please fix the mentioned issues. The proposal is almost ready.


> > PD: Completely out of this review, maybe you'll like to contrib a full featured idjc to RPM Fusion (something like idjc-freeworld)
> 
> Yeap. I plan to do so :)

Great!

Comment 8 Nikos Roussos 2011-12-04 20:45:42 UTC
Here are the new files:
http://comzeradd.fedorapeople.org/specs/idjc.spec
http://repos.fedorapeople.org/repos/comzeradd/autoverse/fedora-16/SRPMS/idjc-0.8.6-3.fc16.src.rpm

I changed the group to Multimedia and added an AudioVideo category on desktop file.

I added a short README.Fedora, but since I didn't find any documentation about that I'm not sure I did it the right way.

Comment 9 Ismael Olea 2011-12-05 17:47:34 UTC
(In reply to comment #8)

> I added a short README.Fedora, but since I didn't find any documentation about
> that I'm not sure I did it the right way.

Seems correct to me; you'll be able to fix it later if something appears to be wrong.

Review checklist:

+: OK
-: must be fixed
=: should be fixed (at your discretion)
?: Question or clairification needed
N: not applicable

MUST:
[+] rpmlint output: shown in comment: none
[+] follows package naming guidelines
[+] spec file base name matches package name
[+] package meets the packaging guidelines
[+] package uses a Fedora approved license: GPLv2+
[+] license field matches the actual license.
[+] license file is included in %doc: COPYING
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream: md5sum matches
[+] package builds on at least one primary arch: Tested F15 x86
[N] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires
[N] spec file handles locales properly
[N] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[+] no relocatable packages
[+] package owns all directories that it creates
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[N] large documentation in -doc
[N] no runtime dependencies in %doc
[N] header files in -devel
[N] static libraries in -static
[N] .so in -devel
[N] -devel requires main package
[+] package contains no libtool archives
[+] package contains a desktop file, uses desktop-file-install/validate
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[N] query upstream for license text
[=] description and summary contains available translations
[+] package builds in mock
[=] package builds on all supported arches: Tested x86
[+] package functions as described: 
[N] sane scriptlets
[N] subpackages require the main package
[N] placement of pkgconfig files
[+] file dependencies versus package dependencies
[+] package contains man pages for binaries/scripts

*** APPROVED ***

Congrats!

Comment 10 Nikos Roussos 2011-12-07 16:10:41 UTC
New Package SCM Request
=======================
Package Name: idjc
Short Description: DJ application with streaming capabilities
Owners: comzeradd
Branches: f15 f16
InitialCC:

Comment 11 Gwyn Ciesla 2011-12-07 16:21:27 UTC
Git done (by process-git-requests).

Comment 12 Fedora Update System 2011-12-07 21:47:58 UTC
idjc-0.8.6-4.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/idjc-0.8.6-4.fc16

Comment 13 Fedora Update System 2011-12-07 21:48:50 UTC
idjc-0.8.6-4.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/idjc-0.8.6-4.fc15

Comment 14 Fedora Update System 2012-01-04 01:54:09 UTC
idjc-0.8.6-4.fc15 has been pushed to the Fedora 15 stable repository.

Comment 15 Fedora Update System 2012-01-04 01:57:19 UTC
idjc-0.8.6-4.fc16 has been pushed to the Fedora 16 stable repository.


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