Forgot to tell in first comment: it works on Koji too (http://koji.fedoraproject.org/koji/taskinfo?taskID=6508742)

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). ;)

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

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.

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

(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. :-)

(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 :)

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 :).

Koji link :) http://koji.fedoraproject.org/koji/taskinfo?taskID=7126311

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.

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 ;)

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.

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.

(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.

Thankyou for your suggestions, Ralf :)

Is there a new SRPM and SPEC incorporating the above?

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)

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?

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?

(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.

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.

(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.

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

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.

Thank you Jon for sponsoring me as a Fedora packager :)

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:

Git done (by process-git-requests).

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

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

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

perl-App-CSV-0.08-3.fc19 has been pushed to the Fedora 19 stable repository.

perl-App-CSV-0.08-3.fc20 has been pushed to the Fedora 20 stable repository.

perl-App-CSV-0.08-3.el6 has been pushed to the Fedora EPEL 6 stable repository.