Bug 996311

Summary: Review Request: perl-CAD-Format-STL - Read and Write STL (STereo Lithography) format files
Product: [Fedora] Fedora Reporter: John C Peterson <jcp>
Component: Package ReviewAssignee: Petr Pisar <ppisar>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: i, notting, package-review, ppisar
Target Milestone: ---Flags: ppisar: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: perl-CAD-Format-STL-0.2.1-5.fc20 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-10-30 01:57:45 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description John C Peterson 2013-08-12 22:52:52 UTC
Spec URL: http://www.eskimo.com/~jcp/perl-CAD-Format-STL.spec
SRPM URL: http://www.eskimo.com/~jcp/perl-CAD-Format-STL-0.2.1-1.fc19.src.rpm

Description: The CAD::Format::STL perl module provides object-oriented methods to read and write files in STL (STereoLithography) format. Support is provided for both the ASCII and binary versions of the STL format.

Notes: This software is licensed under the same terms as Perl itself (GPL+ or Artistic), so there should not be any issues there. One of the build pre-requisites is perl-Class-Accessor-Classy, which should now be available from F18/19 updates. This package is a candidate for the 3D Printing Feature.

Fedora Account System Username: jcp

Comment 1 Christopher Meng 2013-08-12 23:56:26 UTC
1. Change 0%{?rhel} to 0%{?rhel} < 6 as many old stuffs are EL5 only, even EL6 you can safely drop them.

Remove this:

find %{buildroot} -depth -type d -exec rmdir {} 2>/dev/null \;

As ExtUtils has solved this "bug" years ago.

Move this:

%if 0%{?rhel} in %clean:

%clean
%if 0%{?rhel}
rm -rf %{buildroot}
%endif

to

%if 0%{?rhel}
%clean
rm -rf %{buildroot}
%endif

%defattr(-,root,root,-) is also EL5 only, EL6 can drop it.

Comment 2 John C Peterson 2013-08-13 06:19:06 UTC
Spec URL: http://www.eskimo.com/~jcp/perl-CAD-Format-STL.spec
SRPM URL: http://www.eskimo.com/~jcp/perl-CAD-Format-STL-0.2.1-2.fc19.src.rpm

Hi Christopher,

Thanks for taking time out to review my package.

I believe I have addressed all the issues you raised in this new version.

Upon closer examination, I think the conditionals need to look like this;

%if 0%{?rhel}
%if 0%{?rhel} < 6
Do something special for RHEL5 and older...
%endif
%endif

The outer test is needed, because on non-RHEL systems like Fedora, the %{?rhel} macro is not defined at all, so 0 < 6 evaluates to true on any non-RHEL system which is not what was intended. (I verified this with a quick test case).

Regards, John

Comment 3 Michael Schwendt 2013-08-13 09:06:05 UTC
True. That's pointed out at the bottom of the "Conditionals" section in the Wiki:
https://fedoraproject.org/wiki/Packaging:DistTag#Conditionals


You can use logical AND, however:

  %if 0%{?rhel} && 0%{?rhel} < 6
  ...
  %endif

Comment 4 John C Peterson 2013-08-13 18:58:39 UTC
Spec URL: http://www.eskimo.com/~jcp/perl-CAD-Format-STL.spec
SRPM URL: http://www.eskimo.com/~jcp/perl-CAD-Format-STL-0.2.1-3.fc19.src.rpm

This version incorporates Michael's suggestion to use the logical AND construct.

Comment 5 Petr Pisar 2013-10-16 08:44:57 UTC
URL is usable. Ok.
Source0 is usable. Ok.
Source tar ball is original (SHA-256: a8a1e743b51e7e0b82f4575f4474b2d1e608a85286232f45c42bf4661298dc5b). Ok.
Summary verified from lib/CAD/Format/STL.pm. Ok.
Description verified from lib/CAD/Format/STL.pm. Ok.
License verified from lib/CAD/Format/STL/part.pm and lib/CAD/Format/STL.pm. Ok.
No XS code, noarch BuildArch is Ok.

TODO: You can replace the `%{__perl}' macro with plain `perl'.

TODO: You can remove the deffattr macro completely, because even EPEL-5 has rpm-4.4 where the macro is redundant.

TODO: META.yml declares minimal Module::Build version 0.35. Make sure your package builds in EPEL-5 or add this constrain to the corresponding BuildRequire tag.

FIX: Remove `v' from `perl(Class::Accessor::Classy) >= v0.1.3' dependency declarations. See what perl-Class-Accessor-Classy package provides and how RPM compares the versions (rpmdev-vercmp v0.1.3 0).

TODO: Build-require `perl(bytes)' (lib/CAD/Format/STL.pm:421).

version module is optional for tests. Ok.

All tests pass. Ok.

$ rpmlint perl-CAD-Format-STL.spec ../SRPMS/perl-CAD-Format-STL-0.2.1-3.fc21.src.rpm ../RPMS/noarch/perl-CAD-Format-STL-0.2.1-3.fc21.noarch.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
rpmlint is Ok.

$ rpm -q -lv -p ../RPMS/noarch/perl-CAD-Format-STL-0.2.1-3.fc21.noarch.rpm 
drwxr-xr-x    2 root    root                        0 Oct 16 10:35 /usr/share/doc/perl-CAD-Format-STL
-rw-r--r--    1 root    root                      267 Apr  5  2010 /usr/share/doc/perl-CAD-Format-STL/Changes
-rw-r--r--    1 root    root                      286 Apr  5  2010 /usr/share/doc/perl-CAD-Format-STL/README
drwxr-xr-x    2 root    root                        0 Apr  5  2010 /usr/share/doc/perl-CAD-Format-STL/files
-rw-r--r--    1 root    root                     1417 Apr  5  2010 /usr/share/doc/perl-CAD-Format-STL/files/cube.stl
-rw-r--r--    1 root    root                      684 Apr  5  2010 /usr/share/doc/perl-CAD-Format-STL/files/cube_binary.stl
-rw-r--r--    1 root    root                     3342 Oct 16 10:35 /usr/share/man/man3/CAD::Format::STL.3pm.gz
-rw-r--r--    1 root    root                     2544 Oct 16 10:35 /usr/share/man/man3/CAD::Format::STL::part.3pm.gz
drwxr-xr-x    2 root    root                        0 Oct 16 10:35 /usr/share/perl5/vendor_perl/CAD
drwxr-xr-x    2 root    root                        0 Oct 16 10:35 /usr/share/perl5/vendor_perl/CAD/Format
drwxr-xr-x    2 root    root                        0 Oct 16 10:35 /usr/share/perl5/vendor_perl/CAD/Format/STL
-rw-r--r--    1 root    root                    11740 Oct 16 10:35 /usr/share/perl5/vendor_perl/CAD/Format/STL.pm
-rw-r--r--    1 root    root                     2297 Oct 16 10:35 /usr/share/perl5/vendor_perl/CAD/Format/STL/part.pm
File location and permissions are Ok.

$ rpm -q --requires -p ../RPMS/noarch/perl-CAD-Format-STL-0.2.1-3.fc21.noarch.rpm | sort -i | uniq -c
      1 perl(:MODULE_COMPAT_5.18.1)
      1 perl(CAD::Format::STL::part)
      1 perl(Carp)
      1 perl(Class::Accessor::Classy)
      1 perl(Class::Accessor::Classy) >= v0.1.3
      1 perl(strict)
      1 perl(warnings)
      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
FIX: Remove `v' from `perl(Class::Accessor::Classy) >= v0.1.3' dependency declarations.

TODO: Filter the under-specified dependency `perl(Class::Accessor::Classy)'. See <https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering>.

TODO: Run-require `perl(bytes)' (lib/CAD/Format/STL.pm:421).

$ rpm -q --provides -p ../RPMS/noarch/perl-CAD-Format-STL-0.2.1-3.fc21.noarch.rpm | sort -i | uniq -c
      1 perl(CAD::Format::STL) = 0.2.1
      1 perl(CAD::Format::STL::part) = 0.2.1
      1 perl-CAD-Format-STL = 0.2.1-3.fc21
Binary provides are Ok.

$ resolvedeps rawhide ../RPMS/noarch/perl-CAD-Format-STL-0.2.1-3.fc21.noarch.rpm 
Binary dependencies resolvable. Ok.

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

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

Please correct all `FIX' items, consider fixing `TODO' items, and provide new SPEC file.
Resolution: Package NOT approved.

Comment 6 John C Peterson 2013-10-17 06:22:11 UTC
Spec URL: http://www.eskimo.com/~jcp/perl-CAD-Format-STL.spec
SRPM URL: http://www.eskimo.com/~jcp/perl-CAD-Format-STL-0.2.1-4.fc19.src.rpm

Hi Petr,

Thank you for taking time out to review my package.

This version should correct all of the issues you raised (all of the FIX and TODO items as well).

I modified the BuildRequires for perl(Module::Build) to require >= 0.35. I did not check to see if it builds under EPEL-5 as I was planning to support EPEL-6 only for now (and EPEL-7, after RHEL-7 is released).

Good eye on the "use bytes" which I missed!

Regards, John

Comment 7 Petr Pisar 2013-10-17 09:10:17 UTC
Spec file changes:

--- perl-CAD-Format-STL.spec.old        2013-08-13 20:53:41.000000000 +0200
+++ perl-CAD-Format-STL.spec    2013-10-17 02:08:27.000000000 +0200
@@ -1,29 +1,32 @@
 # Declare the CPAN name of the module
 %define mod_basename CAD-Format-STL
+# RPM's auto require for perl(Class::Accessor::Classy) fails, handle manually
+%define __requires_exclude ^perl\\(Class::Accessor::Classy\\)$

 Name:           perl-%{mod_basename}
 Version:        0.2.1
-Release:        3%{?dist}
-Summary:        Read and Write STL (STereoLithography) format files
+Release:        4%{?dist}
+Summary:        Read and Write STL (STereo Lithography) format files
 License:        GPL+ or Artistic
 Group:          Development/Libraries
 URL:            http://search.cpan.org/dist/%{mod_basename}/
 Source:         http://www.cpan.org/modules/by-module/CAD/%{mod_basename}-v%{version}.tar.gz
 BuildArch:      noarch
-BuildRequires:  perl
 BuildRequires:  perl(strict)
 BuildRequires:  perl(warnings)
 BuildRequires:  perl(Carp)
-BuildRequires:  perl(Class::Accessor::Classy) >= v0.1.3
-BuildRequires:  perl(Module::Build)
+BuildRequires:  perl(Class::Accessor::Classy) >= 0.1.3
+BuildRequires:  perl(Module::Build) >= 0.35
 # These are needed for "Build test"
+BuildRequires:  perl(bytes)
 BuildRequires:  perl(Test::More)
+Requires:       perl(bytes)
 Requires:       perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))
-Requires:       perl(Class::Accessor::Classy) >= v0.1.3
+Requires:       perl(Class::Accessor::Classy) >= 0.1.3

 %description
 The CAD::Format::STL perl module provides object-oriented methods to read
-and write files in STL (STereoLithography) format. Support is provided
+and write files in STL (STereo Lithography) format. Support is provided
 for both the ASCII and binary versions of the STL format.

 %prep
@@ -31,7 +34,7 @@

 %build
 # Using Module::Build since a Build.PL is present
-%{__perl} Build.PL installdirs=vendor
+perl Build.PL installdirs=vendor
 ./Build

 %install
@@ -50,14 +53,19 @@
 %endif

 %files
-%if 0%{?rhel} && 0%{?rhel} < 6
-%defattr(-,root,root,-)
-%endif
 %doc files Changes README
 %{perl_vendorlib}/*
 %{_mandir}/man3/*

 %changelog
+* Wed Oct 16 2013 John C. Peterson <jcp> 0.2.1-4
+- Various fixes that were required or suggested by the package reviewer:
+  Added the package's version number build requirement for perl(Module::Build)
+  Added filter rule for rpm's auto require of perl(Class::Accessor::Classy)
+  Added perl(bytes) to both the build and runtime requirements
+  Removed the redundant defattr macro from the files section
+  Replaced the __perl macro with just perl in the build section
+
 * Tue Aug 13 2013 John C. Peterson <jcp> 0.2.1-3
 - Some cosmetic tweaks to the macros that check for the specific case of RHEL.


> TODO: You can replace the `%{__perl}' macro with plain `perl'.
TODO: You can do the same at perl(:MODULE_COMPAT_*) definition.

> TODO: You can remove the deffattr macro completely, because even EPEL-5 has
> rpm-4.4 where the macro is redundant.
-%if 0%{?rhel} && 0%{?rhel} < 6
-%defattr(-,root,root,-)
-%endif
Ok.

> TODO: META.yml declares minimal Module::Build version 0.35. Make sure your
> package builds in EPEL-5 or add this constrain to the corresponding
> BuildRequire tag.
-BuildRequires:  perl(Module::Build)
+BuildRequires:  perl(Module::Build) >= 0.35
Ok.

> FIX: Remove `v' from `perl(Class::Accessor::Classy) >= v0.1.3' dependency
> declarations. See what perl-Class-Accessor-Classy package provides and how
> RPM compares the versions (rpmdev-vercmp v0.1.3 0).
-BuildRequires:  perl(Class::Accessor::Classy) >= v0.1.3
+BuildRequires:  perl(Class::Accessor::Classy) >= 0.1.3
Ok.

> TODO: Build-require `perl(bytes)' (lib/CAD/Format/STL.pm:421).
+BuildRequires:  perl(bytes)
Ok.

> FIX: Remove `v' from `perl(Class::Accessor::Classy) >= v0.1.3' dependency
> declarations.
-Requires:       perl(Class::Accessor::Classy) >= v0.1.3
+Requires:       perl(Class::Accessor::Classy) >= 0.1.3
Ok.

> TODO: Filter the under-specified dependency `perl(Class::Accessor::Classy)'.
+# RPM's auto require for perl(Class::Accessor::Classy) fails, handle manually
+%define __requires_exclude ^perl\\(Class::Accessor::Classy\\)$
Ok.
TODO: There is a convention to place the macro just before %description section.

> TODO: Run-require `perl(bytes)' (lib/CAD/Format/STL.pm:421).
+Requires:       perl(bytes)
Ok.

$ rpmlint perl-CAD-Format-STL.spec ../SRPMS/perl-CAD-Format-STL-0.2.1-4.fc21.src.rpm ../RPMS/noarch/perl-CAD-Format-STL-0.2.1-4.fc21.noarch.rpm 
2 packages and 1 specfiles checked; 0 errors, 0 warnings.
rpmlint is OPk.

$ rpm -q --requires -p ../RPMS/noarch/perl-CAD-Format-STL-0.2.1-4.fc21.noarch.rpm | sort -i | uniq -c
      1 perl(:MODULE_COMPAT_5.18.1)
      1 perl(CAD::Format::STL::part)
      1 perl(Carp)
      1 perl(Class::Accessor::Classy) >= 0.1.3
      1 perl(bytes)
      1 perl(strict)
      1 perl(warnings)
      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.

$ resolvedeps rawhide ../RPMS/noarch/perl-CAD-Format-STL-0.2.1-4.fc21.noarch.rpm 
Binary dependencies resolvable. Ok.

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

Package is good.

Please consider fixing the `TODO' item before building this package.
Resolution: Package APPROVED.

Comment 8 John C Peterson 2013-10-18 05:03:33 UTC
New Package SCM Request
=======================
Package Name: perl-CAD-Format-STL
Short Description: Read and Write STL (STereo Lithography) format files
Owners: jcp
Branches: f18 f19 f20 el6
InitialCC: perl-sig

Comment 9 Gwyn Ciesla 2013-10-18 12:50:36 UTC
Git done (by process-git-requests).

Comment 10 Fedora Update System 2013-10-19 17:57:27 UTC
perl-CAD-Format-STL-0.2.1-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/perl-CAD-Format-STL-0.2.1-5.fc18

Comment 11 Fedora Update System 2013-10-19 17:57:39 UTC
perl-CAD-Format-STL-0.2.1-5.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/perl-CAD-Format-STL-0.2.1-5.fc20

Comment 12 Fedora Update System 2013-10-19 17:57:49 UTC
perl-CAD-Format-STL-0.2.1-5.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/perl-CAD-Format-STL-0.2.1-5.el6

Comment 13 Fedora Update System 2013-10-19 17:57:58 UTC
perl-CAD-Format-STL-0.2.1-5.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/perl-CAD-Format-STL-0.2.1-5.fc19

Comment 14 Fedora Update System 2013-10-20 16:19:27 UTC
perl-CAD-Format-STL-0.2.1-5.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 15 Fedora Update System 2013-10-30 01:57:45 UTC
perl-CAD-Format-STL-0.2.1-5.fc18 has been pushed to the Fedora 18 stable repository.

Comment 16 Fedora Update System 2013-10-30 01:58:57 UTC
perl-CAD-Format-STL-0.2.1-5.fc19 has been pushed to the Fedora 19 stable repository.

Comment 17 Fedora Update System 2013-11-04 17:34:59 UTC
perl-CAD-Format-STL-0.2.1-5.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 18 Fedora Update System 2013-11-10 07:06:41 UTC
perl-CAD-Format-STL-0.2.1-5.fc20 has been pushed to the Fedora 20 stable repository.