[Bug 623425] Review Request: python-pyside - Python bindings for Qt4

Sun Aug 15 17:26:08 UTC 2010

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=623425

--- Comment #10 from Kalev Lember <kalev at smartlink.ee> 2010-08-15 13:26:07 EDT ---
Thanks for the review, Orcan!

(In reply to comment #9)
> * koji scratch build failed
>    http://koji.fedoraproject.org/koji/taskinfo?taskID=2401747

Yes, as of right now python-pyside is only buildable in rawhide. I'll build the
whole pyside stack (apiextractor, generatorrunner, shiboken, python-pyside) in
F13 and F14 eventually, but as it needs buildroot overrides, I'll do it once
the newly imported package is in rawhide.

> ! rpmlint says
>    python-pyside-devel.x86_64: W: no-documentation
> It seems like the directory doc/ contains some developer documentation. No?
> Also the file ChangeLog contains developer oriented information.

Looks like it needs Qt source tree to generate API documentation. Not sure how
to solve this; what do you think? Bundling whole qt tarball with python-pyside
source rpm would probably work, but I'm not sure if we want to go down that
road.

In any case the docs are also available at  http://www.pyside.org/docs/pyside/

> * The file PySide/licensecomment.txt should be packaged.

Done.

> * The source files in libpyside/ are "LGPLv2 with exceptions", however the
> source files in PySide/ does not specifiy a license in their headers. Please
> notify upstream. However the file PySide/licensecomment.txt gives us a hint
> that these files are LGPLv2. So the full license tag should be
>    License: LGPLv2 and LGPLv2 with exceptions
> Please verify this with upstream and note this in the specfile.

Opened a ticket with upstream bug tracker:
http://bugs.openbossa.org/show_bug.cgi?id=297

I'll update the license tag once I get confirmation.

> * Patch0:         python-pyside-release-type.patch
> Is this a Fedora specific patch? Is it upstreamable?

I need to give it some more thought; perhaps we need to change something on
Fedora's cmake side instead. In any case, right now I consider this as a Fedora
specific patch which makes sure CMake's default -O3 doesn't end up overriding
-O2 in $RPM_OPT_FLAGS.

> * The Group tag for the main package should be System Environment/Libraries

I disagree here. Both PyQt4 and pygtk2 use the group Development/Languages.

> ! The file
>    tests/util/valgrind-python.supp
> contains hardcoded /usr/lib/ strings. I don't know if this affects the tests.

Looks like it's a valgrind suppression file for people who want to debug Python
in valgrind. At least it doesn't appear to get used during the build.

> * I am also not sure about the naming of the package. The guideline says:
>    """There is an exception to this rule. If the upstream source has "py" (or
> "Py") in its name, you can use that name for the package. So, for example,
> pygtk is acceptable. """

PySide should get Python 3 compatibility very soon now, so we'll most likely
end up with separate packages for both Python 2 and Python 3. Fedora Python
guidelines require that Python 3 modules need to be prefixed with python3-. I
think it looks nicer if the packages are called "python-pyside" and
"python3-pyside" as opposed to just plain "pyside" and "python3-pyside".

> Note that it says "you can" and not "you should/must". Shall we name the
> package just PySide? You can add a virtual provides "python-pyside" if you want
> Debian compatibility. Note also that Debian calls PyQt4 as python-qt4, very
> strange. There are other naming weirdnesses on Debian too. I think that it is
> Debian who deviates from upstreams.

In this case, it's _upstream_ that's mostly using the name "python-pyside" in
their packaging page: http://www.pyside.org/downloads/
Variations found in this page are:
python-pyside
pyside-qt4
pyside-git

I'd rather pick one from the list above instead of inventing yet another name
for Fedora.

> ! QtMultimedia_audio_test fails. This may be a python-2.7 incompatibility.
> 
>    185: Traceback (most recent call last):
>    185:   File
> "/builddir/build/BUILD/pyside-qt4.6+0.4.0/tests/QtMultimedia/audio_test.py",
> line 30, in testListDevices
>    185:     self.assert_(False)
>    185: AssertionError: False is not True

Perhaps; or it might be something that changed in Qt 4.7. On my F-13 machine
the very same test dies with an assertion in what appears to be pulseaudio code
instead:
python2.6: pcm_params.c:2348: sndrv_pcm_hw_params: Assertion `err >= 0' failed.
/home/kalev/rpmbuild/BUILD/pyside-qt4.6+0.4.0/tests/run_test.sh: line 13: 14050
Aborted                 $3 $4

> ! The Phonon test segfaults
> 
>    DEBUG: 186: .../builddir/build/BUILD/pyside-qt4.6+0.4.0/tests/run_test.sh:
> line 13: 29595 Segmentation fault      (core dumped) $3 $4
>    DEBUG: 186/189 Test #186: phonon_basic_playing_test
> .......................***Failed    2.59 sec
> 
> I hope this is not significant.

It's not showing up in the scratch builds I posted above though. How did you
get it to segfault?

> ! You don't require pkgconfig. This is fine if this package is Fedora only.

Yes, it's meant only for Fedora and perhaps EPEL 6 if someone wants to take
care of that one day. It's not really buildable on EPEL 4 and 5; Qt just isn't
new enough in there.

> ? On the devel package, do we really need
>    Requires:       shiboken-devel
> I checked the other Requires, and I verified they are all needed, but I can't
> find the use of this one.

all the /usr/include/PySide/*/pyside_*_python.h files have:
#include <basewrapper.h>

The file basewrapper.h is from shiboken-devel.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.