[Bug 510734] Review Request: x11vnc - VNC server for the current X11 session

Tue Aug 25 20:00:22 UTC 2009

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 <chkr at 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.