Bug 608852 - Review Request: epris - a dbus service to listen to music
Summary: Review Request: epris - a dbus service to listen to music
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Gwyn Ciesla
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-06-28 18:45 UTC by Ratnadeep Debnath
Modified: 2012-03-06 19:42 UTC (History)
6 users (show)

Fixed In Version: epris-0.2-4.fc16
Clone Of:
Environment:
Last Closed: 2012-03-06 19:29:19 UTC
Type: ---
Embargoed:
gwync: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Ratnadeep Debnath 2010-06-28 18:45:17 UTC
Spec URL: http://rtnpro.fedorapeople.org/Packages/SPECS/epris.spec
SRPM URL: http://rtnpro.fedorapeople.org/Packages/SRPMS/epris-0.2-1.fc13.src.rpm
Description:
epris is a small command line based audio player. Unlike xmms2 or mpd, it 
uses GStreamer and DBus. It is written in Vala.

Comment 2 Rangeen Basu Roy Chowdhury 2010-06-28 19:57:03 UTC
Does it really not require anything to run. I think Gstreamer and dbus must be needed.

Comment 3 Ratnadeep Debnath 2010-06-29 06:48:42 UTC
(In reply to comment #2)
> Does it really not require anything to run. I think Gstreamer and dbus must be
> needed.    

I referred to totem.spec and found that epris should require gstreamer during runtime, if it requiures gstreamer-devel during building. I also added an obsoletes line to remove any older version of epris if present in the system.

I updated the the spec and srpm mentioned in comment#1

Comment 4 Ankur Sinha (FranciscoD) 2010-06-29 07:05:59 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > Does it really not require anything to run. I think Gstreamer and dbus must be
> > needed.    
> 
> I referred to totem.spec and found that epris should require gstreamer during
> runtime, if it requiures gstreamer-devel during building. I also added an
> obsoletes line to remove any older version of epris if present in the system.
> 
> I updated the the spec and srpm mentioned in comment#1    

hi Ratnadeep,

Since you've made changes to the SPEC, you need to bump the release version and add what you changed to the ChangeLog. Please provide links to the updated spec with the new release and the corresponding srpm.

Again, it's a NEEDSPONSOR,we can only provide you with some tips and comments, you need to look for a sponsor for this package to be approved

Ankur

Comment 5 Ratnadeep Debnath 2010-06-29 14:19:13 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)

> Since you've made changes to the SPEC, you need to bump the release version and
> add what you changed to the ChangeLog. Please provide links to the updated spec
> with the new release and the corresponding srpm.

I have updated the spec and srpm files with updated changelogs. The Urls are the same as used in comment#1

Comment 6 Ankur Sinha (FranciscoD) 2010-06-29 14:54:12 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> 
> > Since you've made changes to the SPEC, you need to bump the release version and
> > add what you changed to the ChangeLog. Please provide links to the updated spec
> > with the new release and the corresponding srpm.
> 
> I have updated the spec and srpm files with updated changelogs. The Urls are
> the same as used in comment#1    


How can that be?

"http://rtnpro.fedorapeople.org/Packages/SRPMS/epris-0.2-1.fc13.src.rpm"?

this should have a new release number??

Comment 7 Ratnadeep Debnath 2010-06-30 18:30:34 UTC
 
> this should have a new release number??    

Updated to epris-0.2-2.fc12.src.rpm

SRPM: http://rtnpro.fedorapeople.org/Packages/SRPMS/epris-0.2-2.fc13.src.rpm
SPEC: http://rtnpro.fedorapeople.org/Packages/SPECS/epris.spec

Comment 8 Fabian Affolter 2010-07-01 22:54:19 UTC
Some quick comments:

- You must add the doc files (README, TODO, THANKS)
- You should ask upstream to include a license file to the source and license header to the source files.

Comment 9 Michael Schwendt 2010-08-05 11:38:28 UTC
Please do test your packages a little bit yourself, too. :(  Reviewers are there to add another pair of eyes, but basically *you* are supposed to practise RPM packaging in accordance with Fedora's Packaging and Reviewing Guidelines.


> %define gstreamer_version 0.10
> %define dbus-glib_version 0.70

> BuildRequires: gstreamer-devel > gstreamer_version
> BUildRequires: dbus-glib-devel > dbus-glib_version

> Requires:	gstreamer > gstreamer_version

This won't work. Did it even build and install?
Above you defined two macros, but below you didn't use them. So:

  BuildRequires: gstreamer-devel > %{gstreamer_version}
  BUildRequires: dbus-glib-devel > %{dbus-glib_version}
  Requires: gstreamer > %{gstreamer_version}

Further, using '>' and not '>=' is somewhat unclear. GStreamer is still in the 0.10 series for a long time, so would 0.10 be sufficient? Or does it strictly need to be > 0.10? Notice that if the package %release value is not specified in such a versioned dependency, it is left out of RPM version comparison, too. For example, 0.10-2.fc14 would not be > 0.10


Btw, for safety reasons, notice:
https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define


> Obsoletes: epris < 0.2

A comment would be good here. There is no such "epris" provided in the Fedora package collection. *This* package is called "epris". What's the reason for the Obsoletes tag?


> %files
> %defattr(-,root,root,-)
> %doc

Absolutely no need to put an empty %doc there.

Comment 10 Felipe Contreras 2010-08-10 11:05:57 UTC
(In reply to comment #9)
> > %define gstreamer_version 0.10
> > %define dbus-glib_version 0.70
> 
> > BuildRequires: gstreamer-devel > gstreamer_version
> > BUildRequires: dbus-glib-devel > dbus-glib_version
> 
> > Requires:	gstreamer > gstreamer_version
> 
> This won't work. Did it even build and install?
> Above you defined two macros, but below you didn't use them. So:
> 
>   BuildRequires: gstreamer-devel > %{gstreamer_version}
>   BUildRequires: dbus-glib-devel > %{dbus-glib_version}
>   Requires: gstreamer > %{gstreamer_version}
> 
> Further, using '>' and not '>=' is somewhat unclear. GStreamer is still in the
> 0.10 series for a long time, so would 0.10 be sufficient? Or does it strictly
> need to be > 0.10?

There is no GStreamer > 0.10; should be >=.

I recommend:
BuildRequires: gstreamer-devel >= 0.10
BUildRequires: dbus-glib-devel >= 0.70

This is not needed:
>   Requires: gstreamer > %{gstreamer_version}

Because the binary would be linked to libgstreamer, so that would be automatic.

> > Obsoletes: epris < 0.2
> 
> A comment would be good here. There is no such "epris" provided in the Fedora
> package collection. *This* package is called "epris". What's the reason for the
> Obsoletes tag?

Yeah, this should go away.

Comment 11 Jason Tibbitts 2010-11-23 20:12:28 UTC
It has been several months and there have been several good comments since the last response from the submitter.  I will close this soon if there is no further progress.

Comment 13 Ratnadeep Debnath 2010-12-11 05:12:40 UTC
ping?

Comment 14 Fabian Affolter 2011-01-28 08:29:29 UTC
It seams that your are still looking for a sponsor. Please follow https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Comment 15 Ratnadeep Debnath 2011-02-04 10:44:53 UTC
(In reply to comment #14)
> It seams that your are still looking for a sponsor. Please follow
> https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

I got sponsored previously for two packages : wordroupz, python-keyring. I am now maintaining them in the Fedora repository. And the epris package is also correct. I don't why is it taking this long to get this package sponsored?

Comment 16 Jason Tibbitts 2011-02-04 16:57:35 UTC
Several things:

Packages aren't sponsored, people are.  Once you are a packager you (or your sponsor) should remove the FE-NEEDSPONSOR blockers from any other tickets you may have open.  Any packager can review this package; you do not need a sponsor to do it.

This package doesn't appear in the list of packages to be reviewed because it's still listed as being stalled.  You didn't clear the whiteboard when you posted your update, and then you sent a ping when it wasn't at all clear who you were pinging.  

I'll set the various fields appropriately; hopefully someone who is interested in this package will review it.

Comment 17 Gwyn Ciesla 2011-06-27 13:58:20 UTC
I'll take this:

Initially, Source URL is invalid:

epris.src: W: invalid-url Source0: http://epris.googlecode.com/files/epris-0.2.tar.gz HTTP Error 404: Not Found
The value should be a valid, public HTTP, HTTPS, or FTP URL.

Additionally, some non-issue spelling errors, and:

epris.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

epris.x86_64: W: no-manual-page-for-binary epr
Each executable in standard binary directories should have a man page.


Working on a mock build, will post results when complete.

Comment 18 Gwyn Ciesla 2011-06-27 14:20:42 UTC
Good:

- rpmlint checks return: Above^^^

Mock build good.

- package meets naming guidelines
- package meets packaging guidelines
- license ( ) OK, text in not in %doc, not included but matches website.
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- nothing in %doc affects runtime
- no need for .desktop file 

Fix docs and ideally manpage, but I'm not going to worry too much if other docs are present.

Comment 19 Gwyn Ciesla 2011-12-06 18:08:50 UTC
Ping?

Comment 20 Ratnadeep Debnath 2011-12-07 09:49:40 UTC
I have added the doc files. I have also written a man page for epris. But it seems that the make file isn't configured for man pages. I have patched the source directory to include the man file. Is there a way to copy the man file directly without patching the makefiles?

Comment 21 Gwyn Ciesla 2011-12-07 12:58:28 UTC
Sure, something like this (assuming 6, modify for 1, etc):

In install:

mkdir -p %{buildroot}%{_mandir}/man6
install -p -m644 <filename>.6 %{buildroot}%{_mandir}/man6/<filename>.6

In files:

%{_mandir}/man6/<filename>.6.gz

That should do it.

Comment 22 Ratnadeep Debnath 2011-12-11 12:30:52 UTC
Thanks for the tip. I have updated the SPEC file accordingly and it works. The only gotcha is that I could not find any other download URL for epris other than the current one (which shows HTTP 404 warning in rpmlint). Nevertheless, the URL works from a browser.

Here are the updated files:
SPEC URL: http://rtnpro.fedorapeople.org/Packages/SPECS/epris.spec
SRPM URL: http://rtnpro.fedorapeople.org/Packages/SRPMS/epris-0.2-4.fc16.src.rpm

Thanks,
Regards,
rtnpro

Comment 23 Gwyn Ciesla 2011-12-12 15:36:47 UTC
epris.spec:13: W: mixed-use-of-spaces-and-tabs (spaces: line 13, tab: line 4)
The specfile mixes use of spaces and tabs for indentation, which is a cosmetic
annoyance.  Use either spaces or tabs for indentation, not both.

I still get the bad URL error, but it's good, you might file an rpmlint bug.

So it's just the above, which is cosmetic. 

And you are sponsored now, correct?

Comment 24 Ratnadeep Debnath 2011-12-14 19:38:04 UTC
Thanks Jon. Now, it's time for SCM request.

Comment 25 Ratnadeep Debnath 2011-12-14 19:41:24 UTC
New Package SCM Request
=======================
Package Name: epris
Short Description: A dbus service to listen to music
Owners: rtnpro
Branches: F-14 F-15 F-16
InitialCC: rtnpro

Comment 26 Gwyn Ciesla 2011-12-14 20:02:30 UTC
A:  I didn't approve it yet.  Are you sponsored?

B:  You didn't set the fedora-cvs flag.

Comment 27 Ratnadeep Debnath 2011-12-15 18:30:59 UTC
Hi Jon,

>A:  I didn't approve it yet.  Are you sponsored?
Yes. I already maintain two packages at Fedora: python-keyring and wordgroupz. I am also approved in the Fedora Packager GIT Commit Group.

>B:  You didn't set the fedora-cvs flag.
setting that now.

Comment 28 Gwyn Ciesla 2011-12-15 18:47:03 UTC
Ok, thanks.  APPROVED.

In the future, wait for approval before making SCM requests.

Comment 29 Gwyn Ciesla 2011-12-15 18:48:00 UTC
Git done (by process-git-requests).

Comment 30 Fedora Update System 2011-12-31 17:16:41 UTC
epris-0.2-4.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/epris-0.2-4.fc15

Comment 31 Fedora Update System 2011-12-31 17:16:53 UTC
epris-0.2-4.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/epris-0.2-4.fc16

Comment 32 Fedora Update System 2011-12-31 20:24:27 UTC
epris-0.2-4.fc16 has been pushed to the Fedora 16 testing repository.

Comment 33 Gwyn Ciesla 2012-01-26 18:49:36 UTC
This can go to stable now, from the look of it.

Comment 34 Gwyn Ciesla 2012-02-28 15:53:07 UTC
Ping?

Comment 35 Ratnadeep Debnath 2012-03-01 06:34:28 UTC
Hi Jon,

How can it be done? AFAIK, for my previous packages, I didn't have to do anything for the package to move to stable.

Regards,
rtnpro

Comment 36 Gwyn Ciesla 2012-03-02 14:08:43 UTC
Login to bodhi, https://admin.fedoraproject.org/updates/, and mark each update as stable.

Comment 37 Fedora Update System 2012-03-06 19:29:19 UTC
epris-0.2-4.fc15 has been pushed to the Fedora 15 stable repository.

Comment 38 Fedora Update System 2012-03-06 19:42:11 UTC
epris-0.2-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.