Bug 1128253 - Review Request: gerrymander - A client API and command line tool for gerrit
Summary: Review Request: gerrymander - A client API and command line tool for gerrit
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-08-08 17:18 UTC by Kashyap Chamarthy
Modified: 2015-03-22 04:42 UTC (History)
5 users (show)

Fixed In Version: gerrymander-1.4-1.fc21
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-03-22 04:42:21 UTC
panemade: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Kashyap Chamarthy 2014-08-08 17:18:21 UTC
Spec URL: https://kashyapc.fedorapeople.org/spec-files/gerrymander.spec
SRPM URL: https://kashyapc.fedorapeople.org/srpms/gerrymander-1.3-1.fc20.src.rpm
Description: The gerrymander package provides a set of command line tools
for interacting with Gerrit
Fedora Account System Username: kashyapc

Comment 1 Kashyap Chamarthy 2014-08-09 10:20:12 UTC
Successful Koji scratch build
-----------------------------

$ koji build --scratch rawhide ../SRPMS/gerrymander-1.3-1.fc20.src.rpm 
Uploading srpm: ../SRPMS/gerrymander-1.3-1.fc20.src.rpm
[====================================] 100% 00:00:01  33.37 KiB  17.33 KiB/sec
Created task: 7257733
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=7257733
Watching tasks (this may be safely interrupted)...
7257733 build (rawhide, gerrymander-1.3-1.fc20.src.rpm): open (buildhw-04.phx2.fedoraproject.org)
  7257736 buildArch (gerrymander-1.3-1.fc20.src.rpm, noarch): open (arm02-builder03.arm.fedoraproject.org)
  7257736 buildArch (gerrymander-1.3-1.fc20.src.rpm, noarch): open (arm02-builder03.arm.fedoraproject.org) -> closed
  0 free  1 open  1 done  0 failed
7257733 build (rawhide, gerrymander-1.3-1.fc20.src.rpm): open (buildhw-04.phx2.fedoraproject.org) -> closed
  0 free  0 open  2 done  0 failed

7257733 build (rawhide, gerrymander-1.3-1.fc20.src.rpm) completed successfully
kashyap@SPECS$

Comment 2 Parag AN(पराग) 2014-08-11 10:53:59 UTC
Hey Kashyap,
   Let me take this for review.

Comment 3 Parag AN(पराग) 2014-08-12 07:57:38 UTC
First few things to fix
1) We use %global over %define. Fix this

2) If not defined macro "%{?extra_release}" then we don't even need it. You can remove this.

3) Group tag is optional but can be kept if building for EPEL

4) Replace python with python2 wherever it applies.

5) Please have correct summary matching to package name

6) use python2 macros wherever applies

7) defattr is not needed 

8) This package is not compiling anything so we don't need
CFLAGS="$RPM_OPT_FLAGS" in %build

9) If upstream provides egginfo in source we need to remove it in %prep and package built egginfo in %install. You are removing it actually in %install

10) use source url as
Source0: https://github.com/berrange/%{name}/archive/v%{version}.tar.gz

Please provide updated package for further review and check the https://fedoraproject.org/wiki/Packaging:Python page. This package spec is not following those guidelines.

Comment 4 Kashyap Chamarthy 2014-08-19 06:15:01 UTC
Thank you Parag, for the review, and your help on IRC. I addressed all your comments (please see inline).

Updated SPEC: https://kashyapc.fedorapeople.org/spec-files/gerrymander.spec
Updated SRPM: https://kashyapc.fedorapeople.org/srpms/gerrymander-1.3-2.fc20.src.rpm

(In reply to Parag AN(पराग) from comment #3)
> First few things to fix
> 1) We use %global over %define. Fix this

Done - Replaced %define with %global

> 2) If not defined macro "%{?extra_release}" then we don't even need it. You
> can remove this.

Removed the "%{?extra_release}" macro.

> 3) Group tag is optional but can be kept if building for EPEL

I left the "Group" tag intact, in case we may have to build this for EPEL.

> 4) Replace python with python2 wherever it applies.

Done - s/python/python2

> 5) Please have correct summary matching to package name

Corrected summary for python2 package description.

> 6) use python2 macros wherever applies

Done - used python2 macros consistently wherever it applies.
 
> 7) defattr is not needed 

Removed %defattr section from %files, as it is now implied by default.

> 8) This package is not compiling anything so we don't need
> CFLAGS="$RPM_OPT_FLAGS" in %build

Done - Removed it.

> 9) If upstream provides egginfo in source we need to remove it in %prep and
> package built egginfo in %install. You are removing it actually in %install

Done - Changed egg info removal section from %install to %prep

> 
> 10) use source url as
> Source0: https://github.com/berrange/%{name}/archive/v%{version}.tar.gz

Done.

> 
> Please provide updated package for further review and check the
> https://fedoraproject.org/wiki/Packaging:Python page. This package spec is
> not following those guidelines.

Please review the updated SPEC to see if it complies the guidelines now.

Comment 5 Kashyap Chamarthy 2014-08-19 11:38:44 UTC
Successful Koji scratch build with the updated SPEC/SRPM:

$ koji build --scratch rawhide  /home/kashyap/rpmbuild/SRPMS/gerrymander-1.3-2.fc20.src.rpm 
Uploading srpm: /home/kashyap/rpmbuild/SRPMS/gerrymander-1.3-2.fc20.src.rpm
[====================================] 100% 00:00:02  33.54 KiB  13.79 KiB/sec
Created task: 7426618
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=7426618
Watching tasks (this may be safely interrupted)...
7426618 build (rawhide, gerrymander-1.3-2.fc20.src.rpm): open (buildvm-15.phx2.fedoraproject.org)
  7426619 buildArch (gerrymander-1.3-2.fc20.src.rpm, noarch): open (buildhw-04.phx2.fedoraproject.org)
7426618 build (rawhide, gerrymander-1.3-2.fc20.src.rpm): open (buildvm-15.phx2.fedoraproject.org) -> closed
  0 free  1 open  1 done  0 failed
  7426619 buildArch (gerrymander-1.3-2.fc20.src.rpm, noarch): open (buildhw-04.phx2.fedoraproject.org) -> closed
  0 free  0 open  2 done  0 failed

7426618 build (rawhide, gerrymander-1.3-2.fc20.src.rpm) completed successfully

Comment 6 Parag AN(पराग) 2014-08-19 13:14:29 UTC
1) [!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib/python3.4/site-
     packages/gerrymander/__pycache__, /usr/lib/python3.4/site-packages,
     /usr/lib/python2.7/site-packages/gerrymander, /usr/lib/python3.4/site-
     packages/gerrymander, /usr/lib/python3.4

Fix this

2) Add Requires: gerrymander to both subpackages

Comment 7 Kashyap Chamarthy 2014-08-19 13:24:19 UTC
Thanks a lot to Parag for your comments and help on IRC.

New SPEC and SRPM with some more changes with updated files section correctly and add correct "Requires".


SPEC: https://kashyapc.fedorapeople.org/spec-files/gerrymander.spec
SRPM: https://kashyapc.fedorapeople.org/srpms/gerrymander-1.3-3.fc20.src.rpm

Comment 8 Parag AN(पराग) 2014-08-19 13:26:30 UTC
I think we are good now. 

APPROVED.

Comment 9 Daniel Berrangé 2014-08-19 13:28:22 UTC
(In reply to Parag AN(पराग) from comment #6)
> Fix this
> 
> 2) Add Requires: gerrymander to both subpackages

That makes no sense. The subpackages exist precisely so the modules can be installed on their own without pulling in the /usr/bin command line tool.

Comment 10 Parag AN(पराग) 2014-08-19 13:51:59 UTC
so I can use gerrymander without installing python2-gerrymander and python3-gerrymander?

I installed gerrymander only and tried to run gerrymander
$ gerrymander
Traceback (most recent call last):
  File "/usr/bin/gerrymander", line 18, in <module>
    from gerrymander.commands import CommandTool
ImportError: No module named gerrymander.commands

Comment 11 Daniel Berrangé 2014-08-19 13:55:13 UTC
(In reply to Parag AN(पराग) from comment #10)
> so I can use gerrymander without installing python2-gerrymander and
> python3-gerrymander?
> 
> I installed gerrymander only and tried to run gerrymander
> $ gerrymander
> Traceback (most recent call last):
>   File "/usr/bin/gerrymander", line 18, in <module>
>     from gerrymander.commands import CommandTool
> ImportError: No module named gerrymander.commands

That says that 'gerrymander' needs a 'Requires: python3-gerrymander' dependency, *not* that 'python[3]-gerrymander' needs a 'Requires: gerrymander' dependency.

Comment 12 Parag AN(पराग) 2014-08-19 13:56:21 UTC
I don't remember any package that pulls its python2 and python3 subpackage for main package. Does anyone knows any such example in Fedora?

Comment 13 Daniel Berrangé 2014-08-19 14:10:32 UTC
(In reply to Parag AN(पराग) from comment #12)
> I don't remember any package that pulls its python2 and python3 subpackage
> for main package. Does anyone knows any such example in Fedora?

It is totally normal packaging practice to choose to structure things so the CLI tool is separate from the library code. It means people can simply 'yum install gerrymander' without having to know/care whether it is currently using  the python2 or python3 library modules

Comment 14 Kashyap Chamarthy 2014-08-19 14:28:03 UTC
Addressed Dan's concern,removed the needless circular dependency of "Requires: gerrymander". 


Updated:

SPEC - https://kashyapc.fedorapeople.org/spec-files/gerrymander.spec
SRPM - https://kashyapc.fedorapeople.org/srpms/gerrymander-1.3-3.fc20.src.rpm

Comment 15 Bohuslav "Slavek" Kabrda 2014-08-19 14:30:02 UTC
It makes sense for the main gerrymander package to depend *either* on python-gerrymander *or* python3-gerrymander, so that the users just install "gerrymander" and don't care about the dependencies. It however wouldn't make sense to me to install *both* python-gerrymander *and* python3-gerrymander - /usr/bin/gerrymander can only import one, anyway.
The specfile seems fine, but you should take care of the situation when with_python3 is "0" - in this situation, /usr/bin/gerrymander wouldn't work.
I'd solve it like this (incomplete example):

# in the main package
%if 0%{with_python3}
Requires: python3-gerrymander
%else
Requires: python-gerrymander
%endif

# in prep
%if 0%{with_python3}
# make sure that hashbang points to /usr/bin/python3 explicitly
%else
# make sure that hashbang points to /usr/bin/python2 explicitly
%endif

Hope this makes sense.

Comment 16 Daniel Berrangé 2014-08-20 08:51:58 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #15)
> The specfile seems fine, but you should take care of the situation when
> with_python3 is "0" - in this situation, /usr/bin/gerrymander wouldn't work.
> I'd solve it like this (incomplete example):

Even simpler is to just delete the 'with_python3' conditional - all current Fedora releases have python3  so might as well just rely on it existing.

Comment 17 Bohuslav "Slavek" Kabrda 2014-08-20 09:17:29 UTC
(In reply to Daniel Berrange from comment #16)
> (In reply to Bohuslav "Slavek" Kabrda from comment #15)
> > The specfile seems fine, but you should take care of the situation when
> > with_python3 is "0" - in this situation, /usr/bin/gerrymander wouldn't work.
> > I'd solve it like this (incomplete example):
> 
> Even simpler is to just delete the 'with_python3' conditional - all current
> Fedora releases have python3  so might as well just rely on it existing.

Yes, assuming the maintainer doesn't want to also put this package in EPEL and keep the same specfile that he has in Fedora.

Comment 18 Kashyap Chamarthy 2014-08-20 12:35:51 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #17)
> (In reply to Daniel Berrange from comment #16)
> > (In reply to Bohuslav "Slavek" Kabrda from comment #15)
> > > The specfile seems fine, but you should take care of the situation when
> > > with_python3 is "0" - in this situation, /usr/bin/gerrymander wouldn't work.
> > > I'd solve it like this (incomplete example):
> > 
> > Even simpler is to just delete the 'with_python3' conditional - all current
> > Fedora releases have python3  so might as well just rely on it existing.
> 
> Yes, assuming the maintainer doesn't want to also put this package in EPEL
> and keep the same specfile that he has in Fedora.

For now, I don't have any intentions to make an EPEL package, only for Fedora. If there's strong interest for EPEL, and someone wants to co-maintain, they're more than welcome.

Comment 19 Parag AN(पराग) 2014-08-21 07:18:38 UTC
(In reply to Bohuslav "Slavek" Kabrda from comment #17)
> (In reply to Daniel Berrange from comment #16)
> > (In reply to Bohuslav "Slavek" Kabrda from comment #15)
> > > The specfile seems fine, but you should take care of the situation when
> > > with_python3 is "0" - in this situation, /usr/bin/gerrymander wouldn't work.
> > > I'd solve it like this (incomplete example):
> > 
> > Even simpler is to just delete the 'with_python3' conditional - all current
> > Fedora releases have python3  so might as well just rely on it existing.
> 
> Yes, assuming the maintainer doesn't want to also put this package in EPEL
> and keep the same specfile that he has in Fedora.

So do we need to create python-gerrymander or skip it? The script gerrymander points to python3 hashbang. If we think on dropping with_python3 conditionals then drop python-gerrymander also and just package gerrymander and python3-gerrymander.

Comment 20 Bohuslav "Slavek" Kabrda 2014-08-21 07:45:33 UTC
(In reply to Parag AN(पराग) from comment #19)
> (In reply to Bohuslav "Slavek" Kabrda from comment #17)
> > (In reply to Daniel Berrange from comment #16)
> > > (In reply to Bohuslav "Slavek" Kabrda from comment #15)
> > > > The specfile seems fine, but you should take care of the situation when
> > > > with_python3 is "0" - in this situation, /usr/bin/gerrymander wouldn't work.
> > > > I'd solve it like this (incomplete example):
> > > 
> > > Even simpler is to just delete the 'with_python3' conditional - all current
> > > Fedora releases have python3  so might as well just rely on it existing.
> > 
> > Yes, assuming the maintainer doesn't want to also put this package in EPEL
> > and keep the same specfile that he has in Fedora.
> 
> So do we need to create python-gerrymander or skip it? The script
> gerrymander points to python3 hashbang. If we think on dropping with_python3
> conditionals then drop python-gerrymander also and just package gerrymander
> and python3-gerrymander.

That is up to you. If you think people will not want to use the python-gerrymander library with Python 2 or you don't want to maintain it, you *don't have to* provide it.
Having said that, many people have apps written in Python 2 that are still not compatible with Python 3, so providing both subpackages might help them with migration and testing their apps on both Python major versions. But again, this is your decision as a packager. You can start having just Python 3 subpackage and add Python 2 support if someone requests it.

Comment 21 Daniel Berrangé 2014-08-21 08:31:28 UTC
(In reply to Parag AN(पराग) from comment #19)
> So do we need to create python-gerrymander or skip it? The script
> gerrymander points to python3 hashbang. If we think on dropping with_python3
> conditionals then drop python-gerrymander also and just package gerrymander
> and python3-gerrymander.

Of course we should keep python-gerrymander. Plenty of the world still wants to use Python2 and it is normal Fedora practice to provide libraries for both 2 & 3.

Comment 22 Kashyap Chamarthy 2014-08-21 08:41:02 UTC
Ok, I removed the with_python3 conditionals per Dan's suggestion, and did a scratch build again (this'll generate packages: gerrymander, python-gerrymander, python3-gerrymander

Updated

SPEC - https://kashyapc.fedorapeople.org/spec-files/gerrymander.spec
SRPM - https://kashyapc.fedorapeople.org/srpms/gerrymander-1.3-4.fc20.src.rpm


Successful Koji scratch build
-----------------------------

$ koji build --scratch rawhide  /home/kashyap/rpmbuild/SRPMS/gerrymander-1.3-4.fc20.src.rpm 
Uploading srpm: /home/kashyap/rpmbuild/SRPMS/gerrymander-1.3-4.fc20.src.rpm
[====================================] 100% 00:00:02  33.73 KiB  13.10 KiB/sec
Created task: 7430651
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=7430651
Watching tasks (this may be safely interrupted)...
7430651 build (rawhide, gerrymander-1.3-4.fc20.src.rpm): open (buildvm-15.phx2.fedoraproject.org)
  7430652 buildArch (gerrymander-1.3-4.fc20.src.rpm, noarch): open (buildvm-16.phx2.fedoraproject.org)
  7430652 buildArch (gerrymander-1.3-4.fc20.src.rpm, noarch): open (buildvm-16.phx2.fedoraproject.org) -> closed
  0 free  1 open  1 done  0 failed
7430651 build (rawhide, gerrymander-1.3-4.fc20.src.rpm): open (buildvm-15.phx2.fedoraproject.org) -> closed
  0 free  0 open  2 done  0 failed

7430651 build (rawhide, gerrymander-1.3-4.fc20.src.rpm) completed successfully

Comment 23 Kashyap Chamarthy 2014-08-21 11:23:55 UTC
New Package SCM Request
=======================
Package Name: gerrymander
Short Description: A client API and command line tool for gerrit
Upstream URL: http://github.com/berrange/gerrymander
Owners: kashyapc
Branches: f20 21
InitialCC: berrange

Comment 24 Gwyn Ciesla 2014-08-21 12:00:03 UTC
Git done (by process-git-requests).

Comment 25 Parag AN(पराग) 2014-10-13 15:14:07 UTC
is this built on koji?

Comment 26 Kashyap Chamarthy 2014-10-20 17:32:17 UTC
(In reply to Parag AN(पराग) from comment #25)
> is this built on koji?

My bad, missed that.

Just issued one for F21:

  http://koji.fedoraproject.org/koji/taskinfo?taskID=7917014

Comment 27 Kashyap Chamarthy 2014-10-20 19:03:43 UTC
Please ignore the above failedbuild. 

Here's a successful build in Rawhide: http://koji.fedoraproject.org/koji/buildinfo?buildID=586833 -- gerrymander-1.3-4.fc22

------------------------------------------------
$ fedpkg build
Building gerrymander-1.3-4.fc22 for rawhide
Created task: 7917617
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=7917617
Watching tasks (this may be safely interrupted)...
7917617 build (rawhide, /gerrymander:8e72f5b3482968b5c36473b2a9e0876e1c4f8a76): open (buildvm-08.phx2.fedoraproject.org)
  7917618 buildSRPMFromSCM (/gerrymander:8e72f5b3482968b5c36473b2a9e0876e1c4f8a76): open (arm02-builder19.arm.fedoraproject.org)
  7917618 buildSRPMFromSCM (/gerrymander:8e72f5b3482968b5c36473b2a9e0876e1c4f8a76): open (arm02-builder19.arm.fedoraproject.org) -> closed
  0 free  1 open  1 done  0 failed
  7917632 buildArch (gerrymander-1.3-4.fc22.src.rpm, noarch): open (buildhw-04.phx2.fedoraproject.org)
  7917632 buildArch (gerrymander-1.3-4.fc22.src.rpm, noarch): open (buildhw-04.phx2.fedoraproject.org) -> closed
  0 free  1 open  2 done  0 failed
  7917663 tagBuild (noarch): open (buildvm-23.phx2.fedoraproject.org)
  7917663 tagBuild (noarch): open (buildvm-23.phx2.fedoraproject.org) -> closed
  0 free  1 open  3 done  0 failed
7917617 build (rawhide, /gerrymander:8e72f5b3482968b5c36473b2a9e0876e1c4f8a76): open (buildvm-08.phx2.fedoraproject.org) -> closed
  0 free  0 open  4 done  0 failed

7917617 build (rawhide, /gerrymander:8e72f5b3482968b5c36473b2a9e0876e1c4f8a76) completed successfully
------------------------------------------------

Comment 28 Parag AN(पराग) 2014-11-19 13:29:10 UTC
Is this package built for requested branches? can we close this?

Comment 29 Kashyap Chamarthy 2014-11-20 10:08:55 UTC
Yes (they're both tagged for 'updates-candidate')

F20: http://koji.fedoraproject.org/koji/buildinfo?buildID=587126
F21: http://koji.fedoraproject.org/koji/buildinfo?buildID=586839

Comment 30 Fedora Update System 2015-01-09 14:25:11 UTC
gerrymander-1.4-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/gerrymander-1.4-1.fc21

Comment 31 Fedora Update System 2015-01-10 03:00:28 UTC
gerrymander-1.4-1.fc21 has been pushed to the Fedora 21 testing repository.

Comment 32 Fedora Update System 2015-03-22 04:42:21 UTC
gerrymander-1.4-1.fc21 has been pushed to the Fedora 21 stable repository.


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