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=510734 --- Comment #52 from Christian Krause <c...@plauener.de> 2009-08-25 16:00:16 EDT --- Pavel, Thanks for all your work on this package and also for your patience, here is now the full review of the package. In general it looks good, but there are still some issues left. * rpmlint: OK rpmlint SPECS/x11vnc.spec SRPMS/x11vnc-0.9.8-8.fc10.src.rpm RPMS/i386/x11vnc-* 3 packages and 1 specfiles checked; 0 errors, 0 warnings. * naming: OK - name matches upstream - spec file name matches package name * License: TODO 1. source files - OK The COPYING files contains the regular GPLv2. There are some source files released under GPLv2+ and there are also a couple of source files which explicitly state GPLv2. Most of these files are in the unused libvncserver/ directory, so they can be ignored. However, the file x11vnc/x11vnc.c which used to build /usr/bin/x11vnc (included in the binary RPM) is licensed under GPLv2. This would lead to the conclusion that the whole package should be GPLv2. - License tag: OK - matches actual License: OK - License file packaged: OK - OpenSSL exception: OK In theory GPL code can not be compiled against the openssl library. However, Karl Runge, the upstream author, has explicitely give the permission to do so" In addition, as a special exception, Karl J. Runge gives permission to link the code of its release of x11vnc with the OpenSSL project's "OpenSSL" library (or with modified versions of it that use the same license as the "OpenSSL" library), and distribute the linked executables. You must obey the GNU General Public License in all respects for all of the code used other than "OpenSSL". If you modify this file, you may extend this exception to your version of the file, but you are not obligated to do so. If you do not wish to do so, delete this exception statement from your version. " 2. binary files - TODO The upstream package ships some pre-compiled java binaries: x11vnc-0.9.8/classes/VncViewer.jar x11vnc-0.9.8/classes/ssl/VncViewer.jar x11vnc-0.9.8/classes/ssl/SignedVncViewer.jar x11vnc-0.9.8/classes/ssl/UltraViewerSSL.jar x11vnc-0.9.8/classes/ssl/SignedUltraViewerSSL.jar The jar files can be re-generated using the shipped sources (GPLv2). However, as Tom Callaway pointed out, only if we are 100% sure about the actual license of the binary files we may distribute it. I have checked their web page as well as the various README files in the package and I could not find any specific information regarding the status of the binaries. This means, that I'm not 100% sure about it and so I think we should re-package it: http://fedoraproject.org/wiki/Packaging:SourceURL#When_Upstream_uses_Prohibited_Code However, if Tom or somebody else from the FE-LEGAL team gives their explicit permission for redistribution I would be happily accept this. ;-) * specfile in American English and legible: TODO - indentation: I know this may sound like nitpicking, however I'm still convinced that the spec files should be written in such a way that it can be easily read by any other user with any program. It should not be necessary to guess the tab width for each spec file. ;-) I think using some kind of standard for things like this will certainly help that all Fedora package maintainers can easily work together. - typo: pathes -> paths - wording: IMHO "performant" isn't an Enlish word, probably just use "fast" - spelling: "prebuild clients" -> "prebuilt clients" - spelling: "acording" -> "according" - spelling: "macroses" -> "macros" - spelling: "arount" -> "around" - indentation in %prep: The small code snippet to do the conversion into UTF-8 is not indented correctly. The begin ("for file....") and the end ("done") of the for loop should be one tab more to the left than the body of the for loop. - please order the patches by number * %description: TODO (minor) - Although "x11vnc" is a name, I would start it with a capital "C" at the beginning of the sentence. - In the second sentence I think the comma can be omitted. * Sources: OK - Source0 URL ok - spectool -g x11vnc.spec works - sources matches upstream - md5sum: 13e41380fe9ba2581db180061d1cbd22 x11vnc-0.9.8.tar.gz * Patch0: TODO During the review I was wondering that x11vnc is neither linked against the lzo library nor the minilzo library. Indeed since we neither build or use libvncclient nor libvncserver from the x11vnc package, there is no need at all to take care of lzo here. Just to be on the save side I would still delete the minilzo.[ch] in the spec file, but I think the lzo patch as well as the usage of __ln_s/__sed can be omitted. * Patch1: OK * Patch2: OK * Compilation: TODO - the first 2 lines which exports the CFLAGS and the LDFLAGS should not be necessary Otherwise, the compilation is OK - package builds correctly in koji for F12, F11 and F10 - RPMOPTFLAGS used - parallel build supported via _smp_mflags * debuginfo sub-package: OK - non-empty - debuginfo file works together with gdb * BuildRequires: TODO - most likely lzo-devel can be omitted * Locales handling: OK (n/a) * shared/static libs, pkgconfig/header/*.la files: OK (n/a) * packages must own all directories: OK * packaged files and directories: TODO It looks like that /usr/share/x11vnc is packaged although it is empty. Please don't package it. * files not listed twice: OK * permissions of files: OK - %defattr used - final file permissions OK * %clean section: OK * macro usage: OK * code vs. content: OK (only code) * large documentation into sub-package: OK It may be debatable whether 1MB README file is too large, but since it describes the command line parameters in detail etc. I think it is quite useful even in the main package and since the size is not that big, it seems to be OK. * GUI application needs %{name}.desktop: OK * no directories owned which are already owned by other packages: OK * rm -rf %{buildroot} at the beginning of %{install}: OK * all filenames UTF8: OK * functional test: OK -- 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
