Bug 605674 - Review Request: perl-IO-InSitu - Avoid clobbering files opened for both input and output
Review Request: perl-IO-InSitu - Avoid clobbering files opened for both input...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Unspecified
low Severity medium
: ---
: ---
Assigned To: Paul Howarth
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-18 10:31 EDT by Bill Pemberton
Modified: 2013-01-13 12:12 EST (History)
6 users (show)

See Also:
Fixed In Version: perl-IO-InSitu-0.0.2-6.fc17
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-03-30 23:14:44 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
paul: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Bill Pemberton 2010-06-18 10:31:16 EDT
Note that this is my first package submission.

Spec URL: http://wfp.fedorapeople.org/perl-IO-InSitu.spec
SRPM URL:http://wfp.fedorapeople.org/perl-IO-InSitu-0.0.2-1.fc13.src.rpm 
Description: This module provides a function called open_rw(), that is passed two
file names and returns two handles, one open for reading and the other
for writing. It's like doing to separate open() calls, except that it
detects cases where the input and output file are the same, and avoids
clobbering the input file when reopening it for output.
Comment 1 Paul Howarth 2010-11-25 10:10:18 EST
I'll look at this for you. Initial quick comments:

 * Spelling error in %description:
   "to separate open() calls" should be "two separate open() calls"

 * Manual runtime dependency on perl(Test::More) is not needed; there's no reference to Test::More in the resulting package and I guess the reference to it in META.yml is probably an error

 * Manual runtime dependency on perl(version) is redundant as rpm's automatic dependency generator adds it anyway

 * A manual runtime dependency of perl(IO::File) could be added as the module requires this via a "use base qw( IO::File )" construct

 * As a matter of style, I personally prefer:
     %{perl_vendorlib}/IO/
     %{_mandir}/man3/IO::InSitu.3pm*
   to:
     %{perl_vendorlib}/*
     %{_mandir}/man3/*
   as this makes it easier to tell what's included in the package from reading the spec file, and will help you spot if other modules are added to the package in a later upstream release, which may be worthy of a changelog mention.. However, this is not a blocker if you prefer the wildcard style.

 * The package provides perl(IO::File::SE) as there is a package of this name included in the InSitu.pm file; I think this provide needs filtering as the package is not in a file where perl would look for it if some other package needed it (there should be a separate perl-IO-File-SE package if that became necessary). You can find information on filtering provides at: https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering


Regarding sponsorship, you need to demonstrate your understanding and application of the Fedora Packaging Guidelines. The usual way to do this is to  point potential sponsors to mock reviews of other packages in the review queue that you've done, and/or other packages you've submitted for review. It's a little hard to tell from this submission as cpanspec produces pretty good specs automatically.

Do you have any plans to submit further packages for review, and if so, would they be just perl modules or other things as well?
Comment 2 Bill Pemberton 2010-11-30 09:45:09 EST
I've updated my rpm to match your suggestions.  The updated files are http://wfp.fedorapeople.org/perl-IO-InSitu.spec and http://wfp.fedorapeople.org/perl-IO-InSitu-0.0.2-2.fc14.src.rpm 

As for my future plans, it's a matter of finding packages that need packaging.  I've been maintaining local rpms for years, most are not perl.  They're typically things Fedora has orphaned or things not suitable for Fedora.  Neither are good candidates for getting approved as a packager.
Comment 3 Paul Howarth 2010-11-30 10:57:53 EST
Whilst the runtime dependency on perl(version) was redundant, the removed buildrequire of perl(version) wasn't, and the package now fails to build in mock.

Executing(%check): /bin/sh -e /var/tmp/rpm-tmp.StvRIX
+ umask 022
+ cd /builddir/build/BUILD
+ cd IO-InSitu-0.0.2
+ unset DISPLAY
+ ./Build test
#   Failed test 'use IO::InSitu;'
#   at t/00.load.t line 4.
#     Tried to use 'IO::InSitu'.
#     Error:  Can't locate version.pm in @INC (@INC contains: /builddir/build/BUILD/IO-InSitu-0.0.2/blib/lib /builddir/build/BUILD/IO-InSitu-0.0.2/blib/arch /usr/local/lib64/perl5 /usr/local/share/perl5 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5 .) at /builddir/build/BUILD/IO-InSitu-0.0.2/blib/lib/IO/InSitu.pm line 3.
# BEGIN failed--compilation aborted at /builddir/build/BUILD/IO-InSitu-0.0.2/blib/lib/IO/InSitu.pm line 3.
# Compilation failed in require at (eval 4) line 2.
# BEGIN failed--compilation aborted at (eval 4) line 2.
Use of uninitialized value $IO::InSitu::VERSION in concatenation (.) or string at t/00.load.t line 7.
# Testing IO::InSitu
# Looks like you failed 1 test of 1.
t/00.load.t .......
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests

If you've no further packages to submit, your best route to sponsorship would be to do a few pre-reviews of other packages in the review queue:

http://fedoraproject.org/PackageReviewStatus/NEW.html

If you do any, make a note of them here for a potential sponsor (such as me) to see.
Comment 4 Bill Pemberton 2010-11-30 11:34:34 EST
Serves me right for rushing and not actually testing it on mock myself.  Sorry to waste your time.   http://wfp.fedorapeople.org/perl-IO-InSitu.spec and http://wfp.fedorapeople.org/perl-IO-InSitu-0.0.2-3.fc14.src.rpm are updated to fix the build issue.
Comment 5 Paul Howarth 2010-11-30 17:11:44 EST
That's better. Just one minor quibble now, in the rpmlint output for the src.rpm:

perl-IO-InSitu.src:17: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 17)

Just expand the tab on line 17 and all will be well.

I'll try to do a full review tomorrow.
Comment 6 Paul Howarth 2010-12-01 07:34:01 EST
--------------------------------------------------------------------------------------
Review checklist:
--------------------------------------------------------------------------------------
- rpmlint output mostly OK (see below)
- package and spec file naming OK
- package meets guidelines
- license as same as Perl, spec matches actual license
- no bundled license file to include
- spec file written in English and is legible
- sources match upstream (content and timestamp identical)
- package builds OK in mock for Rawhide
- build requirements OK
- no locale data to worry about
- no shared libraries to worry about
- no bundled libraries to worry about
- package makes no attempt to be relocatable (which is good)
- directory ownership is fine
- no duplicate files
- macro usage is consistent
- package is code, not content
- no large docs to concern ourselves with
- docs don't affect runtime
- no header files to worry about
- no static or other libraries to worry about
- no subpackages created or needed
- no libtool archives present
- package is not a GUI app so does not need a desktop file
- no non-ascii filenames
- supplied test suite run in %check and passes in Rawhide
- no scriptlets present or needed
- no pkgconfig files to worry about

--------------------------------------------------------------------------------------
rpmlint output:
--------------------------------------------------------------------------------------
perl-IO-InSitu.noarch: W: spelling-error %description -l en_US rw -> re, r, w
perl-IO-InSitu.src: W: spelling-error %description -l en_US rw -> re, r, w
perl-IO-InSitu.src:17: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 17)
2 packages and 0 specfiles checked; 0 errors, 3 warnings.

Spelling "errors" are to be expected.
Mixed use of spaces and tabs can be trivially fixed by expanding the one tab in the spec.

--------------------------------------------------------------------------------------
requires:
--------------------------------------------------------------------------------------
perl(:MODULE_COMPAT_5.12.2)  
perl(IO::File)  
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(CompressedFileNames) <= 3.0.4-1
perl(base)  
perl(Carp)  
perl(File::Copy)  
perl(File::Temp)  
perl(strict)  
perl(version)  
perl(warnings)  
rpmlib(PayloadIsXz) <= 5.2-1

These are sane. The perl(base) requirement comes from "use base qw( IO::File )";
the manual perl(IO::File) dependency covers this.

--------------------------------------------------------------------------------------
provides:
--------------------------------------------------------------------------------------
perl(IO::InSitu)  
perl-IO-InSitu = 0.0.2-3.fc15

These are also reasonable but it would be better if the perl(IO::InSitu) provide was
versioned. The reason that it isn't is that the current auto-provides script can't
extract the version from this code:

package IO::InSitu;

use version; $VERSION = qv('0.0.2');

You'd have to write a custom provides script to fix that (I did one for
perl-Mail-Mbox-MessageParser), but I wouldn't say that was a blocker.
RPM itself may grow a better perl provides/requires checker once rpm 4.9 lands.

--------------------------------------------------------------------------------------
Summary
--------------------------------------------------------------------------------------

This package is good to go (assuming the tab in the spec is expanded).

Now I just need convincing that you understand the Fedora Packaging Guidelines,
and a small number of package pre-reviews should do the trick.
Comment 7 Bill Pemberton 2010-12-01 08:58:55 EST
http://wfp.fedorapeople.org/perl-IO-InSitu-0.0.2-4.fc14.src.rpm is updated to fix rpmlint warning about spaces and tabs.  Thank you so much for your tutelage.
Comment 8 Paul Howarth 2012-03-20 11:43:22 EDT
OK, time to take another look at this.

With rpm 4.9, the automatic dependency finder detects the "use base" construct and so you no longer need the explicit perl(IO::File) dependency for current Fedora versions. For all EPEL releases, which all have older versions of rpm, the IO::File module is bundled with the main perl package so it's not really needed there either. So I'd now suggest getting rid of that requirement.

Looking at build requirements, the module uses the following dual-lived modules that are not explicitly build-required:

* perl(base)
* perl(Carp)
* perl(File::Temp)
* perl(IO::File)

I'd add build-requires for those given that they may be dual-lived in Fedora either now or in the future.

It's not actually necessary to remove empty directories from the buildroot as rpm-build will not include them in the package unless they're included in the %files list, and it won't complain about them either.

rpm 4.9 now has a native method for filtering unwanted requires and provides. The method used in this spec file will work for current Fedora releases and EPEL 6 but not EPEL 5. If you want filters that work everywhere, you'll currently need to use two sets of filters - for example as I've done in the perl-SUPER package:

http://pkgs.fedoraproject.org/gitweb/?p=perl-SUPER.git;a=blob;f=perl-SUPER.spec

That's using rpm 4.9's native filtering and the old %__perl_provides macro as a fallback for older rpm versions.

Using macros for commands (e.g. %{__perl}, %{__id_u} -n) is now discouraged:
http://fedoraproject.org/wiki/Packaging:Guidelines#Macros

I'd just use "perl" and "id -nu" in these cases now.
Comment 9 Bill Pemberton 2012-03-21 10:08:48 EDT
Here's an update addressing the most recent comments.

Spec URL: http://wfp.fedorapeople.org/perl-IO-InSitu.spec
SRPM URL:http://wfp.fedorapeople.org/perl-IO-InSitu-0.0.2-5.fc16.src.rpm
Comment 10 Paul Howarth 2012-03-21 10:22:37 EDT
OK, last few nits:

%defattr is redundant in all currently supported releases, including EPEL-5.

You missed a %{__perl} in the :MODULE_COMPAT dependency.

Given that there's only one "provide" needing to be filtered, the "traditional" filter is over-escaped and you could simplify the grep to:
grep -Fvx 'perl(IO::File::SE)'

None of those are blockers though.

APPROVED.
Comment 11 Bill Pemberton 2012-03-21 10:38:40 EDT
New Package SCM Request
=======================
Package Name: perl-IO-InSitu
Short Description: Avoid clobbering files opened for both input and output
Owners: wfp
Branches: f16 f17
InitialCC: perl-sig
Comment 12 Gwyn Ciesla 2012-03-21 11:04:45 EDT
Git done (by process-git-requests).
Comment 13 Fedora Update System 2012-03-21 13:23:54 EDT
perl-IO-InSitu-0.0.2-6.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/perl-IO-InSitu-0.0.2-6.fc16
Comment 14 Fedora Update System 2012-03-21 13:32:53 EDT
perl-IO-InSitu-0.0.2-6.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/perl-IO-InSitu-0.0.2-6.fc17
Comment 15 Fedora Update System 2012-03-21 21:53:30 EDT
perl-IO-InSitu-0.0.2-6.fc16 has been pushed to the Fedora 16 testing repository.
Comment 16 Fedora Update System 2012-03-30 23:14:44 EDT
perl-IO-InSitu-0.0.2-6.fc16 has been pushed to the Fedora 16 stable repository.
Comment 17 Fedora Update System 2012-04-11 23:45:34 EDT
perl-IO-InSitu-0.0.2-6.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.