Bug 678554

Summary: Review Request: iceplayer - a simple media player
Product: [Fedora] Fedora Reporter: Mike Ma <zhtx10>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED CANTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: akurtako, fedora-package-review, mario.blaettermann, martin.gieseking, notting, susi.lehtola, zhtx10
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-03-16 10:42:15 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:

Description Mike Ma 2011-02-18 07:04:33 EST
SRPM URL: http://repos.fedorapeople.org/repos/zhtx/iceplayer/fedora-14/SRPMS/iceplayer-4.0.3-20110214.src.rpm
Description: A simple media player. It can download lyrics and show them automatically, and supports themes, ID3, Equalizer and more.
Comment 1 Alexander Kurtakov 2011-02-18 07:28:24 EST
Everything in the spec file is supposed to be in American English AFAIK.
Comment 2 Mike Ma 2011-02-18 08:39:39 EST
Everything updated.
Comment 3 Mario Blättermann 2011-02-18 09:08:42 EST
$ rpmlint iceplayer*
iceplayer.src: W: summary-not-capitalized C iceplayer - A powerful media player for Linux
iceplayer.src: W: name-repeated-in-summary C iceplayer
iceplayer.src: E: no-changelogname-tag
iceplayer.src: W: invalid-license GPL
iceplayer.src: W: invalid-url Source0: iceplayer-4.0.3-20110214.tar.gz
1 packages and 0 specfiles checked; 1 errors, 4 warnings.

The "Summary:" shouldn't repeat the package name.

The given source URL is not existent. The current download URL is:
http://iceplayer.googlecode.com/files/iceplayer%20src%20%204.03-20110118.tar.gz
Note that we have whitespaces in the filename, escaped with %20.

The license description should match any from the Fedora packaging guidelines:
http://fedoraproject.org/wiki/Licensing
In our case, it has to be "GPLv3", unless in future versions of this application the authors will decide to go ahead with any successors of the GPLv3, then it has to be changed to GPLv3+.

The %changelog is still missing. Without it, we are unable to track changes to your package.

Remove the executable bit from any installed files, which don't need it. This affects the files in %doc and the files in %{_datadir}.

Don't add empty files to %docs. README and NEWS are currently empty.

Use consistenly macros. The %files section should be:

%defattr (-,root,root)
%doc COPYING AUTHORS
%{_bindir}/*
%{_datadir}/%{name}/
%{_datadir}/applications/%{name}.desktop

Some "BuildRequires:" are missing. The package depends on GTK2 and GStreamer. That's why you have to define the following:

BuildRequires: gtk2-devel gstreamer-devel

The latter points to the required gstreamer-0.10 development package.


Generally, you shouldn't leave any comments in languages other than American English in the spec file. The Chinese ones are not really useful.
Comment 4 Mario Blättermann 2011-02-18 09:13:23 EST
Referring to your updated package:

The "Add later" comment to the %changelog section is odd. What do you think what's a changelog for? It is here for tracking the changes, not only to fulfill the packaging rules. For any change in your package, you have to add a changelog entry and to screw up the version number.
Comment 5 Martin Gieseking 2011-02-18 09:39:15 EST
First of all, is this your first Fedora package submission? I can't find your email address in the packager group. If so, please add FE-NEEDSPONSOR to the Blocks field above and have a look at the following pages for further information:

http://fedoraproject.org/wiki/PackageMaintainers/Join
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Please also enter your real name in the Bugzilla preferences.

Some additional comments on your package:

- drop the initial %define

- the Release field should be something like %{X}%{?dist} where %{X} is the 
  number to increase every time you provide a new revision of your SRPM.
  You may also add the date to the release field: %{X}.20110214%{?dist}
  To simplify packaging, add 
  %global rev 20110214
  at the beginning of the spec file and use the macro %{rev} everywhere the
  date is required.

- libnotify-devel is also required to build the package

- add %{_smp_mflags} to "make" in order to enable parallel builds

- replace %defattr (-,root,root) with %defattr (-,root,root,-)
Comment 6 Mario Blättermann 2011-02-18 09:40:38 EST
Just seen there's really a source tarball which almost matches the defined source:

http://iceplayer.googlecode.com/files/iceplayer_4.03-20110214_%E6%BA%90%E7%A0%81_%E6%83%85%E4%BA%BA%E8%8A%82%E5%BF%AB%E4%B9%90.tar.gz

This is created by copying the link target to the clipboard and paste it here. No idea if it works in the spec file.

Don't know whether it is possible to use such names. It's actually unusual to use Chinese characters in filenames. Perhaps you should ask the developers to provide tarballs with ASCII names. Well, creating a MD5 checksum works for the time being.
Comment 7 Mike Ma 2011-02-18 10:32:44 EST
(In reply to comment #3)
> $ rpmlint iceplayer*
> iceplayer.src: W: summary-not-capitalized C iceplayer - A powerful media player
> for Linux
> iceplayer.src: W: name-repeated-in-summary C iceplayer
> iceplayer.src: E: no-changelogname-tag
> iceplayer.src: W: invalid-license GPL
> iceplayer.src: W: invalid-url Source0: iceplayer-4.0.3-20110214.tar.gz
> 1 packages and 0 specfiles checked; 1 errors, 4 warnings.
> 
> The "Summary:" shouldn't repeat the package name.
> 
> The given source URL is not existent. The current download URL is:
> http://iceplayer.googlecode.com/files/iceplayer%20src%20%204.03-20110118.tar.gz
> Note that we have whitespaces in the filename, escaped with %20.
This URL is not a tgz maked by "make dist".
Can't use it to make a rpm currently.
I have uploaded the valid tgz to fedorapeople.org.

> 
> The license description should match any from the Fedora packaging guidelines:
> http://fedoraproject.org/wiki/Licensing
> In our case, it has to be "GPLv3", unless in future versions of this
> application the authors will decide to go ahead with any successors of the
> GPLv3, then it has to be changed to GPLv3+.
Changed to GPLv3

> 
> The %changelog is still missing. Without it, we are unable to track changes to
> your package.
Added.

> 
> Remove the executable bit from any installed files, which don't need it. This
> affects the files in %doc and the files in %{_datadir}.
> 
> Don't add empty files to %docs. README and NEWS are currently empty.
> 
> Use consistenly macros. The %files section should be:
> 
> %defattr (-,root,root)
> %doc COPYING AUTHORS
> %{_bindir}/*
> %{_datadir}/%{name}/
> %{_datadir}/applications/%{name}.desktop
Changed.

> 
> Some "BuildRequires:" are missing. The package depends on GTK2 and GStreamer.
> That's why you have to define the following:
> 
> BuildRequires: gtk2-devel gstreamer-devel
> 
> The latter points to the required gstreamer-0.10 development package.
Added.

> 
> 
> Generally, you shouldn't leave any comments in languages other than American
> English in the spec file. The Chinese ones are not really useful.
Changed.

Please try.
SRPM URL: http://repos.fedorapeople.org/repos/zhtx/iceplayer/fedora-14/SRPMS/iceplayer-4.0.3-1.src.rpm

TGZ URL: http://ekd123.fedorapeople.org/iceplayer-4.0.3-20110214.tar.gz
Comment 8 Susi Lehtola 2011-02-18 10:34:07 EST
Where's the spec file?

Please input your full name in Bugzilla.
Comment 9 Mike Ma 2011-02-18 10:42:30 EST
(In reply to comment #8)
> Where's the spec file?
> 
> Please input your full name in Bugzilla.

There is the specfile.
http://ekd123.fedorapeople.org/iceplayer.spec
Comment 10 Susi Lehtola 2011-02-18 10:48:12 EST
Increment the release tag whenever you make changes to the spec file, and make a corresponding entry in the changelog.

Otherwise it is impossible for other people to see what has been done and at what stage.
Comment 11 Mike Ma 2011-02-18 10:59:46 EST
(In reply to comment #10)
> Increment the release tag whenever you make changes to the spec file, and make
> a corresponding entry in the changelog.

I know. Changed it to 5. But I think it's unnecessary to edit changelog section.
Comment 12 Mike Ma 2011-02-19 00:31:40 EST
Sorry, I am a new fedora packager.
What should I do next?
Comment 13 Mario Blättermann 2011-02-19 18:08:53 EST
The changelog is not for changes to the packaged application itself. It is for your changes to the package, actually. We expect entries such as the following:

- Added BuildRequires
- Removed empty files from %%doc

That's what we really need here. And every time you change the spec file, please add an appropriate changelog entry and increase the version number.
Comment 14 Mike Ma 2011-03-03 00:42:46 EST
I'm sorry, I almost forgot there is a review.

OK, I removed empty files and added BuildRequires.
there is the new specfile: http://ekd123.fedorapeople.org/iceplayer.spec

please check.

Thanks for all!
Comment 15 Michael Schwendt 2011-03-07 15:47:31 EST
The reviewers' comments about the spec %changelog are true. You are supposed to add your own entries - also during review. If you adapt a spec file from a package made by somebody else, make that clear by adding a first changelog entry with your full name and email address.
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
Comment 16 Mike Ma 2011-03-08 00:22:38 EST
OK. Added them.
Comment 17 Mike Ma 2011-03-12 06:12:33 EST
Just not,
I tried rpmlint.
No warnings or errors.
Is there any SPONSOR? I need you very much...
Comment 18 Mike Ma 2011-03-12 06:13:28 EST
(In reply to comment #17)
> Just not,
> I tried rpmlint.
> No warnings or errors.
> Is there any SPONSOR? I need you very much...

Wrong... Just now..
Comment 19 Mike Ma 2011-03-16 10:42:15 EDT
I have re-opened a Review about this..
Thanks for all helps!