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 454921 - Review Request: python-dotconf - Parser for dot.conf file
Summary: Review Request: python-dotconf - Parser for dot.conf file
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-07-10 19:46 UTC by Hemant Goyal
Modified: 2008-08-01 09:39 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-08-01 09:39:31 UTC
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Hemant Goyal 2008-07-10 19:46:15 UTC
Spec URL: http://www.nsitonline.in/hemant/stuff/pydotconf/rpm/pydotconf.spec
SRPM URL: http://www.nsitonline.in/hemant/stuff/pydotconf/rpm/pydotconf-0.1-1.src.rpm
Description: python parser for dot.conf file

Comment 1 Kyle VanderBeek 2008-07-10 20:07:52 UTC
At a glance you're going to need to change your spec quite a bit.  Please refer
to the packaging guidelines:

https://fedoraproject.org/wiki/Packaging/Guidelines
https://fedoraproject.org/wiki/Packaging/Python

In particular, don't include a Vendor, don't define things like name and version
needlessly, Source0 should be an URL, URL should be the package's home page, and
the package needs to be named python-pydotconf.

(Not to mention that putting "py" in the name of your python library is a bit
silly.  Why not just name it dotconf?  Also, you should consider reading PEP-8
and just rolling everything into a single dotconf.py instead of a directory...
it's only about 350 lines.)

You should also install the rpmdevtools RPM and use the "rpmdev-newspec -t
python" to generate a python-esque base .spec file.

Comment 2 Hemant Goyal 2008-07-11 19:24:31 UTC
Hi Kyle,

Thanks for the review.

SPEC URL :
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf.spec

SRPM URL:
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf-0.2-2.fc7.src.rpm

> In particular, don't include a Vendor, don't define things like name and
version, needlessly, Source0 should be an URL, URL should be the package's home
page, and the package needs to be named python-pydotconf.

Yes this was my mistake, I directly worked on the SPEC generated by disutils
bdist command. I have use the rpmdev (python) SPEC template now.

> (Not to mention that putting "py" in the name of your python library is a bit
> silly.  Why not just name it dotconf?  Also, you should consider reading PEP-8
> and just rolling everything into a single dotconf.py instead of a directory...
> it's only about 350 lines.)

Hmmm, thanks for the idea. I ve merged the two files into dotconf.py now. Are
you also suggesting that I do not include the file in a directory like dotconf?

Thanks,
Hemant

Comment 5 Mamoru TASAKA 2008-07-17 14:29:52 UTC
Would you review my review request (bug 453944) instread?
(note that scratch builds posted on the bug are now out of date)

Comment 6 Mamoru TASAKA 2008-07-17 14:42:14 UTC
Currently dist-f10 python is completely broken so please wait...

Comment 7 Mamoru TASAKA 2008-07-17 16:16:01 UTC
BTW

* Your latest package does not build on dist-f9-updates-candidate:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=722566
  egg info is created on F-9/devel (I forgot for F-8)

* Currently all branches are >= F-8 and %if 0%{?fedora} >= 8 is of no sense
  (unless you are thinking of OLPC branch)

* Currently no issue (as they are same), however would you clarify 
  this package is GPLv3 or GPLv3+?

Comment 8 Hemant Goyal 2008-07-18 08:57:06 UTC
Hi Mamoru,

Thanks for the review.

Package Updated.

SPEC URL :
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf.spec
 
SRPM URL:
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf-0.2-5.fc7.src.rpm


(In reply to comment #7)
> BTW
> 
> * Your latest package does not build on dist-f9-updates-candidate:
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=722566
>   egg info is created on F-9/devel (I forgot for F-8)

It just did not strike me to scratch build on koji myself.

dist-f9
http://koji.fedoraproject.org/koji/taskinfo?taskID=723978 

dist-f10
http://koji.fedoraproject.org/koji/taskinfo?taskID=723983

dist-f8
http://koji.fedoraproject.org/koji/taskinfo?taskID=723982

dist-olpc3
http://koji.fedoraproject.org/koji/taskinfo?taskID=723985

> * Currently all branches are >= F-8 and %if 0%{?fedora} >= 8 is of no sense
>   (unless you are thinking of OLPC branch)

Yes I will be requesting an OLPC branch. However I have fixed it as follows 

%if 1%{?olpc}
BuildRequires: python-setuptools-devel
%else
BuildRequires: python-setuptools
%endif   

> * Currently no issue (as they are same), however would you clarify 
>   this package is GPLv3 or GPLv3+?

Thanks for checking this up. The license should have been GPLv3+. I have fixed
this now, however it will not be reflected in the SRPM built on koji scratch
build. I have made the requisite fix in the SPEC that that I have uploaded.

Thanks

Comment 9 Mamoru TASAKA 2008-07-18 13:10:13 UTC
Well,

* Duplicate files
-----------------------------------------------------------------------
%dir %{python_sitelib}/dotconf
%{python_sitelib}/*
-----------------------------------------------------------------------
  Now the directory %{python_sitelib}/dotconf is listed twice.

* %if 1%{?olpc}
  This is always true.

Comment 10 Kyle VanderBeek 2008-07-18 20:16:14 UTC
(In reply to comment #2)
> Hmmm, thanks for the idea. I ve merged the two files into dotconf.py now. Are
> you also suggesting that I do not include the file in a directory like dotconf?

Yes, at this point there seems little reason to have a package directory since
it's just one file.  Move all the code into a dotconf.py file and out of the
subdirectory, delete and use the "py_modules" parameter in your setup.py:

  http://docs.python.org/dist/listing-modules.html

Additionally, please start creating tags in your google code svn repository. 
We'd like to see things a little more professionally versioned.

Comment 11 Hemant Goyal 2008-07-20 12:53:01 UTC
Thanks for the review.

Package Updated.

SPEC URL :
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf.spec
 
SRPM URL:
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf-0.2-6.fc7.src.rpm

@Kyle : Great I did not know about the concept of tagging until now :). I've
created the tags in the google SVN now. I've also moved dotconf.py out of the
directory and used py_modules parameter.

Thanks.

Comment 12 Mamoru TASAKA 2008-07-20 16:12:27 UTC
Did you change the tarball in the srpm without changing version?
(As you seem to be the upstream) please change the version and release the new one
formally if you want to change the tarball itself.

Comment 13 Hemant Goyal 2008-07-23 11:08:55 UTC
Hi,
I have changed the version of the tarball.

SPEC URL :
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf.spec
 
SRPM URL:
http://www.nsitonline.in/hemant/stuff/python-dotconf/rpm/python-dotconf-0.2.1-7.fc7.src.rpm

Thanks,
Hemant


Comment 14 Mamoru TASAKA 2008-07-24 05:11:32 UTC
----------------------------------------------------------------------
    This package (python-dotconf) is APPROVED by me
----------------------------------------------------------------------

Comment 15 Hemant Goyal 2008-07-24 07:04:43 UTC
Thanks Mamoru :)

New Package CVS Request
=======================
Package Name: python-dotconf
Short Description: python parser for dot.conf file
Owners: hemantg
Branches: OLPC-2 OLPC-3 F-8 F-9
InitialCC:
Cvsextras Commits: yes


Comment 16 Kevin Fenzi 2008-07-24 18:12:16 UTC
cvs done.

Comment 17 Hemant Goyal 2008-08-01 09:39:31 UTC
Thank you for the CVS creation and the review.




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