Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 993636 (perl-Text-BarGraph) - Review Request: perl-Text-BarGraph - ASCII bargraph generator
Summary: Review Request: perl-Text-BarGraph - ASCII bargraph generator
Keywords:
Status: CLOSED NOTABUG
Alias: perl-Text-BarGraph
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW sartgraph
TreeView+ depends on / blocked
 
Reported: 2013-08-06 10:43 UTC by Veaceslav Mindru
Modified: 2014-07-23 15:32 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-07-23 15:32:57 UTC
i: fedora-review-


Attachments (Terms of Use)

Description Veaceslav Mindru 2013-08-06 10:43:53 UTC
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

Comment 1 Joshua Small 2013-08-07 02:07:17 UTC
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.

Comment 2 Christopher Meng 2013-08-07 04:12:57 UTC
And, use %global, not %define.

Comment 3 Michael Schwendt 2013-08-07 10:10:42 UTC
> 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.

Comment 4 Veaceslav Mindru 2013-08-07 10:22:59 UTC
Hello,

Thank you for review will correct this. 



VM

Comment 5 Rex Dieter 2013-08-07 12:43:43 UTC
I can help do the formal review...

Comment 6 Rex Dieter 2013-08-07 12:46:13 UTC
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.

Comment 7 Veaceslav Mindru 2013-08-07 19:54:52 UTC
>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

Comment 8 Veaceslav Mindru 2013-08-07 19:59:04 UTC
>> 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

Comment 9 Veaceslav Mindru 2013-08-07 19:59:57 UTC
>And, use %global, not %define. 

fixed

Comment 10 Veaceslav Mindru 2013-08-07 20:26:16 UTC
There remain still couple of things to be fixed, will fix them tomorrow. 

VM

Comment 11 Michael Schwendt 2013-08-07 20:41:46 UTC
> 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

Comment 12 Veaceslav Mindru 2013-08-13 15:52:36 UTC
Querry regarding proper Licensing sent to  legal@lists.fedoraproject.org. Will update once processed.

VM

Comment 13 Ralf Corsepius 2013-08-14 16:30:18 UTC
(In reply to Veaceslav Mindru from comment #12)
> Querry regarding proper Licensing sent to  legal@lists.fedoraproject.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.

Comment 14 Christopher Meng 2014-07-15 02:07:38 UTC
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.

Comment 15 Rex Dieter 2014-07-23 15:32:57 UTC
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.


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