Bug 198878

I forgot to write that this is my first package and I need a sponsor :)

== Not an official review as I'm not yet sponsored ==
   Mock build for rawhide i386 is sucessfull  

* MUST Items:
      - rpmlint shows errors as
        E: mutagen no-binary
        W: mutagen wrong-file-end-of-line-encoding 
/usr/share/doc/mutagen-1.5.1/TUTORIAL
        E: mutagen non-executable-script         
/usr/lib/python2.4/site-packages/mutagen/__init__.py 0644
  
      - dist tag is present.
      - The package is named according to the Package Naming Guidelines.
      - The spec file name matching the base package mutagen, in the
format mutagen.spec.
      - This package meets the Packaging Guidelines.
      - The spec file for the package MUST be legible.
      - The package is licensed with an open-source compatible license GPL.
      - This package includes License file COPYING.
      - This source package includes the text of the license in its own file,and
that file, containing the text of the license for the package is included in %doc.
      - The sources used to build the package matches the upstream source,
as provided in the spec URL. md5sum is correct (9ce5d5f14e02f2eabd919d6bdaebadbc)
      - This package successfully compiled and built into binary rpms for i386
architecture.
      - This package did not containd any ExcludeArch.
      - This package owns all directories that it creates. 
      - This package did not contain any duplicate files in the %files
listing.
      - This package  have a %clean section, which contains rm -rf
$RPM_BUILD_ROOT.
      - This package used macros.
      - Document files are included like COPYING, NEWS, README, TUTORIAL.
      - Package did NOT contained any .la libtool archives.

Also,
      * Source URL is present and working.
      * BuildRoot is correct BuildRoot:       
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

What you Need to Do:-
I think you dont need following line if you are building for only fc5
Requires:   python-abi = %(%{__python} -c "import sys ; print sys.version[:3]")
BuildRequires:  python-devel
 also try to clean %files section of SPEC as
%files
%defattr(-,root,root,-)
%doc COPYING NEWS README TUTORIAL
%{_bindir}/*
%{_mandir}/man*/*gz
%dir %{python_sitelib}/mutagen
%{python_sitelib}/mutagen/*.py
%{python_sitelib}/mutagen/*.pyc
%ghost %{python_sitelib}/mutagen/*.pyo

I removes those above Requires and modified %files as shown above and package
built without any problem.

For rpmlint errors, 
E: mutagen no-binary
 There is no mutagen named binary when i did python setup.py build in Source

W: mutagen wrong-file-end-of-line-encoding /usr/share/doc/mutagen-1.5.1/TUTORIAL
   I check TUTORIAL file but did not understood what is the problem.

E: mutagen non-executable-script
/usr/lib/python2.4/site-packages/mutagen/__init__.py 0644
   I found other python packages are also having same 
__init__.py 0644 permissions. so why rpmlint report problem i confuesd?

 can anyone comment on this issues?

Created attachment 132479 [details]
mutagen-1.5.1-2.spec

New SPEC file version (1.5.1-2)

Thanks for review this package.

The line:
Requires:   python-abi = %(%{__python} -c "import sys ; print sys.version[:3]"
is from fedora-rpmdevtools template so I didn't change it and, as I see, other 
SPEC files also has this line so I don't delete it.

W: mutagen wrong-file-end-of-line-encoding /usr/share/doc/mutagen-1.5.1/TUTORIAL
I checked it and I noticed that TUTORIAL file has Windows-style line endings,
so there is \r\n instead of \n. In new SPEC this is fixed.

E: mutagen non-executable-script ...
The first line of file is
#! /usr/bin/env python
and for this reason rpmlint treat this file as executable but it shouldn't be 
executable. I think we can leave it alone.
I've done new SPEC with your and my fixes.

(In reply to comment #4)
> Thanks for review this package.
> 
> The line:
> Requires:   python-abi = %(%{__python} -c "import sys ; print sys.version[:3]"
> is from fedora-rpmdevtools template so I didn't change it and, as I see, other 
> SPEC files also has this line so I don't delete it.

The python spec template will soon be fixed to get rid of this:

http://www.redhat.com/archives/fedora-extras-list/2006-July/msg00557.html

If you check the "requires" of the built package on FC4 or later, you should
find a dependency on "python(abi) = 2.4" automatically added by RPM, making the
python-abi dependency in the spec file redundant.

> E: mutagen non-executable-script ...
> The first line of file is
> #! /usr/bin/env python
> and for this reason rpmlint treat this file as executable but it shouldn't be 
> executable. I think we can leave it alone.

rpmlint knows it's not executable but thinks that it's a script (which it isn't)
because of the "#! /usr/bin/env python" as you say. You can shut rpmlint up by
editing the first line out of the affected files in the %prep stage of the spec.

Created attachment 132496 [details]
mutagen-1.5.1-3.spec

Okay, I've made new SPEC file with deleted python-abi require and fix in %prep
section.

MichaÅ, I'll sponsor you.  A quick question before I start the review: why is
this package arch-specific?  It doesn't seem to contain any compiled code.  This
results in the following rpmlint warnings:

E: mutagen no-binary
E: mutagen-debuginfo empty-debuginfo-package

which are correct since there are no binary executables and the debuginfo
package ends up empty.  Adding "BuildArch: noarch" seems to result in a proper
package.

Created attachment 132774 [details]
mutagen-1.5.1-4.spec

Thanks for advice and sponsorship. I noticed that python packages are
mostly noarch and I forgot to add it to spec file. So, here is new spec file
with no rpmlint errors.

Note: it's nice to the reviewers if you generate a new src.rpm with each change
you make to your spec.  That way it's simple to just pull down the new package
and build it.

You seem to have tickled a new rpmlint warning:
  W: mutagen mixed-use-of-spaces-and-tabs
This happened because you indented "noarch" with a tab.  Not a big deal but that
means it's easy to fix.

More serious is the name of the package: according to the naming guidelines this
package should be named python-mutagen.  See
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#AddonPython

There's no need to pass CFLAGS to setyp.py since this is a noarch package.

This package seems to have a test suite, but you don't call it.  You should
consider adding a section like:

%check
%{__python} setup.py coverage

Finally, note that if you disagree with me about the necessity of any of these
issues I've raised, let me know why you think my reasoning is bogus and we'll
discuss it.

Review:
* source files match upstream:
   9ce5d5f14e02f2eabd919d6bdaebadbc  mutagen-1.5.1.tar.gz
X package meets naming and packaging guidelines (should be called python-mutagen).
X specfile is properly named, is cleanly written and uses macros consistently.
(looks good but should be named python-mutagen.spec).
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text included in package.
* latest version is being packaged.
* BuildRequires are proper.
X No need to pass compiler flags for noarch packages.
* %clean is present.
* package builds in mock (development, x86_64).
X rpmlint is silent (spaces and tabs thing)
* noarch package; no debuginfo.
* final provides and requires are sane:
   mutagen = 1.5.1-4.fc6
  =
   /usr/bin/python
   python(abi) = 2.4
X %check is not present but there is a test suite.
* no shared libraries are present.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.
* not a GUI app.

Created attachment 132784 [details]
python-mutagen-1.5.1-5.spec

(In reply to comment #9)
> Note: it's nice to the reviewers if you generate a new src.rpm with each
change
> you make to your spec.  That way it's simple to just pull down the new
package
> and build it.

Yes, I know, but I have a slow connection shared on 5 computers in home,
so sending even 296 kB file blocks it completely and I send such files
as rarely as it possible... :/

> You seem to have tickled a new rpmlint warning:
>   W: mutagen mixed-use-of-spaces-and-tabs
> This happened because you indented "noarch" with a tab.  Not a big deal but
that
> means it's easy to fix.

This is odd, because when I checked it in my rpmlint, it didn't show any
errors, but I fixed it in new spec.

> More serious is the name of the package: according to the naming guidelines
this
> package should be named python-mutagen.  See
> http://fedoraproject.org/wiki/Packaging/NamingGuidelines#AddonPython

I don't know how can I overlooked that :/ You're right and I fixed it.

> There's no need to pass CFLAGS to setyp.py since this is a noarch package.

Fixed.

> This package seems to have a test suite, but you don't call it.  You should
> consider adding a section like:
> 
> %check
> %{__python} setup.py coverage

I know, but check procedure looks broken. It shows errors that look like
dependent to errors in check procedure, not in program.

Huh, I found some time to send new SPEC and SRPM so it's here:
Spec URL: http://ecik.zspswidwin.pl/mutagen/python-mutagen.spec
SRPM URL: http://ecik.zspswidwin.pl/mutagen/python-mutagen-1.5.1-5.src.rpm

Nice, thanks.  Let's see:

Package and specfile name fixed.
Compiler flags not passed needlessly.
rpmlint placated.
Absence of %check section reasonably explained.

I did run the checks and it seemed to work but I admit to not understanding the
output, and in the end there wasn't anything that told me whether or not it
actually found any problems.  It would still be nice to work with upstream and
get it enabled eventually; we want to have automated tests for as many packages
as possible.

So everything is fixed, and this package is APPROVED.  Now you can deal with the
CLA if you haven't already done so, and then request your cvsextras account. 
I'll sponsor you.  You can also request fedorabugs access so that you can do
your own reviews; I can approve you for that as well.  Info for doing that is at
http://fedoraproject.org/wiki/Extras/Contributors#GetAFedoraAccount

Once that's all done, you can check in your code, request your branches, start
your builds and close this ticket.  Just email me or catch me on IRC if you have
questions.  (My nick is "tibbs".)

Package built successfully, so I can close this ticket