Spec URL: https://sourceforge.net/p/perltextbargraph/code/ci/master/tree/perl-Text-BarGraph.spec SRPM URL: http://sourceforge.net/projects/perltextbargraph/files/perl-Text-BarGraph-1.1-1.0.src.rpm/download Description: Text::BarGraph is a simple Perl module for generating ASCII bar graphs based on data in a hash, where the keys are labels and the values are magnitudes. It automatically scales the graph to fit on your terminal screen. It is very useful in making data more meaningful. For example, it can be used with statistics gathered from a log file. Fedora Account System Username: mindruv
Hi, This is an informal review as I cannot sponsor. URLS: It would be appreciated if you listed a direct download URL. At present, the URL given embeds the .spec file within a website. Attempting to wget the "direct download" link requires it be escaped and then provides a file named "perl-Text-BarGraph.spec?format=raw". BuildRoot: %{_tmppath}/%{name}-%{version}-build The above line is obsolete and should not be used. %define cpan_name Text-BarGraph Source0: %{cpan_name}-%{version}.tar.gz I believe this macro is quite redundant and could be addressed as a single line. # Text-BarGraph-1.1.tar.gz was downloaded from http://www.cpan.org/authors/id/K/ KB/KBAUCOM/Text-BarGraph-1.1.tar.gz, LICENSE file ammended to archive Am I correct in the understanding that you downloaded from the above URL, then altered the tarball with a LICENSE file? This means there is no programmatic method of downloading the upstream source and creating the one you used. A more correct method would be to define Source1: as containing the LICENSE file, then Source0: could be a proper URL. I believe this would be more in line with http://fedoraproject.org/wiki/Packaging:SourceURL which states: One of the design goals of rpm is to cleanly separate upstream source from vendor modifications. For the Fedora packager, this means that sources used to build a package should be the vanilla sources available from upstream. To help reviewers and QA scripts verify this, the packager needs to indicate where a reviewer can find the source that was used to make the rpm. However, this leads me to the concern that you have listed license as: License: GPL+ or Artistic Everything in the actual source suggests it is using Artistic Clarified. find . -type f -print0 | xargs -0 chmod 644 I'm unsure what value the above line adds to the .spec file. The package fails to actually build in koji which should be a clear blocker: http://kojipkgs.fedoraproject.org//work/tasks/7717/5787717/build.log What I'm seeing is that "make pure_install" doesn't pay attention to the build root. DESTDIR=%{buildroot} might fix it depending on the Makefile. You may be able to remove this line altogether as you appear to have used the "install" command to copy appropriate files. I'm unable to review further due to the inability to build.
And, use %global, not %define.
> Release: 1.0 That's unusual enough to be a eye-brow raiser. > %defattr(-,root,root,755) %defattr is obsolete since RPM 4.4: https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions > %attr(644,root,root) %{_mandir}/man3/Text::BarGraph.3pm.gz > %attr(644,root,root) %{perl_vendorlib}/Text/BarGraph.pm %attr is overhead here. Fix up ordinary permission issues _early_ (in %prep or %install) with the %{_fixperms} macro, during "install" usage or with chmod. Restrict %attr usage to really special cases (setuid/gid, non-root owner/group), so you can grep spec files and locate such places easily. > Text::BarGraph.3pm.gz Prefer Text::BarGraph.3pm* since the compression applied by rpmbuild on-the-fly may be customised/changed/disabled.
Hello, Thank you for review will correct this. VM
I can help do the formal review...
Oh, and if you hadn't looked over the Perl guidelines before, please do so: https://fedoraproject.org/wiki/Packaging:Perl lots of good stuff there.
>This is an informal review as I cannot sponsor. thank you i appreciate your effort and attention >URLS: It would be appreciated if you listed a direct download URL. At present, the URL given embeds the .spec file within a website. Attempting to wget the "direct download" link requires it be escaped and then provides a file named "perl-Text-BarGraph.spec?format=raw". I will stop using sourceforge in the future, for now i am stuck with it :( > BuildRoot: %{_tmppath}/%{name}-%{version}-build >The above line is obsolete and should not be used. this slipped from me, fixed ># Text-BarGraph-1.1.tar.gz was downloaded from http://www.cpan.org/authors/id/K/ > KB/KBAUCOM/Text-BarGraph-1.1.tar.gz, LICENSE file ammended to archive >Am I correct in the understanding that you downloaded from the above URL, then altered the tarball with a LICENSE file? This means there is no programmatic method of downloading the upstream source and creating the one you used. >A more correct method would be to define Source1: as containing the LICENSE file, then Source0: could be a proper URL. fixed Source0 , Source1 URL to LICENSE file >#However, this leads me to the concern that you have listed license as: >#License: GPL+ or Artistic >#Everything in the actual source suggests it is using Artistic Clarified. changed to License: Artistic Clarified >find . -type f -print0 | xargs -0 chmod 644 >I'm unsure what value the above line adds to the .spec file. Fixed >The package fails to actually build in koji which should be a clear blocker: >http://kojipkgs.fedoraproject.org//work/tasks/7717/5787717/build.log previously was building only for root user, fixed. >What I'm seeing is that "make pure_install" doesn't pay attention to the build root. >DESTDIR=%{buildroot} might fix it depending on the Makefile. You may be able >to remove this line altogether as you appear to have used the "install" command to copy appropriate files. fixed Will continue with other review recommendations
>> Release: 1.0 > >>That's unusual enough to be a eye-brow raiser. i am sorry i don't follow you >> %defattr(-,root,root,755) > >%defattr is obsolete since RPM 4.4: >https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions fixed >> %attr(644,root,root) %{_mandir}/man3/Text::BarGraph.3pm.gz >> %attr(644,root,root) %{perl_vendorlib}/Text/BarGraph.pm fixed >> Text::BarGraph.3pm.gz fixed
>And, use %global, not %define. fixed
There remain still couple of things to be fixed, will fix them tomorrow. VM
> I will stop using sourceforge in the future, for now i am stuck with it :( Isn't it possible anymore to upload files into sf.net's project web space area temporarily? Alternatively, use the fedorapeople.org upload space (which new contributors may request prior to sponsorship even): https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Upload_Your_Package > Release: 1.0 Release starts at 1 or 0.1 if it's a pre-release: https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag And, of course, there is seldomly a reason not to use the %{?dist} tag: https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Using_the_.25.7B.3Fdist.7D_Tag
Querry regarding proper Licensing sent to legal.org. Will update once processed. VM
(In reply to Veaceslav Mindru from comment #12) > Querry regarding proper Licensing sent to legal.org. > Will update once processed. There is no reason to do so. This perl-dist applies what is pretty much the norm in most perl-dists on CPAN. Also, in general, there is NO requirement for package maintainers to add a LICENSE file rsp. to demand one from upstream. The Fedora convention is to mandate packagers to add a copy of the Licence file, iff upstream ships one.
Rex, I don't think he should be sponsored, no packages found built by him so far, and all his reviews are seemingly stalled now. If he can't respond to this after neatly a year, I will close this and you please drop the packager privilege of him.
ok, agreed. :( dropped sponsorship for now. Veaceslav, if you're ever interested in picking this all back up, please feel free to do so, and I'll be happy to re-sponsor you and help with reviews.