Bug 808884 - (perl-Data-Clone) Review Request: perl-Data-Clone - Polymorphic data cloning
Review Request: perl-Data-Clone - Polymorphic data cloning
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Petr Pisar
Fedora Extras Quality Assurance
http://search.cpan.org/dist/Data-Clone/
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-01 09:52 EDT by Iain Arnell
Modified: 2012-06-17 18:24 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-06-17 18:21:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ppisar: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Iain Arnell 2012-04-01 09:52:57 EDT
Spec URL: http://fedorapeople.org/~iarnell/review/perl-Data-Clone.spec
SRPM URL: http://fedorapeople.org/~iarnell/review/perl-Data-Clone-0.003-1.fc18.src.rpm

Description:
Data::Clone does data cloning, i.e. copies things recursively. This is
smart so that it works with not only non-blessed references, but also
with blessed references (i.e. objects). When clone() finds an object, it
calls a clone method of the object if the object has a clone, otherwise
it makes a surface copy of the object. That is, this module does
polymorphic data cloning.

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=3954441

*rt-0.10_02
Comment 1 Petr Pisar 2012-05-04 08:05:18 EDT
Source file is original. Ok.
URL and Source0 are usable. Ok.
Summary verified from lib/Data/Clone.pm. Ok.
License verified from lib/Data/Clone.pm. Ok.
Description verified from lib/Data/Clone.pm. Ok.
Package compiles XS code, the architecture specific BuildArch is Ok.

All releases provides perl(Test::More) >= 0.88, nonqualified build-require is Ok.
All releases provides perl(Test::Release) >= 0.03, nonqualified build-require is Ok.

FIX: Build-require `perl(inc::Module::Install)' (Makefile.PL:3) instead of `perl(ExtUtils::MakeMaker)'.

TODO: Consider removing bundled Perl modules under inc/ and build-requiring appropriate modules like Module::Install::AuthorTests. Otherwise you need explicitly build-require all dependencies of inc/* code.

FIX: Build-require modules used by code under inc/ (ExtUtils::Manifest, YAML::Tiny, etc.). I really recommend unbundling inc/*.

TODO: Build-require `perl(threads)' for optional tests (t/10_threads.t:4).
TODO: Build-require `perl(constant)' which can dual-live in the future (t/10_threads.t:4).

All tests pass. Ok.

$ rpmlint perl-Data-Clone.spec  ../SRPMS/perl-Data-Clone-0.003-1.fc18.src.rpm  ../RPMS/x86_64/perl-Data-Clone-*
perl-Data-Clone.x86_64: W: devel-file-in-non-devel-package /usr/lib64/perl5/vendor_perl/auto/Data/Clone/data_clone.h
3 packages and 1 specfiles checked; 0 errors, 1 warnings.
rpmlint is Ok.

$ rpm -q -lv -p ../RPMS/x86_64/perl-Data-Clone-0.003-1.fc18.x86_64.rpm
drwxr-xr-x    2 root    root                        0 May  4 13:51 /usr/lib64/perl5/vendor_perl/Data
-rw-r--r--    1 root    root                     5187 Jan 15  2011 /usr/lib64/perl5/vendor_perl/Data/Clone.pm
drwxr-xr-x    2 root    root                        0 May  4 13:51 /usr/lib64/perl5/vendor_perl/auto/Data
drwxr-xr-x    2 root    root                        0 May  4 13:51 /usr/lib64/perl5/vendor_perl/auto/Data/Clone
-rwxr-xr-x    1 root    root                    14688 May  4 13:51 /usr/lib64/perl5/vendor_perl/auto/Data/Clone/Clone.so
-rw-r--r--    1 root    root                      365 Jan 15  2011 /usr/lib64/perl5/vendor_perl/auto/Data/Clone/data_clone.h
drwxr-xr-x    2 root    root                        0 May  4 13:51 /usr/share/doc/perl-Data-Clone-0.003
-rw-r--r--    1 root    root                      291 Jan 15  2011 /usr/share/doc/perl-Data-Clone-0.003/Changes
-rw-r--r--    1 root    root                      505 Jan 15  2011 /usr/share/doc/perl-Data-Clone-0.003/README
-rw-r--r--    1 root    root                     3680 May  4 13:51 /usr/share/man/man3/Data::Clone.3pm.gz
File permissions and layout are Ok.

$ rpm -q --requires -p ../RPMS/x86_64/perl-Data-Clone-0.003-1.fc18.x86_64.rpm |sort |uniq -c
      1 libc.so.6()(64bit)
      1 libc.so.6(GLIBC_2.11)(64bit)
      1 libc.so.6(GLIBC_2.2.5)(64bit)
      1 perl >= 0:5.008_001
      2 perl(Exporter)
      1 perl(:MODULE_COMPAT_5.14.2)
      1 perl(parent)
      1 perl(strict)
      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)
TODO: Do not explicitly run-require auto-discovered perl(Exporter).

$ rpm -q --provides  -p ../RPMS/x86_64/perl-Data-Clone-0.003-1.fc18.x86_64.rpm |sort |uniq -c
      1 perl(Data::Clone) = 0.003
      1 perl-Data-Clone = 0.003-1.fc18
      1 perl-Data-Clone(x86-64) = 0.003-1.fc18
Binary provides are Ok.

$ resolvedeps rawhide  ../RPMS/x86_64/perl-Data-Clone-0.003-1.fc18.x86_64.rpm 
Binary dependencies resolvable. Ok.

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

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


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

Resolution: Package NOT approved.
Comment 2 Iain Arnell 2012-05-19 11:06:59 EDT
(In reply to comment #1)
> 
> FIX: Build-require `perl(inc::Module::Install)' (Makefile.PL:3) instead of
> `perl(ExtUtils::MakeMaker)'.
> 
> TODO: Consider removing bundled Perl modules under inc/ and build-requiring
> appropriate modules like Module::Install::AuthorTests. Otherwise you need
> explicitly build-require all dependencies of inc/* code.
> 
> FIX: Build-require modules used by code under inc/ (ExtUtils::Manifest,
> YAML::Tiny, etc.). I really recommend unbundling inc/*.

Oh! This is new. I'm not convinced that avoiding bundled Module::Install is the way to go here. The packaged inc::Module::Install even states
# This module ONLY loads if the user has manually installed their own
# installation of Module::Install, and are some form of MI author.

The additional dependencies under inc/ are either optional (e.g. YAML::Tiny for writing MYMETA.yml which we don't need) or already pulled in by EU::MM. Since Module::Install is intended to work with nothing more than core perl installed, I don't see any reason to explicitly mention dependencies unless there's an unbundled core module missing that causes build failure.
Comment 3 Petr Pisar 2012-05-21 03:45:20 EDT
Iain,

minimal build root does not guarantee perl-core being available. It's not acceptable to rely on transitive dependencies (Perl dependencies are tracked by modules, not RPM packages in Fedora). If your code needs a dependency directly, it's has to be declared in the spec file.

If you think inc/* files are not needed because they are already available from system (be ware Module::Install::AuthorTests is not part of EU::MM), then why do you restrain from removing them?
Comment 4 Ralf Corsepius 2012-05-21 04:59:18 EDT
(In reply to comment #2)
> (In reply to comment #1)
> > 
> > FIX: Build-require `perl(inc::Module::Install)' (Makefile.PL:3) instead of
> > `perl(ExtUtils::MakeMaker)'.
> > 
> > TODO: Consider removing bundled Perl modules under inc/ and build-requiring
> > appropriate modules like Module::Install::AuthorTests. Otherwise you need
> > explicitly build-require all dependencies of inc/* code.
> > 
> > FIX: Build-require modules used by code under inc/ (ExtUtils::Manifest,
> > YAML::Tiny, etc.). I really recommend unbundling inc/*.
> 
> Oh! This is new.
IMO, bundled modules in inc are harmless as long as they do not contain modules which later on are being used as run time deps.

> I'm not convinced that avoiding bundled Module::Install is
> the way to go here.
IMO, Module::Install are just "build-/install instruction file fragments" and therefore are OK to be used and to be bundled.
Comment 5 Petr Pisar 2012-05-21 05:14:45 EDT
Yes, it's possible to keep the inc files there. Even sometimes it's necessary because upstream bundles incompatible or modified version.

I only think it's easier for packager to unbundle them and build-require two or three lines, than reviewing them to extract all their dependencies (or worrying when build fails because sub-packaging some core module).
Comment 7 Petr Pisar 2012-06-04 04:29:15 EDT
Spec file changes:
--- perl-Data-Clone.spec.old    2012-04-01 15:45:49.000000000 +0200
+++ perl-Data-Clone.spec        2012-06-02 16:13:07.000000000 +0200
@@ -1,21 +1,23 @@
 Name:           perl-Data-Clone
 Version:        0.003
-Release:        1%{?dist}
+Release:        2%{?dist}
 Summary:        Polymorphic data cloning
 License:        GPL+ or Artistic
 Group:          Development/Libraries
 URL:            http://search.cpan.org/dist/Data-Clone/
 Source0:        http://www.cpan.org/authors/id/G/GF/GFUJI/Data-Clone-%{version}.tar.gz
-BuildRequires:  perl >= 1:5.8.1
+BuildRequires:  perl(constant)
 BuildRequires:  perl(Data::Dumper)
 BuildRequires:  perl(Devel::PPPort) >= 3.19
-BuildRequires:  perl(ExtUtils::MakeMaker)
 BuildRequires:  perl(ExtUtils::ParseXS) >= 2.21
+BuildRequires:  perl(inc::Module::Install)
+BuildRequires:  perl(Module::Install::AuthorTests)
 BuildRequires:  perl(parent)
 BuildRequires:  perl(Scalar::Util)
 BuildRequires:  perl(Test::LeakTrace)
 BuildRequires:  perl(Test::More)
 BuildRequires:  perl(Test::Requires)
+BuildRequires:  perl(threads)
 BuildRequires:  perl(Tie::Array)
 BuildRequires:  perl(Tie::Hash)
 BuildRequires:  perl(Time::HiRes)
@@ -59,5 +61,9 @@
 %{_mandir}/man3/*

 %changelog
+* Fri Jun 01 2012 Iain Arnell <iarnell@gmail.com> 0.003-2
+- BuildRequire inc::Module::Install, not EU::MM
+- additional BuildRequires for tests
+
 * Sun Apr 01 2012 Iain Arnell <iarnell@gmail.com> 0.003-1
 - Specfile autogenerated by cpanspec 1.79.

> FIX: Build-require `perl(inc::Module::Install)' (Makefile.PL:3) instead of
> `perl(ExtUtils::MakeMaker)'.
-BuildRequires:  perl(ExtUtils::MakeMaker)
 BuildRequires:  perl(ExtUtils::ParseXS) >= 2.21
+BuildRequires:  perl(inc::Module::Install)
+BuildRequires:  perl(Module::Install::AuthorTests)
Ok.

> TODO: Consider removing bundled Perl modules under inc/ and build-requiring
> appropriate modules like Module::Install::AuthorTests. Otherwise you need explicitly
> build-require all dependencies of inc/* code.
> FIX: Build-require modules used by code under inc/ (ExtUtils::Manifest, YAML::Tiny,
> etc.). I really recommend unbundling inc/*.
Well, you build-require the inc::Module::Install and Module::Install::AuthorTests now, but because you left the bundled files in inc, they are still used instead of the system ones:

# strace -eopen perl Makefile.PL 2>&1 |grep Install
open("/usr/share/perl5/vendor_perl/inc/Module/Install.pm", O_RDONLY) = 4
open("inc/Module/Install.pm", O_RDONLY) = 4
open("inc/Module/Install/XSUtil.pm", O_RDONLY) = 4
open("inc/Module/Install/Base.pm", O_RDONLY) = 5
open("inc/Module/Install/AuthorTests.pm", O_RDONLY) = 4
open("inc/Module/Install/Repository.pm", O_RDONLY) = 4
open("inc/Module/Install/WriteAll.pm", O_RDONLY) = 4
open("inc/Module/Install/Metadata.pm", O_RDONLY) = 4
open("inc/Module/Install/Can.pm", O_RDONLY) = 4
open("inc/Module/Install/Makefile.pm", O_RDONLY) = 4

TODO: That means you still need to build-require all modules used from those files. I believe you can see the reasoning for removing the inc directory now and you will do it.

> TODO: Build-require `perl(threads)' for optional tests (t/10_threads.t:4).
+BuildRequires:  perl(threads)
Ok.

> TODO: Build-require `perl(constant)' which can dual-live in the future
> (t/10_threads.t:4).
+BuildRequires:  perl(constant)
Ok.

All tests pass. Ok.

> TODO: Do not explicitly run-require auto-discovered perl(Exporter).
$ rpm -q --requires -p ../RPMS/x86_64/perl-Data-Clone-0.003-2.fc18.x86_64.rpm |sort |uniq -c
      1 libc.so.6()(64bit)
      1 libc.so.6(GLIBC_2.11)(64bit)
      1 libc.so.6(GLIBC_2.2.5)(64bit)
      1 perl >= 0:5.008_001
      2 perl(Exporter)
      1 perl(:MODULE_COMPAT_5.14.2)
      1 perl(parent)
      1 perl(strict)
      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)
Not addressed. Ok.

Package build is F18 (http://koji.fedoraproject.org/koji/taskinfo?taskID=4124880). Ok.


Please consider purging the inc directory before building this package.

Resolution: Package APPROVED.
Comment 8 Iain Arnell 2012-06-06 07:51:21 EDT
New Package SCM Request
=======================
Package Name: perl-Data-Clone
Short Description: Polymorphic data cloning
Owners: iarnell
Branches: f16 f17
InitialCC: perl-sig
Comment 9 Gwyn Ciesla 2012-06-06 09:19:28 EDT
Git done (by process-git-requests).
Comment 10 Fedora Update System 2012-06-09 12:33:33 EDT
perl-Data-Clone-0.003-2.fc16.1 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/perl-Data-Clone-0.003-2.fc16.1
Comment 11 Fedora Update System 2012-06-09 12:34:15 EDT
perl-Data-Clone-0.003-2.fc17.1 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/perl-Data-Clone-0.003-2.fc17.1
Comment 12 Fedora Update System 2012-06-09 21:31:30 EDT
perl-Data-Clone-0.003-2.fc16.1 has been pushed to the Fedora 16 testing repository.
Comment 13 Fedora Update System 2012-06-17 18:21:52 EDT
perl-Data-Clone-0.003-2.fc16.1 has been pushed to the Fedora 16 stable repository.
Comment 14 Fedora Update System 2012-06-17 18:24:57 EDT
perl-Data-Clone-0.003-2.fc17.1 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.