Bug 234122 - Review Request: pygtkglext - Python bindings for GtkGLExt
Review Request: pygtkglext - Python bindings for GtkGLExt
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-27 04:09 EDT by Hans de Goede
Modified: 2007-11-30 17:12 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-04-02 12:25:15 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2007-03-27 04:09:03 EDT
Spec URL: http://people.atrpms.net/~hdegoede/pygtkglext.spec
SRPM URL: http://people.atrpms.net/~hdegoede/pygtkglext-1.1.0-1.fc7.src.rpm
Description:
Python bindings for GtkGLExt
Comment 1 Mamoru TASAKA 2007-03-29 04:32:20 EDT
I will review this after I return to home.
Comment 2 Mamoru TASAKA 2007-03-29 14:57:40 EDT
Well, for 1.1.0-1

* %makeinstall
-------------------------------------
# Note: make install PREFIX=$RPM_BUILD_ROOT doesnot work :(
-------------------------------------
  - Perhaps you want to use the following?
-------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -p"
-------------------------------------
    The above works for me. And INSTALL option should be
    added to keep timestamps on python scripts installed.

* Documentation
-------------------------------------
sed -i 's/\r//g' README.win32
-------------------------------------
  - Simply remove this line. README._win32_ should
    not be for linux.

* %files
  - By the way, why does file entry has the same line?
    (perhaps you just did copy & paste)

* libtool archives
  - Perhaps .la files should be removed.

* Executable permission vs shebang
-------------------------------------
chmod +x $RPM_BUILD_ROOT%{python_sitearch}/gtk-2.0/gtk/gtkgl/apputils.py
-------------------------------------
  - Again please check if this is correct

* Requires
-------------------------------------
Requires:       gtkglext
-------------------------------------
  - Is this needed? Libraries' dependency should automatically
    pull this.

* python_sitelib vs python_sitearch
-------------------------------------
if [ %{python_sitelib} != %{python_sitearch} ]; then
<skip>
fi
-------------------------------------
  - While this should work, IMO it is better to configure
    configure says:
-------------------------------------
 19151  if test "${am_cv_python_pythondir+set}" = set; then
 19152    echo $ECHO_N "(cached) $ECHO_C" >&6
 19153  else
 19154    am_cv_python_pythondir=`$PYTHON -c "from distutils import sysconfig;
print sysconfig.get_python_lib(0,0,prefix='$PYTHON_PREFIX')" 2>/dev/null ||
 19155       echo "$PYTHON_PREFIX/lib/python$PYTHON_VERSION/site-packages"`
 19156  fi
-------------------------------------
    From my impression, this should be okay with changing to
    get_python_lib(1,0,prefix='$PYTHON_PREFIX')

* License
  - Well, the included files are GPL and LGPL, and actually this
    is licensed under LGPL (also declared on
    http://www.k-3d.org/gtkglext/Main_Page#Licensing )

* Encoding
  - The following documents are not encoded with UTF-8. Please
    fix.
--------------------------------------
<file>          <encoded by>
AUTHORS         EUC-JP
README          EUC-JP
(README.win32   SJIS)
--------------------------------------
    (Just a note: usually Japanese documents are not encoded
     with UTF-8 and normally encoded with either EUC-JP, ISO-2022-JP
     <both are for UNIX file> or SJIS <for Windows file>)
Comment 3 Hans de Goede 2007-03-29 16:59:25 EDT
(In reply to comment #2)
> * %files
>   - By the way, why does file entry has the same line?
>     (perhaps you just did copy & paste)
> 
Those lines are not identical (gdkgl versus gtkgl)

> * Executable permission vs shebang
> -------------------------------------
> chmod +x $RPM_BUILD_ROOT%{python_sitearch}/gtk-2.0/gtk/gtkgl/apputils.py
> -------------------------------------
>   - Again please check if this is correct
> 

Actually this file contains a main with test routines and thus can be executed
stand alone, I've added a comment about this.

> * python_sitelib vs python_sitearch
> -------------------------------------
> if [ %{python_sitelib} != %{python_sitearch} ]; then
> <skip>
> fi
> -------------------------------------
>   - While this should work, IMO it is better to configure
>     configure says:
> -------------------------------------
>  19151  if test "${am_cv_python_pythondir+set}" = set; then
>  19152    echo $ECHO_N "(cached) $ECHO_C" >&6
>  19153  else
>  19154    am_cv_python_pythondir=`$PYTHON -c "from distutils import sysconfig;
> print sysconfig.get_python_lib(0,0,prefix='$PYTHON_PREFIX')" 2>/dev/null ||
>  19155       echo "$PYTHON_PREFIX/lib/python$PYTHON_VERSION/site-packages"`
>  19156  fi
> -------------------------------------
>     From my impression, this should be okay with changing to
>     get_python_lib(1,0,prefix='$PYTHON_PREFIX')
> 

Erm the above solution gives me a headache, and as said my own (simple,
understandable) solution works fine.

All other items fixed, here is a new version:
Spec URL: http://people.atrpms.net/~hdegoede/pygtkglext.spec
SRPM URL: http://people.atrpms.net/~hdegoede/pygtkglext-1.1.0-2.fc7.src.rpm
Comment 4 Mamoru TASAKA 2007-03-30 12:55:23 EDT
Okay.

--------------------------------------------
  This package (pygtkglext) is APPROVED by me
--------------------------------------------

(In reply to comment #3)
> (In reply to comment #2)
> > * %files
> >   - By the way, why does file entry has the same line?
> >     (perhaps you just did copy & paste)
> > 
> Those lines are not identical (gdkgl versus gtkgl)

Oops.....
Comment 5 Hans de Goede 2007-03-30 14:28:13 EDT
New Package CVS Request
=======================
Package Name:      pygtkglext
Short Description: Python bindings for GtkGLExt
Owners:            j.w.r.degoede@hhs.nl
Branches:          FC-6 devel
InitialCC:         <empty>
Comment 6 Hans de Goede 2007-04-02 02:55:55 EDT
Many thanks for the review!

Imported and build, closing.
Comment 7 Mamoru TASAKA 2007-04-02 12:25:15 EDT
(Actually closing)

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