Bug 630778

Summary: Review Request: llvm-py - Python bindings for LLVM
Product: [Fedora] Fedora Reporter: Eric Smith <spacewar>
Component: Package ReviewAssignee: Michel Lind <michel>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, michel, notting
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-10-27 02:33:49 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Eric Smith 2010-09-06 22:43:26 UTC
Spec URL: http://fedorapeople.org/~brouhaha/llvm-py/llvm-py.spec
SRPM URL: http://fedorapeople.org/~brouhaha/llvm-py/llvm-py-0.6-1.fc13.src.rpm
Koji scratch build for F13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2450031
Description:
llvm-py provides Python bindings for LLVM. It’s goal is to expose
enough of LLVM APIs to implement a compiler backend or a VM in pure
Python. llvm-py consists of Python and C modules that wrap over the
native C++/C bindings of LLVM, and does not use / have dependencies on
"glue utilities" like Boost.Python, swig etc.

Comment 1 Eric Smith 2010-09-06 22:50:32 UTC
Just noticed the erroneous use of an apostrophe in "It's" in the description, which was cut-and pasted from the llvm-py web site.  I'm assuming that I'll need to make some changes to the spec file as a result of package review comments, so I'll remove the apostrophe at that time.

Comment 2 Michel Lind 2010-09-15 12:01:47 UTC
Swap with gedit-valencia ? https://bugzilla.redhat.com/show_bug.cgi?id=518892

It should be a straightforward review, but was stalled for months waiting for a new upstream release.

- doc subpackage should probably be BuildArch: noarch. Changing it later is a bit
  painful so you might as well do it now
- would it be less complicated to just use the default byte-compiler and then exclude the documentation files? It would involve less macro manipulation, and while it would unnecessarily compile some files, a beneficial side effect is that we make sure the example files are not completely broken. But I'm not too concerned either way.

Complete review to follow.

Comment 3 Michel Lind 2010-09-15 20:52:27 UTC
Koji F-14 build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2469802

Needs fixing:
- -doc files should be in versioned directory. Instead of copying them manually
  you should probably just use relative paths when declaring them e.g.
  %doc test/*.py

- -doc files should not be executable (see full review below)

Suggestion for improvement:
- add a %check section with the following:
  cd test
  PYTHONPATH=../build/lib.linux-*/ python testall.py


* TODO Review [80%]
** DONE Names [2/2]
*** DONE Package name
*** DONE Spec name
** DONE Meets [[http://fedoraproject.org/wiki/Packaging/Guidelines][guidelines]]
** DONE source files match upstream
   $ md5sum llvm-py-0.6.tar.bz2 ~/rpmbuild/SOURCES/llvm-py-0.6.tar.bz2 
eec62e4ce6f95f6e01edcba59747933d  llvm-py-0.6.tar.bz2
eec62e4ce6f95f6e01edcba59747933d  /home/michel/rpmbuild/SOURCES/llvm-py-0.6.tar.bz2

** DONE License [3/3]
*** DONE License is Fedora-approved
*** DONE License field accurate
*** DONE License included iff packaged by upstream
** TODO rpmlint [1/2]
*** DONE on src.rpm
llvm-py.src: W: spelling-error %description -l en_US backend -> backed, backbend, back end
==> ignore this

llvm-py.src: W: invalid-url Source0: http://llvm-py.googlecode.com/files/llvm-py-0.6.tar.bz2 HTTP Error 404: Not Found
==> ignore -- spectool works fine, not sure what's wrong with rpmlint here
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

*** FAIL on x86_64.rpm
llvm-py.x86_64: W: spelling-error %description -l en_US backend -> backed, backbend, back end
==> ignore this
llvm-py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/llvm/_core.so _core.so()(64bit)
==> ignore this
llvm-py-doc.x86_64: W: spurious-executable-perm /usr/share/doc/llvm-py/uses.py
...
llvm-py-doc.x86_64: W: spurious-executable-perm /usr/share/doc/llvm-py/operands.py
==> see below
llvm-py-doc.x86_64: W: doc-file-dependency /usr/share/doc/llvm-py/uses.py /usr/bin/env
...
llvm-py-doc.x86_64: W: doc-file-dependency /usr/share/doc/llvm-py/operands.py /usr/bin/env
==> can be ignored, they are example scripts after all
3 packages and 0 specfiles checked; 0 errors, 32 warnings.

** DONE Language & locale [3/3]
*** DONE Spec in US English
*** DONE Spec legible
*** N/A Use %find_lang to handle locale files
** FAIL Build [2/3]
*** DONE Koji results
    http://koji.fedoraproject.org/koji/taskinfo?taskID=2469802
*** DONE BRs complete
*** FAIL Directory ownership
    /usr/share/doc/llvm-py not owned by any package
** TODO Spec inspection [6/8]
*** FAIL ldconfig for libraries
    - State "FAIL"       from "TODO"       [2010-09-15 Wed 21:29] \\
      ldconfig is called on %post and %postun even though there are no
      libraries

*** DONE No duplicate files
*** FAIL File permissions
    - State "FAIL"       from "TODO"       [2010-09-15 Wed 22:22] \\
      -doc: files in %{_docdir} should not have executable permission
*** DONE Filenames must be UTF-8
*** DONE Has %clean section
    (except F-13+:
    https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean)

    Note: BuildRoot declaration can be removed on F-10+ and EL-6+

*** DONE %buildroot cleaned on %install
*** DONE Macro usage consistent
*** DONE Documentation [2/2]
**** DONE If large docs, separate -doc
     note: should mark this as BuildArch: noarch
**** DONE %doc files are non-essential
** N/A Desktop file validation
** DONE [[http://fedoraproject.org/wiki/Packaging/ScriptletSnippets][Scriptlets]] [4/4]
*** N/A desktop-database (desktop entry has MimeType)
*** N/A icon cache (icons in %{_datadir}/icons/)
*** N/A info files
*** N/A mimeinfo (file in %{_datadir}/mime/packages)

Comment 4 Eric Smith 2011-01-03 06:10:34 UTC
I made the recommended changes, and updated to a Subversion snapshot from upstream to get support for LLVM 2.8.  I had a bit of trouble with getting the environment variable set appropriately in your proposed python invocation for the %check section; maybe you know of a better solution than what I used.

Spec URL: http://fedorapeople.org/~brouhaha/llvm-py/llvm-py.spec
SRPM URL: http://fedorapeople.org/~brouhaha/llvm-py/llvm-py-0.7-0.1.20101105svn.fc14.src.rpm
Koji scratch build for F14:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2697237

Thanks!
Eric

Comment 5 Eric Smith 2011-10-27 02:33:49 UTC
Closing this review request as upstream seems to be dead.