(Removing NEEDSPONSOR: bug 455067)

A couple of comments:

If you're going to, for whatever reason, use macros like %__make and %{__mv}, you need to be consistent:  either use the brackets or don't, and then you need to use %{__rm} instead of just "rm".  Or just drop the macros entirely and save the typing.

rpmlint complains about the .packlist file.  Honestly I'm not sure why you would even need to package it, and if you check the perl package template you'll see that it deletes .packlist files and then goes through and deletes empty directory trees.  In fact, perhaps you might want to take a look at the template; it might give you some hints for better ways to do some things.  Install the rpmdevtools package and look at /etc/rpmdevtools/spectemplate-perl.spec.

In any case, it's always useful to run rpmlint on your packages (both the source RPM and the final built packages).

(In reply to comment #2)
> A couple of comments:
> 
> If you're going to, for whatever reason, use macros like %__make and %{__mv},
> you need to be consistent:  either use the brackets or don't, and then you need
> to use %{__rm} instead of just "rm".  Or just drop the macros entirely and save
> the typing.
I do not see this so much sense to pay attention. But ok, I fix it.

> rpmlint complains about the .packlist file.  Honestly I'm not sure why you
> would even need to package it, and if you check the perl package template
> you'll see that it deletes .packlist files and then goes through and deletes
> empty directory trees.  In fact, perhaps you might want to take a look at the
> template; it might give you some hints for better ways to do some things. 
> Install the rpmdevtools package and look at
> /etc/rpmdevtools/spectemplate-perl.spec.
I have long thought about it file...
Very thanks for this hints. I get few lines fronm this spec template (see spec changelog for more details).

> In any case, it's always useful to run rpmlint on your packages (both the
> source RPM and the final built packages).
Off course!
Now it is produce only 1 warning, but it is too erroneously:

$ rpmlint -i xls2csv-1.06-1.fc9.src.rpm 
xls2csv.src:32: W: rpm-buildroot-usage %build %{__perl} Makefile.PL INSTALLDIRS="vendor" PREFIX="%{buildroot}%{_prefix}"
$RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it will
break short circuiting.

so, it is wrong, because usage of %{buildroot} there is not touch this dir!!! This is needed only for configuration build.

http://hubbitus.net.ru/rpm/Fedora9/xls2csv/xls2csv-1.06-1.fc9.src.rpm

· I don't see the point in first including %{buildroot} in both PREFIX and DESTDIR and then hacking around the doubled buildroot that this causes. As far as I can see the build works just as well without %{buildroot} in PREFIX. It eliminates the need for the hack and makes RPMlint happy.

· The guidelines say you shouldn't use both %{buildroot} and $RPM_BUILD_ROOT. Choose one or the other and stick to it. (https://fedoraproject.org/wiki/Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS)

· You changed "rm" to "%{__rm}", so to be consistent you should use "%{__chmod}" as well. (There doesn't seem to be a corresponding macro for "find".) Why do you use these macros by the way? What value do they add?

· According to the Perl packaging guidelines you shouldn't require Perl packages directly by name, but instead require the Perl modules by the "perl(Foo)" naming scheme. (https://fedoraproject.org/wiki/Packaging:Perl#Perl_Requires_and_Provides)

· Most of the explicit dependencies seem unnecessary, because RPMbuild will add dependencies automatically. ("Requires:" that is, not "BuildRequires:")

· Please replace the tabs in the spec with spaces. They make the file look ugly in my editor, and in my web browser too. Apparently your editor has a different idea of tab width than mine.

Björn Persson, thank you very much for the notes.

(In reply to comment #4)
> · I don't see the point in first including %{buildroot} in both PREFIX and
> DESTDIR and then hacking around the doubled buildroot that this causes. As far
> as I can see the build works just as well without %{buildroot} in PREFIX. It
> eliminates the need for the hack and makes RPMlint happy.
I fix it.

> · The guidelines say you shouldn't use both %{buildroot} and $RPM_BUILD_ROOT.
> Choose one or the other and stick to it.
> (https://fedoraproject.org/wiki/Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS)
Ok.

> · You changed "rm" to "%{__rm}", so to be consistent you should use
> "%{__chmod}" as well.
Ok.

> (There doesn't seem to be a corresponding macro for
> "find".)
Yes. I also found it strange.

> Why do you use these macros by the way? What value do they add?
Really now - nothing. But if, when pathes changed - only one system macros definition must be changed too opposite direct search/replace hundreds of values.

> · According to the Perl packaging guidelines you shouldn't require Perl
> packages directly by name, but instead require the Perl modules by the
> "perl(Foo)" naming scheme.
> (https://fedoraproject.org/wiki/Packaging:Perl#Perl_Requires_and_Provides)
> 
> · Most of the explicit dependencies seem unnecessary, because RPMbuild will add
> dependencies automatically. ("Requires:" that is, not "BuildRequires:")

I remove perl and prel-modules requires.
But ppmbuild don't add next requires:
perl(:MODULE_COMPAT_5.10.0)
perl-Unicode-Map or perl(Unicode::Map)
perl-libintl
as you think, its really don't needed (excuse me, I'm dont very similar with perl)?

>Please replace the tabs in the spec with spaces. They make the file look ugly
> in my editor, and in my web browser too. Apparently your editor has a
> different idea of tab width than mine.
Yes, I think 5 spaces tab width (it is convenient to easy distinguish pace and tabs indent in each editor). I think it is not issue. Sorry, I don't want change tabs to spaces.


http://hubbitus.net.ru/rpm/Fedora11/xls2csv/xls2csv.spec
http://hubbitus.net.ru/rpm/Fedora11/xls2csv/xls2csv-1.06-2.fc11.src.rpm

(In reply to comment #5)
> > Why do you use these macros by the way? What value do they add?
> Really now - nothing. But if, when pathes changed - only one system macros
> definition must be changed too opposite direct search/replace hundreds of
> values.

You mean if the commands were to be moved to some directory not in the default PATH?

If you were to write "/usr/bin/make", and then make were moved to /bin, then your script would break, but if you write just "make", then it will still work as long as make is in one of the directories listed in PATH. Moving these commands out of the default PATH would annoy lots of people very very much, so it won't happen. Therefore I think writing "make" is just as future-proof as "%{__make}", and it's easier to read. The macros aren't forbidden though, as far as I know.

> But ppmbuild don't add next requires:
> perl(:MODULE_COMPAT_5.10.0)

You should keep that one, according to the guidelines.

> perl-Unicode-Map or perl(Unicode::Map)

perl-Spreadsheet-ParseExcel depends on perl(Unicode::Map), so you get that pulled in by the automatic dependency on perl(Spreadsheet::ParseExcel)

> perl-libintl

When I build the package I get an automatic dependency on perl(Locale::Recode), which perl-libintl provides.

> Sorry, I don't want change tabs to spaces.

Well, if this is what you want others to see when they read your spec, it's your choice:
http://www.rombobjörn.se/diverse/xls2csv.png
It is a requirement that the spec be legible, but this misalignment isn't bad enough to make it illegible, so I suppose it's allowed.

(In reply to comment #6)
> Therefore I think writing "make" is just as future-proof as
> "%{__make}", and it's easier to read. The macros aren't forbidden though, as
> far as I know.
I thought about it.

> > But rpmbuild don't add next requires:
> > perl(:MODULE_COMPAT_5.10.0)
> 
> You should keep that one, according to the guidelines.
Ok.

> > perl-Unicode-Map or perl(Unicode::Map)
> perl-Spreadsheet-ParseExcel depends on perl(Unicode::Map), so you get that
> pulled in by the automatic dependency on perl(Spreadsheet::ParseExcel)
Hm... Such dependency may be broken in the future. Imagine: perl(Spreadsheet::ParseExcel) in the future may change implementation to do not use perl(Unicode::Map). Therefore, it is not mean what this package not use it also. So, I think it Requires: perl(Unicode::Map) should be explicit there.

> When I build the package I get an automatic dependency on perl(Locale::Recode),
> which perl-libintl provides.
Ok, I wasn't saw that.

> > Sorry, I don't want change tabs to spaces.
> Well, if this is what you want others to see when they read your spec, it's
> your choice:
> http://www.rombobjörn.se/diverse/xls2csv.png
> It is a requirement that the spec be legible, but this misalignment isn't bad
> enough to make it illegible, so I suppose it's allowed.  
Please, see explanations of Ralf Corsepius there - https://bugzilla.redhat.com/show_bug.cgi?id=510865#c23 . In any case I can't say more. Please keep in mind - I am very grateful that he came, but I did not ask him about it myself.

BTW, Björn Persson, thank you for the help. Half way done, don't you want review this package?

http://hubbitus.net.ru/rpm/Fedora11/xls2csv/xls2csv-1.06-3.fc11.src.rpm
http://hubbitus.net.ru/rpm/Fedora11/xls2csv/xls2csv.spec

(In reply to comment #7)
> > perl-Spreadsheet-ParseExcel depends on perl(Unicode::Map), so you get that
> > pulled in by the automatic dependency on perl(Spreadsheet::ParseExcel)
> Hm... Such dependency may be broken in the future. Imagine:
> perl(Spreadsheet::ParseExcel) in the future may change implementation to do not
> use perl(Unicode::Map). Therefore, it is not mean what this package not use it
> also. So, I think it Requires: perl(Unicode::Map) should be explicit there.

The question is, does xls2csv use Unicode::Map directly, or does it need Unicode::Map only because Spreadsheet::ParseExcel uses it? Unicode::Map isn't mentioned anywhere in the code, only in the documentation. I also searched for the names of the documented methods of Unicode::Map, and didn't find any of them. Therefore I think that xls2csv doesn't use Unicode::Map directly, and that if Spreadsheet::ParseExcel gets changed to not use Unicode::Map, then xls2csv won't need it either. I'm not a Perl expert however, so I may have missed something. You may want to ask for advice on Fedora-perl-devel-list.

> BTW, Björn Persson, thank you for the help. Half way done, don't you want
> review this package?  

I'm not qualified to do reviews. Comment and discuss like this is all I can do until I find a sponsor, but I think this package is close to being ready for approval.

There is one thing I haven't mentioned before because I wasn't sure what's right, namely the license field. Using "and" in the license field means that the package contains some files with one license and some other files with another license. That's not really possible when the whole program is only one file. The licensing guidelines also say that in such cases there must be a comment explaining what parts are covered by which license. (https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios)

So should it be "GPL+ or Artistic" or "GPLv2+ or Artistic"? I'm not sure. The Perl packaging document seems to say that "the same terms as Perl itself" should be translated to "GPL+ or Artistic" (http://fedoraproject.org/wiki/Packaging:Perl#License_tag), but the points it makes aren't about the distinction between GPL+ and GPLv2+. Perhaps you should ask on Fedora-perl-devel-list about this too.

Ok, you are agai right.
Readme says: "This script is free software; you can redistribute it and/or modify it under the same terms as Perl itself.", so, according to guidelines: http://fedoraproject.org/wiki/Packaging:Perl#License_tag I change License to "GPL+ or Artistic"

http://hubbitus.net.ru/rpm/Fedora11/xls2csv/xls2csv-1.06-4.fc11.src.rpm

rpmlint must be run on every package. The output should be posted in the review.
- OK, clean

The package must be named according to the Package Naming Guidelines.
- OK

The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
- ok

The package must meet the Packaging Guidelines & perl specific guidelines.
- ok

The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
- OK

The License field in the package spec file must match the actual license.
- OK

If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
- OK

The spec file must be written in American English.
- OK

The spec file for the package MUST be legible.
- OK

The sources used to build the package must match the upstream source, as provided in the spec URL.
- OK

The package MUST successfully compile and build into binary rpms on at least one primary architecture.
- OK

All build dependencies must be listed in BuildRequires.
- OK

The spec file MUST handle locales properly.
- OK

Ldconfig in %post and %postun.
- N/A

Relocatable package and /usr prefix.
- N/A

A package must own all directories that it creates.
- OK

A Fedora package must not list a file more than once in the spec file's %files listings.
- OK

Permissions on files must be set properly.
- OK

Each package must have a correct %clean section.
- OK

Each package must consistently use macros.
- OK

The package must contain code, or permissable content.
-OK

Large documentation files must go in a -doc subpackage.
-OK

If a package includes something as %doc, it must not affect the runtime of the application.
-OK

Header files must be in a -devel package.
-OK

Static libraries must be in a -static package.
-OK

Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' .
- N/A

Library with .so suffix must be in -devel package.
- N/A

In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
- N/A

Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
- N/A

Gui application and desktop-file-install.
- N/A

Packages must not own files or directories already owned by other packages.
- OK

At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
- OK

All filenames in rpm packages must be valid UTF-8.
- OK

-koji build: ok, http://koji.fedoraproject.org/koji/taskinfo?taskID=1670457
-application testing: working as expected

conclusion: Approved

Hi Pavel,

Please request CVS and submit build into bodhi.

Hi, Jan Klepek, sorry for delay

New Package CVS Request
=======================
Package Name: 
Short Description: 
Owners: hubbitus
Branches: F-10 F-11 EL-5
InitialCC:

Hi, Jan Klepek, sorry for delay

New Package CVS Request
=======================
Package Name: xls2csv
Short Description: A script that recodes a spreadsheet's charset and saves as CSV
Owners: hubbitus
Branches: F-10 F-11 EL-5
InitialCC:

CVS done.

I have just found that there is another package which is in fedora for long time which owns /bin/xls2csv. Therefore could you please rename your binary/manpage to something else during %install.

Otherwise it would lead to conflict when somebody installs catdoc package and after that they want to install xls2csv. http://fedoraproject.org/wiki/Packaging:Conflicts

I'm sorry that I forgot to check for this during review.

You are shure?
I try check it:
# repoquery --whatprovides /bin/xls2csv
but nothing found...

In what branches you found this conflict?

Not '/bin/xls2csv' but actually '/usr/bin/xls2csv'.
catdoc (in Fedora) includes this file.

Oh, yes /usr/bin/xls2csv there...

So, what you think how I should name it?

xlsTOcsv
convertxls2csv
convertXls2csv
convertXls2Csv
something other?

convertxls2csv sounds fine.
please take in mind that you need to rename man pages too

I don't know this package well so for renaming I will leave
it to you (convertxls2csv seems fine also for me)

xls2csv-1.06-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/xls2csv-1.06-5.fc10

xls2csv-1.06-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/xls2csv-1.06-5.fc11

xls2csv-1.06-5.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/xls2csv-1.06-5.el5

xls2csv-1.06-5.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update xls2csv'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/EL-5/FEDORA-EPEL-2009-0514

xls2csv-1.06-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

xls2csv-1.06-5.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

xls2csv-1.06-5.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.