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@xxxxxxxxxxxx> 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review