Bug 1062942 - Review Request: perl-App-CSV - The CSV command line Tool
Review Request: perl-App-CSV - The CSV command line Tool
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Gwyn Ciesla
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-08 22:06 EST by Fabio Alessandro Locati
Modified: 2014-08-15 14:57 EDT (History)
6 users (show)

See Also:
Fixed In Version: perl-App-CSV-0.08-3.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-07-30 11:58:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
Proposed perl-App-CSV.spec (1.78 KB, text/x-rpm-spec)
2014-07-16 01:25 EDT, Ralf Corsepius
no flags Details

  None (edit)
Description Fabio Alessandro Locati 2014-02-08 22:06:35 EST
Spec URL: http://data.fabiolocati.com/fedora/csv/csv.spec
SRPM URL: http://data.fabiolocati.com/fedora/csv/csv-0.07-1.fc20.src.rpm
Description: This package provides a command-line tool to manipulate CSV (and other delimited, line-based) files.
Fedora Account System Username: fale

This is my first Fedora package from scratch (I've already created a patch for xls2csv that is possible to find at https://bugzilla.redhat.com/show_bug.cgi?id=1062668 and multiple Ubuntu packages) and I'm looking for a sponsor.
Comment 1 Fabio Alessandro Locati 2014-02-08 22:24:49 EST
Forgot to tell in first comment: it works on Koji too (http://koji.fedoraproject.org/koji/taskinfo?taskID=6508742)
Comment 2 Antonio Trande 2014-02-09 13:12:40 EST
Hi Fabio.

There is a specific wiki page for packaging of Perl software: http://fedoraproject.org/wiki/Packaging:Perl

I see a Buildroot line so this software will be packaged for EPEL5, too; or not ? See http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

I don't even see any documentation file (see http://fedoraproject.org/wiki/Packaging:Guidelines#Documentation). ;)
Comment 3 Fabio Alessandro Locati 2014-02-09 20:34:10 EST
Hi Antonio :).

Thank you for your inputs. I've had removed the BuildRoot and added the doc files that are available.

Here you can find the current version of the files:
- Spec URL: http://data.fabiolocati.com/fedora/csv/csv.spec
- SRPM URL: http://data.fabiolocati.com/fedora/csv/csv-0.07-1.fc20.src.rpm
- Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=6510467
Comment 4 Christopher Meng 2014-02-09 20:43:18 EST
1. It's a good habit to move BuildRequires above Requires:

2. No need to use %{__make}, just make.

3. Remove these:

rm -rf %{buildroot}

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

%clean section

%defattr(-,root,root,-)

4. If the permission is incorrect, correct it in %install, avoid %attr(0755, root, root) nowadays(or, say, always).

5. RPM can help gzip the manpages, so you can write this:

%{_mandir}/man1/%{name}.1*

The glob will let RPM check and finish the compressing.

6. Each time you do something of SPEC in review, please bump the Release:

Release:        1%{?dist}

to

Release:        2%{?dist}

And so forth.

Also in %changelog, please bump also:

* Mon Feb 10 2014 <> - 0.07-2
- Correct the issues mentioned in review(bug 1062942)

* Mon Feb 10 2014 <> - 0.07-1

7. Please scratch a build in Koji, and paste the job URL to here.
Comment 5 Fabio Alessandro Locati 2014-02-10 04:23:16 EST
Hi Christopher :),

Thank you for the inputs.
I've implemented all the changes you mention and I have a question about 6 and 7.

6. I thought that the always-bumping rule was applicable only for versions that have been "released". This believe has been "confirmed" by bug 1018038 which seems to have managed the spec file without bumping the version.

7. I thought I was already doing it. Am I doing it right or wrong?

Here you can find the current version of the files:
- Spec URL: http://data.fabiolocati.com/fedora/csv/csv.spec
- SRPM URL: http://data.fabiolocati.com/fedora/csv/csv-0.07-2.fc20.src.rpm
- Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=6511612
Comment 6 José Matos 2014-02-10 06:21:34 EST
(In reply to Fabio Alessandro Locati from comment #5)
>
> 6. I thought that the always-bumping rule was applicable only for versions
> that have been "released". This believe has been "confirmed" by bug 1018038
> which seems to have managed the spec file without bumping the version.

It should be done also for packages in the review process.

See another example of this discussion in
https://bugzilla.redhat.com/show_bug.cgi?id=1041924

The rationale is that it really helps the reviewer that notice what it has changed. It can become really confusing if you have different versions with the same release number. Been there, done that. :-)
Comment 7 Fabio Alessandro Locati 2014-02-10 06:24:54 EST
(In reply to José Matos from comment #6)
> It should be done also for packages in the review process.
>
> See another example of this discussion in
https://bugzilla.redhat.com/show_bug.cgi?id=1041924
>
> The rationale is that it really helps the reviewer that notice what it has
> changed. It can become really confusing if you have different versions with
> the same release number. Been there, done that. :-)

Thanks a lot for the explanation and the example :)
Comment 8 Fabio Alessandro Locati 2014-07-09 10:32:47 EDT
Hi,

I've just updated to 0.08. Can you please check it?

http://data.fabiolocati.com/fedora/csv/csv-0.08-1.el7.src.rpm
http://data.fabiolocati.com/fedora/csv/csv.spec

I've created the el7 srpm since I'm on RHEL7 at the moment, where it builds flawless :).
Comment 9 Fabio Alessandro Locati 2014-07-10 18:41:59 EDT
Koji link :) http://koji.fedoraproject.org/koji/taskinfo?taskID=7126311
Comment 10 Gwyn Ciesla 2014-07-15 14:42:28 EDT
2 things before the formal review:

1, at build time it complains that IO::String is missing, but still builds.
2, there are tests, so make test should be run in a %check section.  That seems to need additional modules, so BuildRequire those as well.
Comment 11 Ralf Corsepius 2014-07-16 00:28:32 EDT
Folks, I guess you missed you are about to package a perl-module?

This package is App::CSV on CPAN 
i.e. http://search.cpan.org/~gaal/App-CSV-0.08

=> this package should be named perl-App-CSV and the tarball be picked up from cpan and not from github.

Please remodel this package accordingly or close this Review Request and open a new one for perl-App-CSV.

@Fabio: What has happened here, is you accidentially having picked up a CPAN perl modules' authors developement archive at github ;)
Comment 12 Ralf Corsepius 2014-07-16 01:25:14 EDT
Created attachment 918315 [details]
Proposed perl-App-CSV.spec

To get you started, attached is a perl-App-CSV.spec, how I would write it.

I am well aware, this likely is overwhelming and confusing to newcomers, but it actually isn't that difficult to understand once being a bit familiar with perl-packaging.
Comment 13 Fabio Alessandro Locati 2014-07-16 11:30:30 EDT
Thanks to Jon and Ralf suggestions and examples, I've just created a new spec file.

- SPEC: http://data.fabiolocati.com/fedora/csv/perl-App-CSV.spec
- SRPM: http://data.fabiolocati.com/fedora/csv/perl-App-CSV-0.08-2.el7.src.rpm
- Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=7151772

I've noticed that:
1. is complains about Test::Pod::Coverage and therefore does not use it for the test (so it could be removed, since is no use)
2. The package perl-Test-TempDir is not available in EPEL5, EPEL6 and EPEL7, so this entry could be removed (used only for the 03-file test) to grant compilability in EL, at least until the package will be available.
Comment 14 Ralf Corsepius 2014-07-16 13:48:55 EDT
(In reply to Fabio Alessandro Locati from comment #13)
> Thanks to Jon and Ralf suggestions and examples, I've just created a new
> spec file.

> 1. is complains about Test::Pod::Coverage and therefore does not use it for
> the test (so it could be removed, since is no use)
Well upstream seems a bit silly or they are simply unable to cope with it ;)

I'd recommend to keep Test::Pod::Coverage, just in case upstream once should able to cope with it, however I am also OK with leaving out, because Pod-tests are not much of importance.

> 2. The package perl-Test-TempDir is not available in EPEL5, EPEL6 and EPEL7,
> so this entry could be removed (used only for the 03-file test) to grant
> compilability in EL, at least until the package will be available.
Well, my view is different:

BR: perl(Test::TempDir) can omitted for EPEL* (until it should be available there), but it should not be omitted on Fedora.

This can be achieved in 2 ways:
1. Either add an rpm conditional to BR it only on fedora, e.g. by adding something similar to this 
%{?fedora:BuildRequires: perl(Test::TempDir)}

2. Or by utilizing git after this package is imported in to Fedora's git.
I.e. by removing "BuildRequires: perl(Test::TempDir)" from the spec on the corresponding epel branches in git.

I for one would favor 2., because this allows to keep specs for newer distros free from backward-compatibility stuff (BuildRoot, rm -rf %buildroot, etc.) and keeps them simpler and easier readable - but this is just my personal preference.
Comment 15 Fabio Alessandro Locati 2014-07-16 15:52:05 EDT
Thankyou for your suggestions, Ralf :)
Comment 16 Gwyn Ciesla 2014-07-21 11:09:03 EDT
Is there a new SRPM and SPEC incorporating the above?
Comment 17 Fabio Alessandro Locati 2014-07-21 11:13:56 EDT
Hi Jon,

There is no new SRPM/SPEC incorportating the above Ralf's suggestions, since the #1 is already present in the last SRPM/SPEC and the #2 is about the different GIT branches versions policy (so, it will be applied as soon as a GIT repo will be available)
Comment 18 Gwyn Ciesla 2014-07-21 13:15:50 EDT
Gotcha:

- rpmlint checks return:

perl-App-CSV.src: W: strange-permission perl-App-CSV.spec 0600L
A file that you listed to include in your package has strange permissions.
Usually, a file should have 0644 permissions.

Trivial, but fix.

- package meets naming guidelines
- package meets packaging guidelines
- license ( MIT ) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 

So just the spec file perms.  Otherwise pretty clean.  Any practice reviews you've worked on?
Comment 19 Fabio Alessandro Locati 2014-07-22 11:28:27 EDT
Hi Jon :),

I've fixed the permissions of the SPEC file and you can find the koji build at http://koji.fedoraproject.org/koji/taskinfo?taskID=7178191.

(In reply to Jon Ciesla from comment #18)
> Any practice reviews you've worked on?

What do you mean?
Comment 20 Christopher Meng 2014-07-22 19:25:39 EDT
(In reply to Fabio Alessandro Locati from comment #19)
> (In reply to Jon Ciesla from comment #18)
> > Any practice reviews you've worked on?
> 
> What do you mean?

You need to prove yourself qualified to maintain the package. Because at first you even didn't name it correctly. So it's better to give you sometime to do some informal package reviews(review others' packages). You should feel lucky comparing to those people still waiting in the queue.
Comment 21 Gwyn Ciesla 2014-07-23 08:00:49 EDT
That's a bit harsh, Christopher.  It's mostly to provide a way to display an unserstanding of the Packaging Guidelines and the review process.

https://fedoraproject.org/wiki/Package_Review_Process

Find a review or two, go through the process on them but make sure they know you're not sponsored yet.  Then post links here, I'll have a look, and once you're sponsored and this is approved you can go back and finish those reviews and approve those packages.

And yes, Christopher, the queue is long.  I do a review or two when I can.  We all should.
Comment 22 Christopher Meng 2014-07-23 08:51:11 EDT
(In reply to Jon Ciesla from comment #21)
> That's a bit harsh, Christopher.  It's mostly to provide a way to display an
> unserstanding of the Packaging Guidelines and the review process.

@Jon:

No.

I didn't point to this one. But the fact is, some are sponsored within 3 days, some need to wait at least 3 months but still get no feedbacks. And actually some among these 3+3 are not even eligible to being as a packager, they just do a trivial python module which is not required by any packages else, or a perl one line script review and pretend to be a guru.

I don't want to start any flamewars here, but Fedora should really revise the guidelines to attract more devs instead of users to the packager group. Thanks to the @fedoraproject addr is not restricted like Debian or Gentoo, or more craps will come.

@Fabio:

Drop find $RPM_BUILD_ROOT -depth -type d -exec rmdir {} 2>/dev/null \;

This has been fixed in MakeMakers for years.
Comment 23 Fabio Alessandro Locati 2014-07-23 15:10:52 EDT
I did not wanted to argue, I was only looking for an explanation about a question that was obscure to me.

Looking for NEW bugs for 'Package Review' sorted by desc ID, the first one I've found is https://bugzilla.redhat.com/show_bug.cgi?id=1122577, so I've started with this one. I was not able to assign the bug to me nor to mark it as ASSIGNED, probably because I have not enough privileges on bugzilla.


Here there is the 0.08-3 version with the suggestion by Christoper :)
- SPEC: http://data.fabiolocati.com/fedora/csv/perl-App-CSV.spec
- SRPM: http://data.fabiolocati.com/fedora/csv/perl-App-CSV-0.08-3.el7.src.rpm
- Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=7187311
Comment 24 Gwyn Ciesla 2014-07-28 09:37:54 EDT
Ok, the package looks good, as does your practice review.  Please request sponsorship into the packager group in FAS and I'll sponsor you, then approve this.
Comment 25 Fabio Alessandro Locati 2014-07-29 18:56:56 EDT
Thank you Jon for sponsoring me as a Fedora packager :)
Comment 26 Fabio Alessandro Locati 2014-07-29 18:57:19 EDT
New Package SCM Request
=======================
Package Name: perl-App-CSV
Short Description: App::CSV Perl module
Upstream URL: http://search.cpan.org/dist/App-CSV/
Owners: fale
Branches: f19 f20 f21 el6 epel7
InitialCC:
Comment 27 Gwyn Ciesla 2014-07-30 06:10:14 EDT
Git done (by process-git-requests).
Comment 28 Fedora Update System 2014-07-30 11:50:06 EDT
perl-App-CSV-0.08-3.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/perl-App-CSV-0.08-3.fc20
Comment 29 Fedora Update System 2014-07-30 11:50:13 EDT
perl-App-CSV-0.08-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/perl-App-CSV-0.08-3.fc19
Comment 30 Fedora Update System 2014-07-30 11:50:20 EDT
perl-App-CSV-0.08-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/perl-App-CSV-0.08-3.el6
Comment 31 Fedora Update System 2014-08-09 03:32:26 EDT
perl-App-CSV-0.08-3.fc19 has been pushed to the Fedora 19 stable repository.
Comment 32 Fedora Update System 2014-08-09 03:33:27 EDT
perl-App-CSV-0.08-3.fc20 has been pushed to the Fedora 20 stable repository.
Comment 33 Fedora Update System 2014-08-15 14:57:04 EDT
perl-App-CSV-0.08-3.el6 has been pushed to the Fedora EPEL 6 stable repository.

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