Bug 725552 - Review Request: python-confparser - A KISS parse to *nix config files
Summary: Review Request: python-confparser - A KISS parse to *nix config files
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rafael Aquini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-07-25 20:18 UTC by Douglas Schilling Landgraf
Modified: 2011-08-23 19:35 UTC (History)
4 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2011-07-30 03:01:30 UTC
aquini: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Douglas Schilling Landgraf 2011-07-25 20:18:38 UTC
Spec URL: https://raw.github.com/dougsland/confparse/master/confparse.spec
SRPM URL: https://github.com/dougsland/confparse/raw/master/confparse-1.0.0-1.fc13.src.rpm
Description: confparse - A KISS parse to *nix config files

Comment 1 Rafael Aquini 2011-07-25 20:22:11 UTC
Howdy,

I'll proceed this review. I hope to finish it soon.

Cheers!
--aquini

Comment 2 Susi Lehtola 2011-07-26 07:26:38 UTC
Rafael: please set the status to ASSIGNED when you take on the bug.


A few notes:

- Don't repeat the name in the summary. It should be simply
 "A KISS parser to *nix config files"
              ^ note added r

- BuildArchitectures is practically always written short as BuildArch.

- License tag is incorrect, it should be LGPLv2+.

- You are missing BR: python-devel, which is required for python packages.

- The statement
 %if 0%{?fedora}
 BuildRequires: python-setuptools-devel
 %else
 BuildRequires: python-setuptools
 %endif      
is silly, since it is missing version conditions. The first clause will always evaluate to true, unless you are going to target EPEL. However, in this case as well you can just BR: python-setuptools-devel and it will work at least on EPEL-5 and EPEL-6, where python-setuptools provides python-setuptools-devel.

- You are mixing python and %{_python}, please choose a style and stick with it.

- Missing COPYING in %doc.

- Please don't use wildcards where they are not absolutely necessary, since their freeminded use makes it hard to see what actually gets owned by the package and can bite you if you're not careful (in this case, you might not see when egg files are not generated). Please change
 %{python_sitelib}/*
to
 %{python_sitelib}/confparse.py*
 %{python_sitelib}/confparse-%{version}.py*.egg-info

Comment 3 Douglas Schilling Landgraf 2011-07-26 19:06:14 UTC
Hello,

Jussi, thanks for all your comments.

Rafael/Jussi:

New spec available:
https://github.com/dougsland/confparse/raw/master/confparse.spec

New srpm available:
https://github.com/dougsland/confparse/raw/master/confparse-1.0.0-2.fc13.src.rpm

Thanks
Douglas

Comment 4 Susi Lehtola 2011-07-26 19:16:13 UTC
Looks better. I would've used just "python", since IMHO %{__python} is overkill. Although it's allowed by rpm, I don't think there's much sense in using macros such as %{__cp}, %{__mv}, %{__rm}, %{__mkdir} and so on.

You seem to have a tab stop problem with BuildArch now, otherwise things are probably OK.

Now you'll just have to wait for Rafael to do his review.

Comment 5 Rafael Aquini 2011-07-26 22:27:18 UTC
Howdy folks!

Jussi:
Thanks for doing this pre-review. It has made my work a lot easier.

Douglas:

Please, consider the following formal review:


Good:
* rpmlint runs on the source rpm and all binary rpms the build produces. Warnings, and error reported are common false positives, and should be ignored:
** [raquini@optiplex ~]$ rpmlint rpmbuild/SPECS/confparse.spec /home/aquini/rpmbuild/SRPMS/confparse-1.0.0-2.fc14.src.rpm /home/aquini/rpmbuild/RPMS/noarch/confparse-1.0.0-2.fc14.noarch.rpm
confparse.src: W: spelling-error Summary(en_US) config -> con fig, con-fig, configure
confparse.src: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
confparse.src: W: file-size-mismatch confparse-1.0.0.tar.gz = 14371, https://github.com/dougsland/confparse/raw/master/confparse-1.0.0.tar.gz = 1
confparse.noarch: W: spelling-error Summary(en_US) config -> con fig, con-fig, configure
confparse.noarch: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
confparse.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/confparse.py 0644L /usr/bin/python
2 packages and 1 specfiles checked; 1 errors, 5 warnings.


* License in spec and sources is LGPLv2+ which is open source
* Spec is legible and American English
* Source matches upstream
** wget https://github.com/dougsland/confparse/raw/master/confparse-1.0.0.tar.gz
**[raquini@optiplex ~]$ md5sum confparse-1.0.0.tar.gz rpmbuild/SOURCES/confparse-1.0.0/confparse-1.0.0.tar.gz 
08601828f09b97ec4629539dbb893450  confparse-1.0.0.tar.gz
08601828f09b97ec4629539dbb893450  rpmbuild/SOURCES/confparse-1.0.0/confparse-1.0.0.tar.gz


* No locale files
* No shared libraries
* No bundled libraries
* Not relocatable
* No directories created unowned
* No duplicate files
* Default permissions are set
* Package is code
* No large documentation
* No %doc files are used at runtime
* No header files
* Not a GUI application
* Does not own files or directories from other packages
* All filenames are utf8
* Builds in mock
* Builds in koji:
[raquini@optiplex ~]$ koji build --scratch dist-rawhide /var/lib/mock/fedora-14-x86_64/result/confparse-1.0.0-2.fc14.src.rpm
Uploading srpm: /var/lib/mock/fedora-14-x86_64/result/confparse-1.0.0-2.fc14.src.rpm
[====================================] 100% 00:00:02  17.32 KiB   7.11 KiB/sec
Created task: 3232534
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=3232534
Watching tasks (this may be safely interrupted)...
3232534 build (dist-rawhide, confparse-1.0.0-2.fc14.src.rpm): free
3232534 build (dist-rawhide, confparse-1.0.0-2.fc14.src.rpm): free -> open (x86-02.phx2.fedoraproject.org)
  3232535 waitrepo (191): open (x86-04.phx2.fedoraproject.org)
  3232542 buildArch (confparse-1.0.0-2.fc14.src.rpm, noarch): open (x86-03.phx2.fedoraproject.org)
  3232535 waitrepo (191): open (x86-04.phx2.fedoraproject.org) -> closed
  0 free  2 open  1 done  0 failed
  3232542 buildArch (confparse-1.0.0-2.fc14.src.rpm, noarch): open (x86-03.phx2.fedoraproject.org) -> closed
  0 free  1 open  2 done  0 failed
3232534 build (dist-rawhide, confparse-1.0.0-2.fc14.src.rpm): open (x86-02.phx2.fedoraproject.org) -> closed
  0 free  0 open  3 done  0 failed

3232534 build (dist-rawhide, confparse-1.0.0-2.fc14.src.rpm) completed successfully



NEEDSWORK:
[1] As your package is enhancing, or adding new functionality to, an existing Fedora package (python) without being useful on its own, it is considered an "addon" package. So, its name should reflect this fact. 
Said that, your package should be renamed to python-confparse, or pyconfparse, according to the Package Naming Guidelines:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Addon_Packages_.28python_modules.29

[2] Fix that last Jussi's observation on BuildArch: header alignment.



As soon as you have this work done I'll grant your package approval.

Cheers!
--Aquini

Comment 6 Douglas Schilling Landgraf 2011-07-27 06:07:15 UTC
Hello Rafael and Jussi,

> NEEDSWORK:
> [1] As your package is enhancing, or adding new functionality to, an existing
> Fedora package (python) without being useful on its own, it is considered an
> "addon" package. So, its name should reflect this fact. 
> Said that, your package should be renamed to python-confparse, or pyconfparse,
> according to the Package Naming Guidelines:
> http://fedoraproject.org/wiki/Packaging> 
> /NamingGuidelines#Addon_Packages_.28python_modules.29

Agreed, renamed to python-confparser
https://github.com/dougsland/python-confparser/wiki

> [2] Fix that last Jussi's observation on BuildArch: header alignment.

Agreed, fixed.

New spec available:
https://github.com/dougsland/python-confparser/raw/master/python-confparser.spec

New srpm available:
https://github.com/dougsland/python-confparser/raw/master/python-confparser-1.0.0-3.fc13.src.rpm

Thanks
Douglas

Comment 7 Rafael Aquini 2011-07-27 17:28:57 UTC
Howdy Douglas,

I am totaly fine with this package of yours. Feel free to request a new SCM branch.

APPROVED

Comment 8 Douglas Schilling Landgraf 2011-07-27 17:57:21 UTC
New Package SCM Request
=======================
Package Name: python-confparser
Short Description: A KISS python module to parse *nix config files
Owners: dougsland
Branches: f14 f15 epel-5 epel-6

Comment 9 Gwyn Ciesla 2011-07-27 18:45:57 UTC
1. Is the package name python-confparser or confparse?  Make the summary and
SCM request match.
2. Branches should be EL-5, EL-6, not epel-5 and epel-6.
3. You should also request f16, as that's now been branched.

Once fixed, re-set the flag.  Thanks!

Comment 10 Douglas Schilling Landgraf 2011-07-27 18:56:32 UTC
Hello Jon,

I have fixed the request and summary, thanks.

New Package SCM Request
=======================
Package Name: python-confparser
Short Description: A KISS python module to parse *nix config files
Owners: dougsland
Branches: F14 F15 F16 EL-5 EL-6

Comment 11 Gwyn Ciesla 2011-07-27 19:27:59 UTC
Git done (by process-git-requests).

Comment 12 Fedora Update System 2011-07-29 16:13:59 UTC
python-confparser-1.0.0-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-confparser-1.0.0-3.el6

Comment 13 Fedora Update System 2011-07-29 16:23:08 UTC
python-confparser-1.0.0-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/python-confparser-1.0.0-3.fc14

Comment 14 Fedora Update System 2011-07-29 16:23:50 UTC
python-confparser-1.0.0-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/python-confparser-1.0.0-3.fc16

Comment 15 Susi Lehtola 2011-07-30 10:19:52 UTC
What about Fedora 15?

Comment 16 Susi Lehtola 2011-07-30 10:20:15 UTC
and EL-5?

Comment 17 Douglas Schilling Landgraf 2011-07-30 19:17:50 UTC
Hello Jussi,

I pushed as well, just forgot to add the BZ# into the bodhi command line to auto-update this BZ.

All packages available here:
https://admin.fedoraproject.org/updates/python-confparser?_csrf_token=303833a082e25231dfc36031b16e66b05be60083

Thanks for all your help! Now I am just waiting the package join the official repo.

Cheers
Douglas

Comment 18 Fedora Update System 2011-08-04 18:56:26 UTC
python-confparser-1.0.1-5.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/python-confparser-1.0.1-5.fc16

Comment 19 Fedora Update System 2011-08-04 19:09:33 UTC
python-confparser-1.0.1-5.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/python-confparser-1.0.1-5.fc14

Comment 20 Fedora Update System 2011-08-04 19:17:21 UTC
python-confparser-1.0.1-5.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-confparser-1.0.1-5.el6

Comment 21 Fedora Update System 2011-08-12 13:13:53 UTC
python-confparser-1.0.1-5.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/python-confparser-1.0.1-5.el5

Comment 22 Fedora Update System 2011-08-13 02:29:31 UTC
python-confparser-1.0.1-5.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2011-08-22 15:12:20 UTC
python-confparser-1.0.1-5.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2011-08-23 19:33:37 UTC
python-confparser-1.0.1-5.el5 has been pushed to the Fedora EPEL 5 stable repository.

Comment 25 Fedora Update System 2011-08-23 19:35:48 UTC
python-confparser-1.0.1-5.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.


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