Bug 1119369 - Review Request: speedtest-cli - Command line interface for testing internet bandwidth
Summary: Review Request: speedtest-cli - Command line interface for testing internet b...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Parag AN(पराग)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-07-14 15:40 UTC by Matías Kreder
Modified: 2014-10-10 16:09 UTC (History)
3 users (show)

Fixed In Version: speedtest-cli-0.3.0-1.fc20
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-10-10 16:09:49 UTC
Type: ---
Embargoed:
panemade: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Matías Kreder 2014-07-14 15:40:46 UTC
Spec URL: http://delete.fedorapeople.org/speedtest-cli/speedtest-cli.spec
SRPM URL: http://delete.fedorapeople.org/speedtest-cli/speedtest-cli-0.3.0-1.fc22.src.rpm
Description: Command line interface for testing internet bandwidth using speedtest.net
Fedora Account System Username: delete

[makerpm@localhost SPECS]$ rpmlint speedtest-cli.spec
rpmli0 packages and 1 specfiles checked; 0 errors, 0 warnings.
n[makerpm@localhost SPECS]$ rpmlint /home/makerpm/rpmbuild/SRPMS/speedtest-cli-0.3.0-1.fc22.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 1 Christopher Meng 2014-07-15 00:19:35 UTC
How did you package it???

No python3 BR, but lots of python3 stuffs.

Actually it supports python3, but if we haven't received the notice to switch to python3 as default, don't build based on py3. (f22 might be py3 as default, but f21 hasn't been released yet, no need to care about it so early)

What's the rationale of including MANIFEST.in as %doc?

This package mustn't be a arch-sepcific package, why do I see many optflags and sitelib? And where is noarch?

You'd better use pypi tarball: https://pypi.python.org/pypi/speedtest-cli

Comment 2 Parag AN(पराग) 2014-07-15 05:21:22 UTC
first I will suggest some changes here

1) This package does not do any compilations. This is a noarch package so just add
BuildArch: noarch

2) Then you will not needed any compilation flags to be set so remove 
CFLAGS="$RPM_OPT_FLAGS" from both the lines

3) For Fedora and EPEL6/EPEL7 you will not need to remove buildroot in %install
drop following line
rm -rf %{buildroot}

See https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

4) another thing I found Pypi already hosts this package source but did not include man page or modified the upstream tarball so don't use that keep the github source only.

5) I see both the binaries are same speedtest and speedtest-cli. Check with upstream to fix this and provide only speedtest-cli. In the spec file you can add to %install
rm -f $RPM_BUILD_ROOT%{_bindir}/speedtest

6) Decide first if you want python3 package as well or not?

7) you may want to install man page manually as by adding to %install
gzip speedtest-cli.1
mkdir -p $RPM_BUILD_ROOT%{_mandir}/man1
install -p -m 644 speedtest-cli.1.gz $RPM_BUILD_ROOT%{_mandir}/man1/speedtest-cli.1.gz

Comment 3 Matías Kreder 2014-08-01 19:43:36 UTC
Hi, I'm sorry for the delay and the spec, this is my first python related package and I tried to grab some parts of code of the package guidelines for Python but looks like I went the wrong way with they python3 stuff.
Anyway, I have updated the spec to meet your requirements.

Spec URL: http://delete.fedorapeople.org/speedtest-cli/speedtest-cli.spec
SRPM URL: http://delete.fedorapeople.org/speedtest-cli/speedtest-cli-0.3.0-1.fc22.src.rpm

Comment 4 Parag AN(पराग) 2014-08-08 11:14:11 UTC
sorry I am at the Flock conference so could not get time to review this. Will check above update by coming Monday.

Comment 5 Parag AN(पराग) 2014-08-22 05:36:12 UTC
Sorry for being late.

Only 2 issues are remaining now

1) rpmlint on binary rpm is showing
speedtest-cli.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/speedtest_cli.py 0644L /usr/bin/env

=> this can be fixed by adding following in %prep, below setup line
sed -i -e '/^#!\//, 1d' *.py


2) You have written in %files
%{python2_sitelib}/speedtest_cli.*
%{python2_sitelib}/speedtest_cli-0.3.0-py2.7.egg-info/*

we need this to be changed to have directory
%{python2_sitelib}/speedtest_cli-0.3.0-py2.7.egg-info owned by this package.

which should be written as
%{python2_sitelib}/speedtest_cli.py*
%{python2_sitelib}/speedtest_cli-0.3.0-py2.7.egg-info

Comment 6 Matías Kreder 2014-08-25 15:03:12 UTC
I have updated the spec file as you requested. Let me know if you have any other advice

Comment 8 Parag AN(पराग) 2014-09-20 02:11:59 UTC
oh. I am so sorry. I missed this review from my tracker. Thank you for reminding me.

The above package looks good.

APPROVED.

also note we are probably moving to using python3 in F22 so ask upstream if they have/can provide python3 compatible code.

Comment 9 Matías Kreder 2014-09-20 22:08:47 UTC
New Package SCM Request
=======================
Package Name: speedtest-cli
Short Description: Command line interface for testing internet bandwidth
Upstream URL: https://github.com/sivel/speedtest-cli
Owners: delete
Branches: f19 f20 f21 el6 epel7
InitialCC:

Comment 10 Gwyn Ciesla 2014-09-20 22:11:07 UTC
Git done (by process-git-requests).

Comment 11 Fedora Update System 2014-09-27 18:41:10 UTC
speedtest-cli-0.3.0-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/speedtest-cli-0.3.0-1.fc20

Comment 12 Fedora Update System 2014-09-29 04:04:22 UTC
speedtest-cli-0.3.0-1.fc20 has been pushed to the Fedora 20 testing repository.

Comment 13 Fedora Update System 2014-10-10 16:09:49 UTC
speedtest-cli-0.3.0-1.fc20 has been pushed to the Fedora 20 stable repository.


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