This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 1272187 - Review Request: google-api-python-client - Google APIs Client Library for Python
Review Request: google-api-python-client - Google APIs Client Library for Python
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Paul Howarth
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 1228389
  Show dependency treegraph
 
Reported: 2015-10-15 12:53 EDT by Michele Baldessari
Modified: 2015-12-09 17:00 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-12-09 17:00:26 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
paul: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Michele Baldessari 2015-10-15 12:53:48 EDT
Spec URL: http://acksyn.org/files/rpms/google-api-python-client/google-api-python-client.spec
SRPM URL: http://acksyn.org/files/rpms/google-api-python-client/google-api-python-client-1.4.2-1.fc22.src.rpm

Description: Written by Google, this library provides a small, flexible, and powerful Python client library for accessing Google APIs.


Fedora Account System Username: mbaldessari
Comment 1 Paul Howarth 2015-10-21 09:12:40 EDT
Some initial comments prior to a formal review:

 * Current upstream version is 1.4.2, not 1.4.1

 * I'd suggest replicating the runtime Requires: as BuildRequires:
   That way, not only should any future test suite run OK, but you'll
   find out at build-time rather than install-time if there are any
   dependency issues.

 * Version requirements from setup.py are not reflected in rpm deps.

 * Dependency on python-six is missing.

 * Comment referring to BZ 1205170 can probably be dropped.

 * Shellbang removal should be done in %prep rather than %install; if
   done before the copy to %{py3dir}, it can be done just once.

 * Typo in comment for 1.4.0-1: pythn3 missing "o".
Comment 2 Michele Baldessari 2015-10-21 10:42:08 EDT
Thanks Paul,

could it be that you took a look at some older packages? The ones in the 
description had most of the comments fixed.

(In reply to Paul Howarth from comment #1)
> Some initial comments prior to a formal review:
> 
>  * Current upstream version is 1.4.2, not 1.4.1

Done (see above)

>  * I'd suggest replicating the runtime Requires: as BuildRequires:
>    That way, not only should any future test suite run OK, but you'll
>    find out at build-time rather than install-time if there are any
>    dependency issues.

Ack, agreed

>  * Version requirements from setup.py are not reflected in rpm deps.

Done (see above)
 
>  * Dependency on python-six is missing.

Done (see above)

>  * Comment referring to BZ 1205170 can probably be dropped.

Done (see above)

>  * Shellbang removal should be done in %prep rather than %install; if
>    done before the copy to %{py3dir}, it can be done just once.

Nice catch, fixed
 
>  * Typo in comment for 1.4.0-1: pythn3 missing "o".

Done (see above)

Thanks for the feedback, Paul!

New packages:
Spec URL: http://acksyn.org/files/rpms/google-api-python-client/google-api-python-client.spec
SRPM URL: http://acksyn.org/files/rpms/google-api-python-client/google-api-python-client-1.4.2-2.fc23.src.rpm
Comment 3 Paul Howarth 2015-10-23 10:48:12 EDT
I'm pretty sure I clicked on the link from this review ticket. Anyway, it's up to date now. I just looked at the python packaging guidelines (https://fedoraproject.org/wiki/Packaging:Python) and it looks like the python package boilerplate has changed since I last looked at a python package, with a view to making Python 3 the default python somewhere down the line. Given that I don't think you're targeting anything older than F-23 with this package, I'd suggest using the modern packaging style:

 * Use python2-* dependencies rather than python-* dependencies where possible (currently python-httplib2, python-oauth2client and python-uri-templates don't have the necessary provides for this as yet, which should get fixed really)

 * Rename the python-google-api-client package to python2-google-api-client

 * Use the %python_provide macro as per the guidelines and sample spec

 * Use %py2_build, %py3_build, %py2_install and %py3_install as per the sample spec

Again, if you're only targeting F-23 onwards, you can dispense with the python3 build conditional and just do it unconditionally, which would simplify the spec quite a bit. Or do you have thoughts of supporting EPEL-7 perhaps?
Comment 4 Michele Baldessari 2015-11-28 14:09:01 EST
Thanks Paul!

I have no plans for now for EPEL-7 so I tweaked everything to make it more
conform to the latest python policy. I have just fixed up python-oauth2client and python-uri-templates, so it will need a bit of time until those python2-* 
deps show up in rawhide.

New packages:
Spec URL: http://acksyn.org/files/rpms/google-api-python-client/google-api-python-client.spec
SRPM URL: http://acksyn.org/files/rpms/google-api-python-client/google-api-python-client-1.4.2-3.fc23.src.rpm
Comment 5 Paul Howarth 2015-12-09 10:34:51 EST
Review
======

Provides
--------
$ rpm -qp --provides python2-google-api-client-1.4.2-3.fc24.noarch.rpm 
python-google-api-client = 1.4.2-3.fc24
python-google-api-client(x86-64) = 1.4.2-3.fc24
python2-google-api-client = 1.4.2-3.fc24

$ rpm -qp --provides python3-google-api-client-1.4.2-3.fc24.noarch.rpm 
python3-google-api-client = 1.4.2-3.fc24

Requires
--------
$ rpm -qp --requires python2-google-api-client-1.4.2-3.fc24.noarch.rpm 
python(abi) = 2.7
python-httplib2 >= 0.8
python2-oauth2client >= 1.4.6
python2-six >= 1.6.1
python2-uri-templates >= 0.6
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1

$ rpm -qp --requires python3-google-api-client-1.4.2-3.fc24.noarch.rpm 
python(abi) = 3.5
python3-httplib2 >= 0.8
python3-oauth2client >= 1.4.6
python3-six >= 1.6.1
python3-uri-templates >= 0.6
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1

Review Checklist
----------------

- rpmlint is silent
- package and spec file naming OK; difference between source and binary
  package names may confuse some users but makes sense
- package meets guidelines, specifically the Python ones
- license is Apache V2, OK for Fedora, matches spec
- license text is packaged in %license
- spec file written in English and is legible
- source tarball matches upstream, including timestamp
- source builds OK for x86_64 in Rawhide
- buildreqs sane and (mostly) versioned as per upstream's setup.py
- no locale data to worry about
- no binary libraries to worry about
- no bundled libraries included
- package is not intended to be relocatable (good)
- directory ownership is correct
- no duplicate files
- permissions are sane
- macro usage is consistent
- code, not content
- no large documentation items
- docs don't affect runtime
- not a GUI app, no desktop file needed
- filenames are all ASCII
- no scriptlets present
- no inter-package dependency issues
- %python_provide macro used correctly
- upstream egg is removed in %prep
- no dependencies need to be downloaded for build or test
- built egg-info is shipped correctly

Nits
----
Could specify python version build requirements:
python2-devel >= 2.7 (or 2.6 if python-argparse added)
python3-devel >= 3.3

Could append "/" to directory names in %files list for clarity

Could comment about the code in %prep removing shellbangs without
losing timestamps

No blocker issues - package is APPROVED.
Comment 6 Michele Baldessari 2015-12-09 12:42:35 EST
Thanks, Paul.

I requested the new package via pkgdb2. I shall try and address the nits you
pointed out before I import it.
Comment 7 Gwyn Ciesla 2015-12-09 13:34:13 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/google-api-python-client

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