Bug 1234791 - Review Request: perl-Inline-Python - Write Perl subs and classes in Python
Summary: Review Request: perl-Inline-Python - Write Perl subs and classes in Python
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Petr Šabata
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-06-23 09:07 UTC by Jon Kerr Nilsen
Modified: 2015-07-21 11:28 UTC (History)
2 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2015-07-21 11:28:15 UTC
psabata: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Jon Kerr Nilsen 2015-06-23 09:07:49 UTC
Spec URL: http://folk.uio.no/jonkni/fedora-packaging/perl-Inline-Python.spec
SRPM URL: http://folk.uio.no/jonkni/fedora-packaging/perl-Inline-Python-0.49-1.fc22.src.rpm
Description:
The Inline::Python module allows you to put Python source code directly
"inline" in a Perl script or module. It sets up an in-process Python
interpreter, runs your code, and then examines Python's symbol table for
things to bind to Perl. The process of interrogating the Python interpreter
for global variables only occurs the first time you run your Python code. The
name-space is cached, and subsequent calls use the cached version.

Please note that this is my first package so I'm also seeking a sponsor. I'm upstream release manager and developer for NorduGrid ARC (https://admin.fedoraproject.org/pkgdb/package/nordugrid-arc/) and we need this package for some new development.

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=10188934
Fedora Account System Username: jonkni

Comment 1 Petr Šabata 2015-06-23 12:13:38 UTC
Welcome, Jon.

I'll take a look and possibly sponsor you.

Comment 2 Petr Šabata 2015-06-23 12:51:56 UTC
Before I start -- do you plan to support CentOS as well?  If so, which versions?

Comment 3 Jon Kerr Nilsen 2015-06-23 13:03:35 UTC
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.

Comment 4 Petr Šabata 2015-06-23 13:11:44 UTC
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...

Comment 5 Petr Šabata 2015-06-23 15:54:37 UTC
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 :)

Comment 6 Jon Kerr Nilsen 2015-06-24 09:51:32 UTC
(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.

Comment 8 Petr Šabata 2015-06-24 15:14:19 UTC
Awesome!  perl(DynaLoader) and perl(Exporter) are still missing, however.  I suppose you just overlooked them?

Comment 9 Jon Kerr Nilsen 2015-06-24 18:28:52 UTC
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

Comment 10 Petr Šabata 2015-06-25 08:43:53 UTC
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!

Comment 11 Jon Kerr Nilsen 2015-06-25 11:45:28 UTC
Cool, thanks!

Comment 12 Jon Kerr Nilsen 2015-06-25 12:12:17 UTC
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

Comment 13 Gwyn Ciesla 2015-06-25 14:23:15 UTC
Git done (by process-git-requests).

Comment 14 Petr Šabata 2015-07-21 11:28:15 UTC
I see you've built the package and it's not in stable.  I'll close the bug for you...


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