Bug 833226

rpmbuild -ba runs fine

rpmlint  python-pycparser-2.07-1.fc17.src.rpm
python-pycparser.src: W: strange-permission remove-relative-sys-path 0755L
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

reproduces your result.

rpmlint python-pycparser-2.07-1.fc17.noarch.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

seems fine.

Two different styles of macros are mixed in this spec file, i.e.: %{__python} versus $RPM_BUILD_ROOT

please choose a consistent style, i.e. use: %{buildroot}
(see http://fedoraproject.org/wiki/Packaging:Guidelines,
 1.35.1 Using %{buildroot} and %{optflags} vs $RPM_BUILD_ROOT and $RPM_OPT_FLAGS)

I noticed the spec file runs dos2unix to convert line-endings in examples files, and it also runs a custom python script to delete unwanted boilerplate sys.path lines in the examples dir.

Looking at https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Scripting_inside_of_spec_files this seems fine to me.

Since all python files in this package are in the module directory pycparser I think it would be better to be more explicit and to have %{python_sitelib}/pycparser/ in stead of %{python_sitelib}/* in the %files section.

(In reply to comment #1)
Spec URL: http://scottt.tw/fedora/python-pycparser/python-pycparser.spec
SRPM URL: http://scottt.tw/fedora/python-pycparser/python-pycparser-2.08-1.fc17.src.rpm

Changes:
1. Use %{buildroot} instead of $RPM_BUILD_ROOT
-%{__python} setup.py install --skip-build --root $RPM_BUILD_ROOT
+%{__python} setup.py install --skip-build --root %{buildroot}
 
2. List files with more specificity:
 %files
 %doc examples
-# For noarch packages: sitelib
-%{python_sitelib}/*
+%{python_sitelib}/pycparser/
+%{python_sitelib}/pycparser-*.egg-info

3. Remove dos2unix since upstream accepted my line ending patches
-BuildRequires:  dos2unix

 Requires:  python-ply

@@ -26,7 +25,6 @@ need to parse C source code.
 %setup -q -n pycparser-%{version}

 # examples
-find examples -type f -exec dos2unix '{}' ';'

thank you for this updated version.

rpmbuild runs fine with the new spec file, and rpmlint has the same result as above.

A small comment on the fact that you rename the upstream name pycparser to python-pycparser. According to the guidelines this is not needed for python packages that start their name with 'py'. See:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28python_modules.29
This therefore is upto you, both ways are allowed, but removing the rename makes the spec file simpler, which always is a good thing.

Also please I would prefer if you would not delete changelog entries, even if the spec file is not yet approved.

Finally the LICENSE file should be added to the %doc tag in the %files section.

(In reply to comment #3)
> rpmbuild runs fine with the new spec file, and rpmlint has the same result
> as above.

@Jos, for checking packages please use either a local Mock installation or a Koji scratch build. For the latter, the following command works, assuming you are in the folder where the *src.rpm resides:

koji build --scratch rawhide *src.rpm

To get a list of possible build targets, use:

koji list-targets


If you build the packages on your workstation only, you possibly miss build dependencies, if some needed packages are present on your system, but not included in the spec file.

Thanks for reminding me Mario.
I tested again using mock, using release fedora-rawhide-x86_64. This runs fine and creates both a srpm and a rpm.

In addition I tested using koji. Also here both rpms where created succesfully. See: http://koji.fedoraproject.org/koji/taskinfo?taskID=4608971

(In reply to comment #5)
> Thanks for reminding me Mario.
> I tested again using mock, using release fedora-rawhide-x86_64. This runs
> fine and creates both a srpm and a rpm.
> 
> In addition I tested using koji. Also here both rpms where created
> succesfully. See: http://koji.fedoraproject.org/koji/taskinfo?taskID=4608971

Then you could go ahead with assigning this bug to you and completing the review.

Mario, thanks for your advice. 
I have assigned it, and will try to complete the review after the last small issue in the spec file has been resolved.

Hi Scott Tsai,

could you please fix the last tiny issue in the spec file (adding the LICENSE file to the %doc section, see Comment 3). The other issues I mentioned are optional, and it's up to you to decide what to do with them.

After that, I have no more comments and will approve this package.

Thanks.

(In reply to comment #8)
Hi Jos, sorry for the late reply.
I've added the LICENSE file to docs in:
http://scottt.tw/fedora/python-pycparser/python-pycparser.spec
http://scottt.tw/fedora/python-pycparser/python-pycparser-2.08-1.fc18.src.rpm

diff --git a/python-pycparser.spec b/python-pycparser.spec
index 63a257d..d749e6d 100644
--- a/python-pycparser.spec
+++ b/python-pycparser.spec
@@ -12,6 +12,7 @@ BuildArch:      noarch

 # for unit tests
 BuildRequires:  python-ply
+BuildRequires:  dos2unix

 Requires:  python-ply

@@ -27,6 +28,7 @@ need to parse C source code.
 # examples
 cp %SOURCE1 .
 ./remove-relative-sys-path examples
+dos2unix LICENSE

 %build
 %{__python} setup.py build
@@ -38,11 +40,10 @@ cp %SOURCE1 .
 %{__python} tests/all_tests.py

 %files
-%doc examples
+%doc examples LICENSE
 %{python_sitelib}/pycparser/
 %{python_sitelib}/pycparser-*.egg-info

-
 %changelog
-* Tue Jun 18 2012 <scottt.tw> Scott Tsai 2.08-1
+* Tue Jun 18 2012 Scott Tsai <scottt.tw> 2.08-1
 - upstream 2.08

Hi Scott,

thanks for the updated version.
I looked again at the package and (since I'm still learning about the review process) found a few other issues.

the package still builds fine with mock:

 mock -r fedora-rawhide-x86_64 --rebuild python-pycparser-2.08-1.fc17.src.rpm

generates these rpms:
   python-pycparser-2.08-1.fc19.noarch.rpm
   python-pycparser-2.08-1.fc19.src.rpm

However rpmlint has some new warnings now:
$ rpmlint python-pycparser-2.08-1.fc19.noarch.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint python-pycparser-2.08-1.fc19.src.rpm 
python-pycparser.src: W: strange-permission remove-relative-sys-path 0755L
python-pycparser.src: W: invalid-url Source0: http://pycparser.googlecode.com/files/pycparser-2.08.tar.gz HTTP Error 404: Not Found
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

This introduces a problem with this requirement from the review guidelines:

MUST: The sources used to build the package must match the upstream source, 

this is a problem now since upstream moved to a different location.
The new source URL of the project is: https://bitbucket.org/eliben/pycparser
However, I don't see version 2.08 listed here anymore. Latest versions are 2.09 and 2.09.1, but the one before is 2.07

Could you please either update to the latest released version, or change the source url to point to the right mercurial revision that matches your v2.08?
(if you choose this you probably should contact upstream to ask which version this was, since it is not noted in mercurial comments or tags)

The other rpmlint warning refers to this requirement:

MUST: Permissions on files must be set properly. 

rpmlint complains about the permissions for the remove-relative-sys-path script. There is no need to have a write permission
for this script, but then it doesn't hurt either I think since the script is not
installed, but only used during the build of the rpm.

Then there are additional requirements from: https://fedoraproject.org/wiki/Packaging:Python

To build a package containing python2 files, you need to have
   BuildRequires: python2-devel

this is currently missing in your spec file.

Best regards,

Jos

(In reply to comment #10)

I've update
http://scottt.tw/fedora/python-pycparser/python-pycparser.spec
http://scottt.tw/fedora/python-pycparser/python-pycparser-2.08-1.fc18.src.rpm
with these changes:

1. Change the source tarball to upstream tag "release_v2.09.1"
Upstream is currently using a "tag releases in mercurial" but offer no files in the download section" strategy:

+# NOTE: "hgrev" and "version" should match, e.g.
+# revision 82ace14bb612 is tagged as "release_v2.09.1" in
+# https://bitbucket.org/eliben/pycparser
+
+%global hgrev 82ace14bb612
+
 Name:           python-pycparser
-Version:        2.08
+Version:        2.09.1
 Release:        1%{?dist}
 Summary:        C parser and AST generator written in Python

 License:        BSD
-URL:            http://code.google.com/p/pycparser/
-Source0:        http://pycparser.googlecode.com/files/pycparser-%{version}.tar.gz
+URL:            https://bitbucket.org/eliben/pycparser
+# tarball generated by bitbucket from mercurial tag:
+# https://bitbucket.org/eliben/pycparser/commits/%{hgrev}
+Source0:        eliben-pycparser-%{hgrev}.tar.bz2


2. Add BuildRequires on python2-devel:

+BuildRequires:  python2-devel


Regarding the permission of "remove-relative-sys-path", I still think the rpmlint warning should just be ignored.

(In reply to comment #11)

The SRPM URL should instead be
http://scottt.tw/fedora/python-pycparser/python-pycparser-2.09.1-1.fc18.src.rpm

Thanks for your new version.

Some remarks on your spec file:

There is a tiny difference between the spec file you published and the spec file in your srpm:

1c1
< # NOTE: "hgrev" and Version should match, e.g.
---
> # NOTE: "hgrev" and "version" should match, e.g.

all patches should have a comment concerning the upstream status,
but this is missing in your spec file.

See:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

Also I noticed that poth upstream pycparser and Ply state that they are
compatible to python3. It would be nice if you could consider adding
support to python3 in a next version as well.

Testing was succesfull using mock, which created the srpm and the noarch rpm.
rpmlint gives the following output:

$ rpmlint  python-pycparser-2.09.1-1.fc19.noarch.rpm  
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint  python-pycparser-2.09.1-1.fc19.src.rpm 
python-pycparser.src: W: strange-permission remove-relative-sys-path 0755L
python-pycparser.src:15: W: macro-in-comment %{hgrev}
python-pycparser.src: W: invalid-url Source0: eliben-pycparser-82ace14bb612.tar.bz2
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

I think the permission warning can indeed be ignored
The macro in comment could be fixed by writing %%{hgrev}
The invalid-url can maybe be fixed by referring to this url:
https://bitbucket.org/eliben/pycparser/get/release_v2.09.1.tar.bz2

This command did retrieve the package correctly on my side:
wget https://bitbucket.org/eliben/pycparser/get/release_v2.09.1.tar.bz2 \
     -O pycparser-2.09.1.tar.bz2

Also koji tested the package succesfully, see:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4894858

---
MUST items as mentioned in:
  https://fedoraproject.org/wiki/Packaging/ReviewGuidelines

key:
[+] OK
[.] OK, not applicable
[X] needs work

[+] MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.[1]
[+] MUST: The package must be named according to the Package Naming Guidelines .
[+] MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] .
PROBLEM SEE BELOW: [X] MUST: The package must meet the Packaging Guidelines.
ADDED: [+] in addition check the python specific Guidelines
[+] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
[+] MUST: The License field in the package spec file must match the actual license. [3]
[+] MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.[4]
[+] MUST: The spec file must be written in American English. [5]
[+] MUST: The spec file for the package MUST be legible. [6]
[X] MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use sha256sum for this task as it is used by the sources file once imported into git. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7]
[.] MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. [8]
[+] MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
[.] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9]
[.] MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [10]
[+] MUST: Packages must NOT bundle copies of system libraries.[11]
[.] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. [12]
[+] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [13]
[+] MUST: A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations)[14]
[+] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. [15]
[+] MUST: Each package must consistently use macros. [16]
[+] MUST: The package must contain code, or permissable content. [17]
[.] MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [18]
[+] MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [18]
[.] MUST: Static libraries must be in a -static package. [19]
[.] MUST: Development files must be in a -devel package. [20]
[.] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release} [21]
[.] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[19]
[.] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [22]
[.] MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [23]
[+] MUST: All filenames in rpm packages must be valid UTF-8. [24]

[.] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [25]
[.] SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. [26]
[+] SHOULD: The reviewer should test that the package builds in mock. [27]
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures. [28]
[+] SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
[.] SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. [29]
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [21]
[.] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. [30]
[+] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [31]
[.] SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.[32]


the Packaging Guidelines state:

Fedora packages should make every effort to avoid having multiple, separate, upstream projects bundled together in a single package. 

this conflicts with the fact that pycparser provides a full copy of the external Ply code.
pycparser should probably be patched to use the existing fedora package python-ply and not use the packaged version.

Hi Scott,

did you already find time to look into the ply issue I mentioned above?
Don't hesitate to ask for help, should that be needed.

Regards,

Jos

(In reply to comment #14)
HI Jos,
I looked at the embedded PLY in the upstream repo history up to the point of determining that it wasn't patched. I tried to come up with a way to not keep a local path in Fedora for using system PLY a.l.a. how projects like Firefox has
"--with-system-libXXX" options in their configure scripts but failed.

Please feel free to take over packing python-pycparser.

Hi Scott,

you are right, the setup script does not allow switching off the embedded copy of ply. However, a simple patch should be enough to fix this issue.
See these example files that I prepared to test the idea:

http://www.jdekloe.nl/Fedora/python-pycparser.spec
http://www.jdekloe.nl/Fedora/python-pycparser-2.09.1-2.fc18.src.rpm

feel free to take them and modify them for your next version.

Personally I have no direct use for pycparser, so I don't plan to takeover the packaging. Thanks for the offer though.

Scott, I need python-pycparser in order to package python-cffi. If you're no longer interested in packaging python-pycparser, I'm willing to take it.

In any case, thanks for the work you've done on it so far, and thanks to Jos for working on the review.  The patched spec Jos provided in comment 16 is working fine for me on f19.

In the build process, _build_tables.py should be invoked to generate the lextab.py and yacctab.py tables, so that they can be included in the RPM. Otherwise they have to be regenerated at runtime, which is a fairly large performance penalty, and also causes pytest regression tests of python-cffi to fail.  I've added that minor change to the spec:

http://fedorapeople.org/~brouhaha/python-pycparser/python-pycparser.spec
http://fedorapeople.org/~brouhaha/python-pycparser/python-pycparser-2.09.1-3.fc19.src.rpm

Upstream provides a release tarball for 2.09.1, so I think the Source0 URL could be changed to refer directly to that, rather than a snapshot.

(In reply to Eric Smith from comment #18)

Eric, please feel free to take over pycparser. I originally started this with python-cffi in mind as well :)

On suggestion:
The upstream author, Eli Bendersky, has since switched his projects from bitbucket to github. So you would want to change the comments to refer to https://github.com/eliben/pycparser

http://fedorapeople.org/~brouhaha/python-pycparser/python-pycparser.spec
http://fedorapeople.org/~brouhaha/python-pycparser/python-pycparser-2.09.1-4.fc19.src.rpm

Thanks Scott!

Updated for upstream move to github, renamed patches and Source1 for easier tracking of future upstream releases, and fixed rpmlint complaint about strange permissions.

Jos, if you're still willing to review this, please use the links above.

Checking out some git/hg repo's file is a pain.

So you should use pypi as Source0.

https://pypi.python.org/packages/source/p/pycparser/pycparser-2.09.1.tar.gz

Easy, Simple.

Eric, besides the Source0 ISSUE, please also add python3 support. We can know that this module supports python3.

Hi Eric,

yes I still am interested in reviewing this request.
Just tried again, but the rpm link
http://fedorapeople.org/~brouhaha/python-pycparser/python-pycparser-2.09.1-4.fc19.src.rpm
seems currently not available. Could you provide it again?

Hi Jos!  Sorry about that, it looks like I did something wrong when I scp'd it before. Perhaps put it in the wrong directory. Anyhow, I scp'd it again and verified that it's there now.

Christopher, I agree with you, and had already updated the Source0 tag in my -4 spec to point to a tarball from github.  I did not change it to pypi for the same reasons I've given in the package review for python-tinycss (bug #986630), though if Jos feels strongly that he would prefer the use of a pypi URL for Source0, I'll change it.

I'd like to add python3 support, but pycparser requires ply, and the Fedora python-ply package doesn't include python3 support.  Once the python-ply package maintainer adds that, I'll add it to python-pycparser as well.

Thanks!

Thanks Eric

here is my review:

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Header files in -devel subpackage, if present.
  Note: python-pycparser : /usr/share/doc/python-
  pycparser-2.09.1/examples/c_files/memmgr.h
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#DevelPackages

==>this clearly is an input file to test the c parser, so this issue
    may be safely ignored.

===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[-]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated", "*No copyright* Public domain". 98 files have
     unknown license. Detailed output of licensecheck in
     /home/user_to_make_rpms/reviews/833226.python-pycparser/tmp/review-
     python-pycparser/licensecheck.txt
==>manually checked to be BSD

[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 40960 bytes in 12 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[-]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: No rpmlint messages.

Python:
[x]: Python eggs must not download any dependencies during the build process.
[-]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[-]: Package contains BR: python2-devel or python3-devel
[-]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: Patches link to upstream bugs/comments/lists or are otherwise justified.
==>patches should be submitted to upstream, and a comment should
   be added to the spec file. (The one comment present in the previous
   version was deleted).

[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: python-pycparser-2.09.1-4.fc20.noarch.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint python-pycparser
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'



Requires
--------
python-pycparser (rpmlib, GLIBC filtered):
    python(abi)
    python-ply



Provides
--------
python-pycparser:
    python-pycparser



Source checksums
----------------
http://github.com/eliben/pycparser/archive/release_v2.09.1.tar.gz :
  CHECKSUM(SHA256) this package     : 3c0e9f6215c759d23625ce1d51dc966341d7a9040ebedaa1b367737d8966920c
  CHECKSUM(SHA256) upstream package : 3c0e9f6215c759d23625ce1d51dc966341d7a9040ebedaa1b367737d8966920c


Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-rawhide-x86_64
Command line :/bin/fedora-review -m fedora-rawhide-x86_64 -b 833226

--------------------

Additional notes:

I am not feeling strongly about pypi versus github, so your explanation given in the tinycss review-request is sufficient for me.

Adding python3 would be a nice bonus (the package is advertised in the setup.py file to be python3 compatible), but is no reason not to approve the package.

The package builds fine in koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5638455

Since the only missing item is a should item (but please look into that for your next update), this package is:

APPROVED

New Package SCM Request
=======================
Package Name: python-pycparser
Short Description: C parser and AST generator written in Python
Owners: brouhaha
Branches: f18 f19 el6
InitialCC:

http://fedorapeople.org/~brouhaha/python-pycparser/python-pycparser.spec
http://fedorapeople.org/~brouhaha/python-pycparser/python-pycparser-2.09.1-5.fc19.src.rpm

As I was taking care of the "should" item regarding the patches, I discovered that the package wouldn't build if an earlier version was installed, due to the current and parent dirs being added to the end of the path rather than the beginning in _build_tables.py. The fix is to do the same thing as the unit test path patch, so I added another patch for that, added comments on the patches, and submitted two of the patches as upstream issues.

Eric, thanks for your effort. I am happy with those 2 upstream reports and agree that removing ply is a fedora specific packaging thing, and upstream may have good reason to decide otherwise.

Git done (by process-git-requests).

python-pycparser-2.09.1-5.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-pycparser-2.09.1-5.fc19

python-pycparser-2.09.1-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/python-pycparser-2.09.1-5.fc18

python-pycparser-2.09.1-5.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-pycparser-2.09.1-5.el6

python-pycparser-2.09.1-5.el6 has been pushed to the Fedora EPEL 6 testing repository.

python-pycparser-2.09.1-6.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/python-pycparser-2.09.1-6.fc19

python-pycparser-2.09.1-6.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/python-pycparser-2.09.1-6.fc18

I added the requested Python 3 support in the -6 release.

python-pycparser-2.09.1-6.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.

python-pycparser-2.09.1-6.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.

python-pycparser-2.09.1-5.el6 has been pushed to the Fedora EPEL 6 stable repository.

Package Change Request
======================
Package Name: python-pycparser
New Branches: epel7
Owners: brouhaha

Git done (by process-git-requests).