Welcome, Jon.

I'll take a look and possibly sponsor you.

Before I start -- do you plan to support CentOS as well?  If so, which versions?

Hi, and thanks for doing the review!

Yes, I plan to support the same platforms as nordugrid-arc, so that would also include CentOS 5 to 7. Not sure what to do with the python >= 2.5 requirement on CentOS 5 though.

Hmm, I guess you'll have to target el6+ only.  el5 python is just too old for this package to work.  I'll review this as a Fedora/el6+ package then...

First of all, familiarise yourself with Fedora Packaging guidelines:
https://fedoraproject.org/wiki/Packaging:Guidelines
There's quite a lot to learn, I know.  Feel free to ask if you need anything.


Okay, so, the review...


#1  Since this is only going to el6+, you can drop the BuildRoot tag, buildroot cleaning in %install and the whole %clean section.  These aren't really needed nowadays.
https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag


#2  There's no need to explitly declare %defattr anymore.  You may drop that too.
https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions


#3  There used to be a list of packages guaranteed to be present in every buildroot.  This is no longer the case and every package must declare all its direct build dependencies explicitly.  To ensure your package builds in the future too, you need to add quite a few to your list of build requirements (BuildRequires):
 - coreutils -- `rm' is called in the spec
 - findutils -- `find' is called in the spec
 - make -- called in the spec
 - perl -- ditto
 - perl(base) -- t/25py_sub.t:70
 - perl(Carp) -- Python.pm:3
 - perl(Config) -- Makefile.PL:2, t/11factor.t:3
 - perl(Cwd) -- Makefile.PL:3, Makefile.PL:307
 - perl(DynaLoader) -- Python.pm:5
 - perl(Exporter) -- Python.pm:6
 - perl(File::Path) -- t/00init.t:5
 - perl(Getopt::Long) -- Makefile.PL:5
 - perl(Inline::denter) -- Python.pm:184, Python.pm:215
 - perl(Inline::Filters) -- Python.pm:69
 - perl(overload) -- Python.pm:313, Python.pm:370, Python.pm:378
 - perl(Parse::RecDescent) -- t/03parse.t:3
 - perl(POSIX) -- t/30floats.t:6
 - perl(strict) -- all over the place
 - perl(Test) -- most tests
 - perl(Test::Simple) -- t/19testref.t:1, t/26undef.t:1
 - perl(utf8) -- t/20unicode.t:4
 - perl(warnings) -- many tests

Inline::Filters appears to be optional.
Parse::RecDescent is optional.
(However, it's good practice to run as many tests as possible so unless it's causing some problems, for example dependency cycles, it's a good idea to require optional dependencies as well.)

One has to go through the sources to figure these out.  There are tools that can help you with it, e.g. `tangerine' for perl code.

Next, since you want to build against python 2.5+, you need to add a version constraint to the python-devel build dependency:
BuildRequires:  python-devel >= 2.5


#4  You don't actually need to buildrequire perl(Digest::MD5).  It's not used anywhere in the code.  Upstream metadata is incorrect.


#5  Next, runtime dependencies.  After the package is built, rpmbuild scans the resulting files and attempts to generate various `requires' and `provides' for you.  This works for perl too, to some extent.  The majority of your dependencies will be found automatically and you don't have to worry about declaring them explicitly with `Requires'.  It doesn't find everything, though.  You'll have to add the following to your list of runtime dependencies (`Requires'):
 - perl(Inline::denter)
 - perl(Inline::Filters) -- possibly; this one is optional, up to you

You may drop the following from your list:
 - perl(Data::Dumper) -- not used at runtime
 - perl(Digest::MD5) -- not needed at all
 - perl(Test::More) -- not used at runtime
 - python -- rpmbuild generates a library dependency from ldd for you, so the package will always require the library against which it was built

The generators also add an unversioned dependency on perl(Inline).  You explicitly require `perl(Inline) >= 0.46', which is correct.  To remove the underspecified generated dependency, you need to add a filter like this:

%global __requires_exclude %{?__requires_exclude:%__requires_exclude|}^perl\\(Inline\\)$

The most common place to put this is between the dep list and the %description.


#6  The following line isn't needed, feel free to drop it:
find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \;


#7  Consider using DESTDIR instead of PERL_INSTALL_ROOT.


#8  Your changelog format isn't valid (missing e-mail address).
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs


#9  Drop META.json and TESTED from %doc.  These files aren't really useful to the end users.


I hope it's all clear.  If not, let me know :)

(In reply to Petr Šabata from comment #5)

Thanks for clear and thorough review!

> First of all, familiarise yourself with Fedora Packaging guidelines:
> https://fedoraproject.org/wiki/Packaging:Guidelines
> There's quite a lot to learn, I know.  Feel free to ask if you need anything.

Thanks, I've read it, but still working on understanding it :) Your review already clarifies quite a bit for me.

> 
> 
> Okay, so, the review...
> 
> 
> #1  Since this is only going to el6+, you can drop the BuildRoot tag,
> buildroot cleaning in %install and the whole %clean section.  These aren't
> really needed nowadays.
> https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

Noted and dropped.

> 
> 
> #2  There's no need to explitly declare %defattr anymore.  You may drop that
> too.
> https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

Dropped.

> 
> 
> #3  There used to be a list of packages guaranteed to be present in every
> buildroot.  This is no longer the case and every package must declare all
> its direct build dependencies explicitly.  To ensure your package builds in
> the future too, you need to add quite a few to your list of build
> requirements (BuildRequires):
>  - coreutils -- `rm' is called in the spec
>  - findutils -- `find' is called in the spec
>  - make -- called in the spec
>  - perl -- ditto
>  - perl(base) -- t/25py_sub.t:70
>  - perl(Carp) -- Python.pm:3
>  - perl(Config) -- Makefile.PL:2, t/11factor.t:3
>  - perl(Cwd) -- Makefile.PL:3, Makefile.PL:307
>  - perl(DynaLoader) -- Python.pm:5
>  - perl(Exporter) -- Python.pm:6
>  - perl(File::Path) -- t/00init.t:5
>  - perl(Getopt::Long) -- Makefile.PL:5
>  - perl(Inline::denter) -- Python.pm:184, Python.pm:215
>  - perl(Inline::Filters) -- Python.pm:69
>  - perl(overload) -- Python.pm:313, Python.pm:370, Python.pm:378
>  - perl(Parse::RecDescent) -- t/03parse.t:3
>  - perl(POSIX) -- t/30floats.t:6
>  - perl(strict) -- all over the place
>  - perl(Test) -- most tests
>  - perl(Test::Simple) -- t/19testref.t:1, t/26undef.t:1
>  - perl(utf8) -- t/20unicode.t:4
>  - perl(warnings) -- many tests

I see I was a bit sloppy there yes. Added all but Inline::Filters now.

> 
> Inline::Filters appears to be optional.
> Parse::RecDescent is optional.
> (However, it's good practice to run as many tests as possible so unless it's
> causing some problems, for example dependency cycles, it's a good idea to
> require optional dependencies as well.)

Inline::Filters isn't available in EPEL, so since it's optional I suggest not to include it.

> 
> One has to go through the sources to figure these out.  There are tools that
> can help you with it, e.g. `tangerine' for perl code.

Ah, tangerine helped a lot, thanks! (I'm not a native perl speaker, so got a bit confused there.)

> 
> Next, since you want to build against python 2.5+, you need to add a version
> constraint to the python-devel build dependency:
> BuildRequires:  python-devel >= 2.5

Got it.

> 
> 
> #4  You don't actually need to buildrequire perl(Digest::MD5).  It's not
> used anywhere in the code.  Upstream metadata is incorrect.

Removed Digest::MD5 requirement and notified upstream.

> 
> 
> #5  Next, runtime dependencies.  After the package is built, rpmbuild scans
> the resulting files and attempts to generate various `requires' and
> `provides' for you.  This works for perl too, to some extent.  The majority
> of your dependencies will be found automatically and you don't have to worry
> about declaring them explicitly with `Requires'.  It doesn't find
> everything, though.  You'll have to add the following to your list of
> runtime dependencies (`Requires'):
>  - perl(Inline::denter)
>  - perl(Inline::Filters) -- possibly; this one is optional, up to you

Added denter.

> 
> You may drop the following from your list:
>  - perl(Data::Dumper) -- not used at runtime
>  - perl(Digest::MD5) -- not needed at all
>  - perl(Test::More) -- not used at runtime
>  - python -- rpmbuild generates a library dependency from ldd for you, so
> the package will always require the library against which it was built

Dropped, thanks.

> 
> The generators also add an unversioned dependency on perl(Inline).  You
> explicitly require `perl(Inline) >= 0.46', which is correct.  To remove the
> underspecified generated dependency, you need to add a filter like this:
> 
> %global __requires_exclude
> %{?__requires_exclude:%__requires_exclude|}^perl\\(Inline\\)$
> 
> The most common place to put this is between the dep list and the
> %description.
> 
> 
> #6  The following line isn't needed, feel free to drop it:
> find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \;

Dropped.

> 
> 
> #7  Consider using DESTDIR instead of PERL_INSTALL_ROOT.

Considered and accepted :)

> 
> 
> #8  Your changelog format isn't valid (missing e-mail address).
> https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

Fixed.

> 
> 
> #9  Drop META.json and TESTED from %doc.  These files aren't really useful
> to the end users.

Dropped.

> 
> 
> I hope it's all clear.  If not, let me know :)

Crystal clear, thanks :)

Uploaded new spec and srpm same place as before.

Updated Spec URL: http://folk.uio.no/jonkni/fedora-packaging/perl-Inline-Python.spec
New SRPM URL: http://folk.uio.no/jonkni/fedora-packaging/perl-Inline-Python-0.49-2.fc22.src.rpm

Awesome!  perl(DynaLoader) and perl(Exporter) are still missing, however.  I suppose you just overlooked them?

Doh! Yes, temporary blindness, thanks for spotting it.

Updated Spec URL: http://folk.uio.no/jonkni/fedora-packaging/perl-Inline-Python.spec
New SRPM URL: http://folk.uio.no/jonkni/fedora-packaging/perl-Inline-Python-0.49-3.fc22.src.rpm

Good.  Approving.
You can now request git branches and, once ready, push and build your package.  Please, add `perl-sig' as a watcher (via the InitialCC tag).  See https://fedoraproject.org/wiki/Package_SCM_admin_requests.

Also, I've just sponsored you into the packager group.  Welcome aboard!

Cool, thanks!

New Package SCM Request
=======================
Package Name: perl-Inline-Python
Short Description: Write Perl subs and classes in Python
Upstream URL: http://search.cpan.org/dist/Inline-Python/
Owners: jonkni
Branches: f21 f22 f23 el6 epel7
InitialCC: perl-sig

Git done (by process-git-requests).

I see you've built the package and it's not in stable.  I'll close the bug for you...