Bug 887913 - Review Request: perl-Math-Clipper - Polygon clipping in 2D
Review Request: perl-Math-Clipper - Polygon clipping in 2D
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Petr Pisar
Fedora Extras Quality Assurance
:
Depends On: 876399 876405
Blocks: 890839
  Show dependency treegraph
 
Reported: 2012-12-17 10:38 EST by Miro Hrončok
Modified: 2013-03-04 17:42 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-02-02 14:50:32 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
ppisar: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Miro Hrončok 2012-12-17 10:38:12 EST
Spec URL: https://github.com/hroncok/SPECS/raw/master/perl-Math-Clipper.spec
SRPM URL: https://github.com/downloads/hroncok/SPECS/perl-Math-Clipper-1.15-1.fc17.src.rpm
Description: Clipper is a C++ (and Delphi) library that implements polygon clipping.
Fedora Account System Username: churchyard
Comment 1 Petr Pisar 2012-12-21 07:22:33 EST
URL and Source0 are usable. Ok.
Source tar ball is original (SHA-256: f92f9febef97ca2fc42b58654cb8753a41e8492a7c047f8aeac09eb2ad1fe498). Ok.
Summary verified from lib/Math/Clipper.pm. Ok.

FIX: The description is not about Math-Clipper Perl package.

License verified from lib/Math/Clipper.pm, src/clipper.hpp. Ok.

FATAL: This package bundles Clipper library <http://sourceforge.net/projects/polyclipping/> (see src directory) and VERSION section in Math::Clipper POD:

  This module was built around, and includes, Clipper version 4.9.6.

You have to package the C library from SF.net as separate package and then to build this perl package against the library.

Alternatively ask for exception Fedora Packaging Committee.

Resolution: Package NOT approved.
Comment 2 Miro Hrončok 2012-12-22 14:30:03 EST
What would you recommand? Askt the committee, or package it separately?

Could you provide help with any of the options?

Thanks
Comment 3 Miro Hrončok 2012-12-28 08:31:51 EST
As the clipper library is already in Fedora (as polyclipping package), I'll package this using it.
Comment 4 Miro Hrončok 2012-12-29 10:00:18 EST
Spec URL: https://github.com/hroncok/SPECS/raw/master/perl-Math-Clipper.spec
SRPM URL: https://github.com/downloads/hroncok/SPECS/perl-Math-Clipper-1.16-1.fc17.src.rpm


- New version
- Removed boundled C clipper and using the distribution one


I have a question about full versioning clipper (polyclipping) dependency.
Math::Clipper version 1.16 comes with clipper 4.10.0. Should I require exactly this version of polyclipping-devel and libpolyclipping.so.3? If so, should I modify the spec for F17 and F18, as they are using older versions, to use older version of this Perl module too?

It seems, recent changes of this Perl module are only new library releases.
Comment 5 Petr Pisar 2013-01-02 07:59:00 EST
This is rebase, restarting review:

URL and Source0 are usable. Ok
Source tar ball is original (SHA-256: 3bf01327a002852504026b673449d9c8e2a77543567d42d206aa7f6156b181de). Ok.
perl-Math-Clipper-1.16-1.no-c-sources.patch is good. Ok.
Summary is good. Ok.
Description is good. Ok.
License verified from lib/Math/Clipper.pm. Ok.
Bundled Clipper is not used. Ok.

TODO: Do not package META.json and xsp subtree. These are not useful for users.

TODO: Specify version for perl(Module::Build::WithXSpp) build-require `>= 0.10' (META.yml:12).

TODO: In my opinion `perl(ExtUtils::XSpp)' is not direct dependency of this package, so it shouldn't be build-required.

TODO: Build-require `perl(XSLoader)' for running tests (lib/Math/Clipper.pm:14).
TODO: Build-require `perl(constant)' for running tests (t/002basic.t:6).

FIX: This package build-requires Module::Build::WithXSpp (bug #876405) and ExtUtils::Typemaps::Default (bug #876399) which are not yet in Fedora. I added proper bug dependencies and I postponed this review until they get into Fedora.

Resolution: POSTPONED.
Comment 6 Petr Pisar 2013-01-02 08:13:45 EST
(In reply to comment #4)
> I have a question about full versioning clipper (polyclipping) dependency.
> Math::Clipper version 1.16 comes with clipper 4.10.0. Should I require
> exactly this version of polyclipping-devel and libpolyclipping.so.3?

I do not need to specify run-time dependency on libpolyclipping.so.3. It should get discovered by rpmbuild automatically.

You need to build-require specific version of polyclipping-devel only if it's required due to incompatibilities. I think it's not necessary in this case as the polyclipping does not change ABI frequently. However I would recommend to build-require minimal version equalled to bundled version because users can imply Clipper version from Math::Clipper. I.e. polyclipping-devel >= 4.10.0 for this perl-Clipper-Math release.

> If so, should I modify the spec for F17 and F18, as they are using older
> versions, to use older version of this Perl module too?
> 
You should be consistent across Fedora releases, thus if you decide to tie the two versions, you should either not package Perl module into older Fedoras, or upgrade polyclipping, or package older Perl modules. That's up to you.
Comment 7 Miro Hrončok 2013-01-03 08:22:22 EST
Spec URL: https://github.com/hroncok/SPECS/raw/master/perl-Math-Clipper.spec
SRPM URL: https://github.com/downloads/hroncok/SPECS/perl-Math-Clipper-1.16-2.fc17.src.rpm

- Removed META.json and xsp from doc
- Specified version for polyclipping-devel BR
- Specified version for perl(Module::Build::WithXSpp) BR
- Removed perl(ExtUtils::XSpp) BR
- Added BRs perl(XSLoader) and perl(constant)
Comment 8 Miro Hrončok 2013-01-07 15:00:47 EST
Both build-required packages are now in rawhide.
Comment 9 Petr Pisar 2013-01-08 04:41:27 EST
Spec file changes:

--- perl-Math-Clipper.spec.old  2013-01-02 10:59:45.371000000 +0100
+++ perl-Math-Clipper.spec      2013-01-08 10:09:41.174000000 +0100
@@ -1,23 +1,24 @@
 Name:           perl-Math-Clipper
 Version:        1.16
-Release:        1%{?dist}
+Release:        2%{?dist}
 Summary:        Perl wrapper around Clipper library
 License:        Boost
 Group:          Development/Libraries
 URL:            http://search.cpan.org/dist/Math-Clipper/
 Source0:        http://www.cpan.org/authors/id/A/AA/AAR/Math-Clipper-%{version}.tar.gz
 Patch0:         %{name}-1.16-1.no-c-sources.patch
-BuildRequires:  perl(ExtUtils::Typemaps::Default) >= 0.05
-BuildRequires:  perl(ExtUtils::XSpp) >= 0.16
-BuildRequires:  perl(Module::Build)
-BuildRequires:  perl(Module::Build::WithXSpp)
-BuildRequires:  perl(Test::More)
-BuildRequires:  perl(Test::Deep)
-BuildRequires:  perl(File::Spec)
 BuildRequires:  perl(Carp)
 BuildRequires:  perl(Config)
+BuildRequires:  perl(constant)
 BuildRequires:  perl(Exporter)
-BuildRequires:  polyclipping-devel
+BuildRequires:  perl(ExtUtils::Typemaps::Default) >= 0.05
+BuildRequires:  perl(File::Spec)
+BuildRequires:  perl(Module::Build)
+BuildRequires:  perl(Module::Build::WithXSpp) >= 0.10
+BuildRequires:  perl(Test::Deep)
+BuildRequires:  perl(Test::More)
+BuildRequires:  perl(XSLoader)
+BuildRequires:  polyclipping-devel >= 4.10
 Requires:       perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

 %{?perl_default_filter} # Filters (not)shared c libs
@@ -45,12 +46,19 @@
 ./Build test

 %files
-%doc Changes META.json xsp
+%doc Changes
 %{perl_vendorarch}/auto/*
 %{perl_vendorarch}/Math*
 %{_mandir}/man3/*

 %changelog
+* Thu Jan 03 2013 Miro Hrončok <miro@hroncok.cz> - 1.16-2
+- Removed META.json and xsp from doc
+- Specified version for polyclipping-devel BR
+- Specified version for perl(Module::Build::WithXSpp) BR
+- Removed perl(ExtUtils::XSpp) BR
+- Added BRs perl(XSLoader) and perl(constant)
+
 * Fri Dec 28 2012 Miro Hrončok <miro@hroncok.cz> - 1.16-1
 - New version
 - Removed boundled C clipper and using the distribution one


> TODO: Do not package META.json and xsp subtree. These are not useful for users.
-%doc Changes META.json xsp
+%doc Changes
Ok.

> TODO: Specify version for perl(Module::Build::WithXSpp) build-require `>= 0.10' (META.yml:12).

> TODO: In my opinion `perl(ExtUtils::XSpp)' is not direct dependency of this package, so it shouldn't be build-required.
-BuildRequires:  perl(Module::Build::WithXSpp)
+BuildRequires:  perl(Module::Build::WithXSpp) >= 0.10
Ok.

> TODO: Build-require `perl(XSLoader)' for running tests (lib/Math/Clipper.pm:14).
+BuildRequires:  perl(XSLoader)
Ok.

> TODO: Build-require `perl(constant)' for running tests (t/002basic.t:6).
+BuildRequires:  perl(constant)
Ok.

> FIX: This package build-requires Module::Build::WithXSpp (bug #876405) and ExtUtils::Typemaps::Default (bug #876399) which are not yet in Fedora. I added proper bug dependencies and I postponed this review until they get into Fedora.
Ok.

TODO: You can replace %__perl macro with plain perl.

All tests pass. Ok.

$ rpmlint perl-Math-Clipper.spec ../SRPMS/perl-Math-Clipper-1.16-2.fc19.src.rpm ../RPMS/x86_64/perl-Math-Clipper-1.16-2.fc19.x86_64.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
rpmlint is Ok.

$ rpm -q -lv -p ../RPMS/x86_64/perl-Math-Clipper-1.16-2.fc19.x86_64.rpm 
drwxr-xr-x    2 root    root                        0 Jan  8 10:24 /usr/lib64/perl5/vendor_perl/Math
-rw-r--r--    1 root    root                    24913 Jan  8 10:24 /usr/lib64/perl5/vendor_perl/Math/Clipper.pm
drwxr-xr-x    2 root    root                        0 Jan  8 10:24 /usr/lib64/perl5/vendor_perl/auto/Math
drwxr-xr-x    2 root    root                        0 Jan  8 10:24 /usr/lib64/perl5/vendor_perl/auto/Math/Clipper
-rwxr-xr-x    1 root    root                    36544 Jan  8 10:24 /usr/lib64/perl5/vendor_perl/auto/Math/Clipper/Clipper.so
drwxr-xr-x    2 root    root                        0 Jan  8 10:24 /usr/share/doc/perl-Math-Clipper-1.16
-rw-r--r--    1 root    root                     3365 Dec 26 15:31 /usr/share/doc/perl-Math-Clipper-1.16/Changes
-rw-r--r--    1 root    root                     8769 Jan  8 10:24 /usr/share/man/man3/Math::Clipper.3pm.gz

File layout and permissions are Ok.

$ rpm -q --requires -p ../RPMS/x86_64/perl-Math-Clipper-1.16-2.fc19.x86_64.rpm |sort |uniq -c
      1 libc.so.6()(64bit)
      1 libc.so.6(GLIBC_2.2.5)(64bit)
      1 libgcc_s.so.1()(64bit)
      1 libgcc_s.so.1(GCC_3.0)(64bit)
      1 libpolyclipping.so.4()(64bit)
      1 libstdc++.so.6()(64bit)
      1 libstdc++.so.6(CXXABI_1.3)(64bit)
      1 libstdc++.so.6(GLIBCXX_3.4)(64bit)
      1 perl >= 0:5.008
      1 perl(Carp)
      1 perl(Config)
      1 perl(Exporter)
      1 perl(:MODULE_COMPAT_5.16.2)
      1 perl(strict)
      1 perl(warnings)
      1 perl(XSLoader)
      1 rpmlib(CompressedFileNames) <= 3.0.4-1
      1 rpmlib(FileDigests) <= 4.6.0-1
      1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
      1 rpmlib(PayloadIsXz) <= 5.2-1
      1 rtld(GNU_HASH)
Binary requires are Ok.

$ rpm -q --provides -p ../RPMS/x86_64/perl-Math-Clipper-1.16-2.fc19.x86_64.rpm |sort |uniq -c
      1 perl(Math::Clipper) = 1.16
      1 perl-Math-Clipper = 1.16-2.fc19
      1 perl-Math-Clipper(x86-64) = 1.16-2.fc19
Binary provides are Ok.

FIX: Package does not build in F19 (http://koji.fedoraproject.org/koji/taskinfo?taskID=4847703).
Math::Clipper::orientation: polygon is not an array reference at t/011Int128Comparison.t line 30.
# Looks like your test exited with 255 before it could output anything.
t/011Int128Comparison.t .. 
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/1 subtests 
Test Summary Report
-------------------
t/011Int128Comparison.t (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 1 tests but ran 0.
Files=11, Tests=63,  1 wallclock secs ( 0.06 usr  0.02 sys +  0.45 cusr  0.07 csys =  0.60 CPU)
Result: FAIL

I think this is due to newer polyclipping-devel-5.0.2-1.fc19 library in rawhide.

Otherwise the package is in line with Fedora and Perl packaging guidelines.


Please correct all `FIX' issues, consider fixing `TODO' items, and provide new spec file.

Resolution: Package NOT approved.
Comment 10 Miro Hrončok 2013-01-08 06:12:41 EST
(In reply to comment #9)
> I think this is due to newer polyclipping-devel-5.0.2-1.fc19 library in
> rawhide.

It seems to.

I'll wait for upstream to publish new version.

Is there a way to add this package to F17 and F18 where are the older versions of polyclipping-devel? Even if it builds in rawhide, i would add older versions to F17 and F18 to reflect upstream perl module and library version relations.

Thanks
Comment 11 Petr Pisar 2013-01-08 07:13:41 EST
(In reply to comment #10)
> Is there a way to add this package to F17 and F18 where are the older
> versions of polyclipping-devel? Even if it builds in rawhide, i would add
> older versions to F17 and F18 to reflect upstream perl module and library
> version relations.
> 
What do sponsors think?

I believe best practise is to package and review for rawhide first. At least because older build inherits into rawhide.
Comment 12 Petr Šabata 2013-01-08 15:26:29 EST
I can only speak for myself :)

Always build for the rawhide first to avoid unintentional upgrade path breakage.

Although unusual, I suppose building older versions of the module for older Fedora releases wouldn't violate any packaging rules.  It's similar to patching the package to work with the older dependencies present there.
Comment 13 Miro Hrončok 2013-01-17 07:57:39 EST
Spec URL: https://github.com/hroncok/SPECS/raw/master/perl-Math-Clipper.spec
SRPM URL: https://github.com/downloads/hroncok/SPECS/perl-Math-Clipper-1.17-1.fc18.src.rpm

Now works with rawhide version of clipper lib. Please, do the review before upstream releases new lib version :D
Comment 14 Petr Pisar 2013-01-17 08:55:52 EST
This is rebase, doing review from scratch.

URL and Source0 are useable. ok.
Source tar ball is original (SHA-256: fadd8d60a1499ae584f3d587475d27a01f4679249c1050f5307d71c09095c684). Ok.
Patch to build against system library is Ok.
Summary is Ok.
Description is Ok.
License verified from lib/Math/Clipper.pm. Ok.
There is XS code, architecture specific BuildArch is Ok.

TODO: You can replace %{__perl} macro with plain perl command.
TODO: You can remove `perl(Module::Build)' from build-requires as there is no direct use of the module (the build is driven by Module::Build::WithXSpp).

All tests pass. Ok.

$ rpmlint perl-Math-Clipper.spec ../SRPMS/perl-Math-Clipper-1.17-1.fc19.src.rpm ../RPMS/x86_64/perl-Math-Clipper-*1.17-1.*
3 packages and 1 specfiles checked; 0 errors, 0 warnings.
rpmlint is Ok.

$ rpm -q -lv -p ../RPMS/x86_64/perl-Math-Clipper-1.17-1.fc19.x86_64.rpm
drwxr-xr-x    2 root    root                        0 Jan 17 14:43 /usr/lib64/perl5/vendor_perl/Math
-rw-r--r--    1 root    root                    24978 Jan 17 14:43 /usr/lib64/perl5/vendor_perl/Math/Clipper.pm
drwxr-xr-x    2 root    root                        0 Jan 17 14:43 /usr/lib64/perl5/vendor_perl/auto/Math
drwxr-xr-x    2 root    root                        0 Jan 17 14:43 /usr/lib64/perl5/vendor_perl/auto/Math/Clipper
-rwxr-xr-x    1 root    root                    36536 Jan 17 14:43 /usr/lib64/perl5/vendor_perl/auto/Math/Clipper/Clipper.so
drwxr-xr-x    2 root    root                        0 Jan 17 14:43 /usr/share/doc/perl-Math-Clipper-1.17
-rw-r--r--    1 root    root                     3522 Jan 15 14:09 /usr/share/doc/perl-Math-Clipper-1.17/Changes
-rw-r--r--    1 root    root                     8790 Jan 17 14:43 /usr/share/man/man3/Math::Clipper.3pm.gz
File permissions and layout are Ok.

$ rpm -q --requires -p ../RPMS/x86_64/perl-Math-Clipper-1.17-1.fc19.x86_64.rpm |sort | uniq -c
      1 libc.so.6()(64bit)
      1 libc.so.6(GLIBC_2.2.5)(64bit)
      1 libgcc_s.so.1()(64bit)
      1 libgcc_s.so.1(GCC_3.0)(64bit)
      1 libpolyclipping.so.5()(64bit)
      1 libstdc++.so.6()(64bit)
      1 libstdc++.so.6(CXXABI_1.3)(64bit)
      1 libstdc++.so.6(GLIBCXX_3.4)(64bit)
      1 perl >= 0:5.008
      1 perl(Carp)
      1 perl(Config)
      1 perl(Exporter)
      1 perl(:MODULE_COMPAT_5.16.2)
      1 perl(strict)
      1 perl(warnings)
      1 perl(XSLoader)
      1 rpmlib(CompressedFileNames) <= 3.0.4-1
      1 rpmlib(FileDigests) <= 4.6.0-1
      1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
      1 rpmlib(PayloadIsXz) <= 5.2-1
      1 rtld(GNU_HASH)
Binary requires are Ok.

$ rpm -q --provides -p ../RPMS/x86_64/perl-Math-Clipper-1.17-1.fc19.x86_64.rpm |sort | uniq -c
      1 perl(Math::Clipper) = 1.17
      1 perl-Math-Clipper = 1.17-1.fc19
      1 perl-Math-Clipper(x86-64) = 1.17-1.fc19
Binary provides are Ok.

$ resolvedeps rawhide ../RPMS/x86_64/perl-Math-Clipper-1.17-1.fc19.x86_64.rpm 
Binary dependencies resolvable. Ok.

Package builds in F19 (http://koji.fedoraproject.org/koji/taskinfo?taskID=4878183). Ok.


Package is in line with Fedora and Perl packaging guidelines.

Please fix all `TODO' items before building the package.
Resolution: Package APPROVED.
Comment 15 Miro Hrončok 2013-01-17 09:09:36 EST
New Package SCM Request
=======================
Package Name: perl-Math-Clipper
Short Description: Polygon clipping in 2D
Owners: churchyard
Branches: f17 f18
InitialCC: perl-sig
Comment 16 Miro Hrončok 2013-01-17 09:11:49 EST
(In reply to comment #14)
> TODO: You can replace %{__perl} macro with plain perl command.
Why is there such macro anyway? Why should I not use it? If it is documented, don't explain it, just show me where, thanks.

Thanks for the review.
Comment 17 Jon Ciesla 2013-01-17 09:14:25 EST
Git done (by process-git-requests).
Comment 18 Petr Pisar 2013-01-17 09:24:37 EST
(In reply to comment #16)
> (In reply to comment #14)
> > TODO: You can replace %{__perl} macro with plain perl command.
> Why is there such macro anyway?
I don't know. Maybe some Fedora elders could know.

> Why should I not use it?
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#macros
https://fedorahosted.org/fpc/ticket/64
Comment 19 Miro Hrončok 2013-01-17 09:47:03 EST
Thanks
Comment 20 Fedora Update System 2013-01-17 10:31:12 EST
perl-Math-Clipper-1.15-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/perl-Math-Clipper-1.15-1.fc18
Comment 21 Fedora Update System 2013-02-21 12:10:58 EST
perl-Math-Clipper-1.15-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/perl-Math-Clipper-1.15-1.fc17
Comment 22 Fedora Update System 2013-03-04 17:42:20 EST
perl-Math-Clipper-1.15-1.fc17 has been pushed to the Fedora 17 stable repository.

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