[Bug 1234791] Review Request: perl-Inline-Python - Write Perl subs and classes in Python

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1234791



--- Comment #6 from Jon Kerr Nilsen <j.k.nilsen@xxxxxxxxxxx> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]