Bug 822585 - Review Request: perl-Module-Install-Repository - Automatically sets repository URL from svn/svk/Git checkout
Review Request: perl-Module-Install-Repository - Automatically sets repositor...
Status: CLOSED RAWHIDE
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: 809931
Blocks: 822878 822881
  Show dependency treegraph
 
Reported: 2012-05-17 11:07 EDT by Jitka Plesnikova
Modified: 2012-05-22 10:28 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-22 10:28:37 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 Jitka Plesnikova 2012-05-17 11:07:13 EDT
Spec URL: http://jplesnik.fedorapeople.org/perl-Module-Install-Repository.spec
SRPM URL: http://jplesnik.fedorapeople.org/perl-Module-Install-Repository-0.06-1.fc18.src.rpm
Description: Module::Install::Repository is a Module::Install plugin to automatically figure out repository URL and set it via repository() which then will be added to resources under META.yml.
Comment 1 Petr Pisar 2012-05-21 10:18:21 EDT
Source file is original. Ok.
Summary verified from lib/Module/Install/Repository.pm. Ok.
License verified from lib/Module/Install/Repository.pm. Ok.
URL and Source0 are usable Ok.
Description verified from lib/Module/Install/Repository.pm. Ok.
no XS code, noarch BuildArch is Ok.

TODO: Build-require `perl(base)' (lib/Module/Install/Repository.pm:8).
TODO: Build-require `perl(File::Temp)' (t/01_find_repo.t:7).

FIX: Build-require all dependencies of inc/* (like `perl(threads::shared)' at inc/Test/Builder.pm:24) or unbundle modules that can be unbundled.

I just gave a try, `find inc -type f \! -name 'Repository.pm'' made the build
system happy.

TODO: You can run-require git, svn, darcs, hg, and svk as they can be used at run-time (lib/Module/Install/Repository.pm).

All tests pass. Ok.

$ rpmlint  perl-Module-Install-Repository.spec ../SRPMS/perl-Module-Install-Repository-0.06-1.fc18.src.rpm ../RPMS/noarch/perl-Module-Install-Repository-0.06-1.fc18.noarch.rpm 
perl-Module-Install-Repository.src: W: spelling-error Summary(en_US) svn -> sen, sin, son
perl-Module-Install-Repository.src: W: spelling-error Summary(en_US) svk -> Sven
perl-Module-Install-Repository.noarch: W: spelling-error Summary(en_US) svn -> sen, sin, son
perl-Module-Install-Repository.noarch: W: spelling-error Summary(en_US) svk -> Sven
2 packages and 1 specfiles checked; 0 errors, 4 warnings.

FIX: Capitalize `svn' and `svk' in these are names of version control systems like a Git.

$ rpm -q -lv -p ../RPMS/noarch/perl-Module-Install-Repository-0.06-1.fc18.noarch.rpm
drwxr-xr-x    2 root    root                        0 May 21 15:47 /usr/share/doc/perl-Module-Install-Repository-0.06
-rw-r--r--    1 root    root                      686 Aug 13  2009 /usr/share/doc/perl-Module-Install-Repository-0.06/Changes
-rw-r--r--    1 root    root                      588 Mar 19  2009 /usr/share/doc/perl-Module-Install-Repository-0.06/README
-rw-r--r--    1 root    root                     2219 May 21 15:47 /usr/share/man/man3/Module::Install::Repository.3pm.gz
drwxr-xr-x    2 root    root                        0 May 21 15:47 /usr/share/perl5/vendor_perl/Module
drwxr-xr-x    2 root    root                        0 May 21 15:47 /usr/share/perl5/vendor_perl/Module/Install
-rw-r--r--    1 root    root                     3158 Aug 13  2009 /usr/share/perl5/vendor_perl/Module/Install/Repository.pm
File permissions and layout are Ok.

$ rpm -q --requires -p ../RPMS/noarch/perl-Module-Install-Repository-0.06-1.fc18.noarch.rpm |sort |uniq -c
      1 perl >= 0:5.005
      1 perl(base)
      1 perl(:MODULE_COMPAT_5.14.2)
      1 perl(Module::Install::Base)
      1 perl(strict)
      1 perl(vars)
      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
Binary requires are Ok.

$ rpm -q --provides -p ../RPMS/noarch/perl-Module-Install-Repository-0.06-1.fc18.noarch.rpm |sort |uniq -c
      1 perl(Module::Install::Repository) = 0.06
      1 perl-Module-Install-Repository = 0.06-1.fc18
Binary provides are Ok.

$ resolvedeps rawhide ../RPMS/noarch/perl-Module-Install-Repository-0.06-1.fc18.noarch.rpm 
rawhide/primary_db                                       |  14 MB     00:01     
Binary dependencies resolvable. Ok.

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

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 2 Petr Pisar 2012-05-21 11:48:16 EDT
> I just gave a try, `find inc -type f \! -name 'Repository.pm'' made the build
> system happy.
I forgot to remove the files. It really does not work so straight-forward.

One solution is to:
(1) Remove inc
(2) Comment the auto_set_repository in Makefile.PL
(3) Run perl Makefile.PL
(4) Remove inc/.author
(5) Copy lib/Modules/Install/Repository.pm to inc
(6) Enable the auto_set_repository in Makefile.PL
(7) Run perl Makefile.PL again

Shorter solution is just to remove auto_set_repository from Makefile.PL because the only outcome of the reflexive cycle is put proper VCS URL into META.yml which we do not package either. Do not forget removing the autocreated inc/.author, so that xt/* tests would not run.

More thorough solution is to guard removing auto_set_repository from Makefile.Pl by perl_bootstrap macro because this extension becomes available after building this package.

I like most the second solution (dropping auto_set_repository from Makefile.PL unconditionally).
Comment 3 Jitka Plesnikova 2012-05-22 04:14:34 EDT
Updated.
Comment 4 Petr Pisar 2012-05-22 05:31:24 EDT
Spec file changes:

--- perl-Module-Install-Repository.spec.old     2012-05-17 17:06:13.000000000 +0200
+++ perl-Module-Install-Repository.spec 2012-05-22 10:12:10.000000000 +0200
@@ -9,13 +9,17 @@
 BuildArch:      noarch
 BuildRequires:  perl >= 0:5.005
 BuildRequires:  perl(inc::Module::Install)
-# Build-time inc-ed:
-# XXX: We cannot remove ./inc because it build-requires this module (bootstrap)
 BuildRequires:  perl(Module::Install::AuthorTests)
 BuildRequires:  perl(Module::Install::Base)
 BuildRequires:  perl(Module::Install::TestBase)
+# Tests
+BuildRequires:  perl(File::Temp)
 BuildRequires:  perl(Path::Class)
 BuildRequires:  perl(Test::More)
+# Optional tests
+BuildRequires:  perl(Test::Perl::Critic)
+BuildRequires:  perl(Test::Spelling)
+BuildRequires:  perl(Test::Pod) >= 1.00
 Requires:       perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

 %description
@@ -25,6 +29,10 @@

 %prep
 %setup -q -n Module-Install-Repository-%{version}
+rm -r inc
+sed -i -e '/^inc\// d' MANIFEST
+sed -i -e '/^auto_set_repository/ d' Makefile.PL
+find -type f -exec chmod -x {} +

 %build
 %{__perl} Makefile.PL INSTALLDIRS=vendor
@@ -34,6 +42,7 @@
 make pure_install PERL_INSTALL_ROOT=$RPM_BUILD_ROOT
 find $RPM_BUILD_ROOT -type f -name .packlist -exec rm -f {} \;
 find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \;
+find $RPM_BUILD_ROOT -type f -name .author -exec rm -f {} \;
 %{_fixperms} $RPM_BUILD_ROOT/*

 %check

> TODO: Build-require `perl(base)' (lib/Module/Install/Repository.pm:8).
Not reflected. Ok.

> TODO: Build-require `perl(File::Temp)' (t/01_find_repo.t:7).
+BuildRequires:  perl(File::Temp)
Ok.

> FIX: Build-require all dependencies of inc/* (like `perl(threads::shared)' at
> inc/Test/Builder.pm:24) or unbundle modules that can be unbundled.
+rm -r inc
+sed -i -e '/^inc\// d' MANIFEST
+sed -i -e '/^auto_set_repository/ d' Makefile.PL
Ok.

TODO: The package does not build locally if some Critic plug-ins and wordlists are installed because of failing optional tests:

PERL_DL_NONLAZY=1 /usr/bin/perl "-MExtUtils::Command::MM" "-e" "test_harness(0, 'inc', 'blib/lib', 'blib/arch')" t/*.t xt/*.t
t/00_compile.t .... ok
t/01_find_repo.t .. 1/2 cannot remove path when cwd is /tmp/xMrPg3p_CH for /tmp/xMrPg3p_CH:  at /home/test/rpmbuild/BUILD/Module-Install-Repository-0.06/inc/File/Temp.pm line 901.
t/01_find_repo.t .. ok
xt/perlcritic.t ... 1/1
#   Failed test 'Test::Perl::Critic for "lib/Module/Install/Repository.pm"'
#   at /usr/share/perl5/vendor_perl/Test/Perl/Critic.pm line 110.
#
# Perl::Critic found these violations in "lib/Module/Install/Repository.pm":
# Three-argument form of open used at line 53, column 9.  Three-argument open is not available until perl 5.6.  (Severity: 5)
# Looks like you failed 1 test of 1.
xt/perlcritic.t ... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests
xt/pod.t .......... ok
xt/podspell.t ..... 1/1
#   Failed test 'POD spelling for lib/Module/Install/Repository.pm'
#   at /usr/share/perl5/vendor_perl/Test/Spelling.pm line 163.
# Errors:
#     Matsuno
#     Tokuhiro
#     svn
# Looks like you failed 1 test of 1.
xt/podspell.t ..... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests

If you removed the inc/.author in %prep, you would not run xt/* tests and you would not need to build-requires the optional dependencies (and prune the file in %install; actually the file from inc/ does not get installed into RPM_BUILD_ROOT.)

$ rpmlint perl-Module-Install-Repository.spec ../SRPMS/perl-Module-Install-Repository-0.06-1.fc18.src.rpm ../RPMS/noarch/perl-Module-Install-Repository-0.06-1.fc18.noarch.rpm 
perl-Module-Install-Repository.src: W: spelling-error Summary(en_US) svn -> sen, sin, son
perl-Module-Install-Repository.src: W: spelling-error Summary(en_US) svk -> Sven
perl-Module-Install-Repository.noarch: W: spelling-error Summary(en_US) svn -> sen, sin, son
perl-Module-Install-Repository.noarch: W: spelling-error Summary(en_US) svk -> Sven
2 packages and 1 specfiles checked; 0 errors, 4 warnings.

TOD: Capitalize `svn' and `svk' in these are names of version control systems like a Git. Current wording is not consistent (it mixes command names and tool names).

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


Please consider fixing all `TODO' items before building this package.

Resolution: Package APPROVED.
Comment 5 Jitka Plesnikova 2012-05-22 07:33:56 EDT
Spec was updated.
Comment 6 Gwyn Ciesla 2012-05-22 08:23:19 EDT
Please include an SCM request when setting the fedora-cvs flag.
Comment 7 Jitka Plesnikova 2012-05-22 08:48:10 EDT
Package Name: perl-Module-Install-Repository
Short Description: Automatically sets repository URL from Svn/Svk/Git checkout
Owners: jplesnik mmaslano ppisar psabata
Branches:
InitialCC: perl-sig
Comment 8 Gwyn Ciesla 2012-05-22 09:08:55 EDT
Use the formatting here:

http://fedoraproject.org/wiki/Package_SCM_admin_requests
Comment 9 Jitka Plesnikova 2012-05-22 09:14:27 EDT
New Package SCM Request
=======================
Package Name: perl-Module-Install-Repository
Short Description: Automatically sets repository URL from Svn/Svk/Git checkout
Owners: jplesnik mmaslano ppisar psabata
Branches:
InitialCC: perl-sig
Comment 10 Gwyn Ciesla 2012-05-22 09:21:44 EDT
Git done (by process-git-requests).
Comment 11 Jitka Plesnikova 2012-05-22 10:28:37 EDT
Thank you for the review and the repository.

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