Bug 439772 (x11vnc-review)

Builds on EL5.

  0 free  7 open  1 done  0 failed
  1234825 buildArch (x11vnc-0.9.3-1.fc8.src.rpm, ppc64): open
(js20-bc1-11.build.redhat.com) -> closed
  0 free  6 open  2 done  0 failed
  1234823 buildArch (x11vnc-0.9.3-1.fc8.src.rpm, s390): open
(spark.z900.redhat.com) -> closed
  0 free  5 open  3 done  0 failed
  1234824 buildArch (x11vnc-0.9.3-1.fc8.src.rpm, x86_64): open
(hs20-bc1-7.build.redhat.com) -> closed
  0 free  4 open  4 done  0 failed
  1234822 buildArch (x11vnc-0.9.3-1.fc8.src.rpm, ppc): open
(js20-bc1-12.build.redhat.com) -> closed
  0 free  3 open  5 done  0 failed
  1234828 buildArch (x11vnc-0.9.3-1.fc8.src.rpm, ia64): open
(natasha.build.redhat.com) -> closed
  0 free  2 open  6 done  0 failed
  1234826 buildArch (x11vnc-0.9.3-1.fc8.src.rpm, s390x): open
(spud.z900.redhat.com) -> closed
  0 free  1 open  7 done  0 failed
1234821 build (dist-5E-qu-candidate, x11vnc-0.9.3-1.fc8.src.rpm): open
(ls20-bc2-13.build.redhat.com) -> closed
  0 free  0 open  8 done  0 failed

 Needs work:
1. Please use either buildroot or RPM_BUILD_ROOT but not both
2. The Summary line is included twice
3. According to the header of most of the .c files, license is GPLv2+; however
some files under libvncserver include in the comments "see GPL (latest version)"
which might be interpretated into GPLv3 :(
4. I am not sure about the attitude towards using an internal copy of minilzo.
Couldn't x11vnc make use of the existing lzo package ?
5. The ugly part is that x11vnc includes some precompiled java bits and I am
almost certain this is not allowed

Minor
1. Duplicate BuildRequires: xorg-x11-proto-devel (by libXext-devel), zlib-devel
(by openssl-devel)
2. the place for /usr/share/x11vnc/classes/ssl/README is somewhere under doc

Manuel, Thank you for your comments.


 Major
=======

1) This shouldn't be an issue, but fixing.
2) Ooops :)
3) x11vnc itself (x11vnc/x11vnc.c) seems to be under GPLv2
libvncserver looks to be GPLv2+ and GPLv2. I consider "see GPL (latest version)"
to the latest version in time of writing the document.
However, I'm gonna use libvncserver from Fedora (--with-system-libvncserver) and
not from the tarball.
4) Hm, I don't think so. The idea of using this minilzo code is to not include
the whole big lzo package. If libvncserver project ships minilzo code inside, I
suppose they take responsibility of maintaining it inside their code.
5) You're right, this is ugly. I'm excluding those classes from the packages.
End-users can download those classes from upstream website (or use their java
programs) with -httpdir switch. So it does not disable the ability of using
x11vnc with a webserver.


 Minor
=======

1) Fixed
2) Removed


Spec URL: http://mmahut.fedorapeople.org/reviews/x11vnc/x11vnc.spec
SRPM URL: http://mmahut.fedorapeople.org/reviews/x11vnc/x11vnc-0.9.3-2.fc8.src.rpm

wrt libvncserver : excellent choice.
wrt minilzo: to me this looks like a violation of
http://fedoraproject.org/wiki/Packaging/Guidelines#head-17396a3b06ec849a7c0c6fc3243673b17e5fed90
I'll ask in #fedora-devel for a second opinion

Everything seems fine, once I found out the correct answer to the lzo problem
I'll post a full review

(In reply to comment #4)
> wrt minilzo: to me this looks like a violation of
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-17396a3b06ec849a7c0c6fc3243673b17e5fed90
> I'll ask in #fedora-devel for a second opinion

minilzo is part of libvncserver, so maybe we should open a bugreport against
this package?

Is this intended to replace any current VNC servers?

(In reply to comment #6)
> Is this intended to replace any current VNC servers?

Not really, just an additional alternative.

WRT #6: it's not a replacement per se, it's a tool which allows connecting via
VNC to already existing X sessions. it's a sort of X equivalent for screen -R. A
damn useful one, in my opinion, I use it quite often in Centos and F-7

WRT #5: It looks like a minilzo-devel.rpm against which both libvncserver and
x11ssh could be built would be useful, right ? should we poke Rex asking him to
try to split minilzo ?

Manuel, I've opened a separate bug report for this issue, we can discuss it in
bug 439979, so we don't block this review request.

ping, Can we go on with this review request?

Marek, I've uploaded to http://wolfy.fedorapeople.org/x11vnc the rpms for lzo,
minilzo and libvncserver (including -devel).

x11vnc must be patched in order to NOT use it's internal copy of minilzo. maybe
using something similar to https://bugzilla.redhat.com/attachment.cgi?id=302074
(which is ugly but seemed to work..) ?
Second thought, maybe we should just clean libvncserver first since my modified
spec creates packages which do not require libminilzo at runtime. Which I am not
sure that is the correct behaviour.

(In reply to comment #12)
> x11vnc must be patched in order to NOT use it's internal copy of minilzo. 

Are you sure of this? I was under impression that internal copy of minilzo is
shipped with linvncserver. In my spec file I'm using
--with-system-libvncserver=%{_libdir}, so it takes libvncserver libraries from
the system (package libvncserver in our case) and not from the source tar ball.
In other words, it's using internal copy of minilzo _from_ libvncserver package
and thus the problem is with libvncserver package and not anymore with x11vnc.
Is it correct?

Almost sure. According to my build.log, the minilzo.* files bundled with x11vnc
are used. I'll try (later today) to verify if the patched libvncserver is really
patched (see https://bugzilla.redhat.com/show_bug.cgi?id=439979#c14 why) and
re-verify the status of x11vnc towards this afterwards.
Unless you beat me to it, the details of the patched libvncserver plus rpms
built by me are available in #12 and bz #439979 :)

Now I am sure. Using
%setup -q
   find . -name minilzo\* -exec rm -f {} \;
makes the build fail. 
BTW, you have to replace
   %configure --with-system-libvncserver=%{_libdir}
with
   %configure --with-system-libvncserver

Marek, can we move on, please ? the patched libvncserver package which provides
minilzo is available in rawhide.

Manuel,

Sorry, my free time is limited at the moment and I did not find a workaround
about the minilzo yet. I'll check this hopefully by end of this week.

Can't find any spare time for this at the moment, is anyone interested in fixing this outstanding issue?

If no one beats me to it, I'll try to squeeze this issue somewhere in my schedule. Unfortunately I am in the middle of several projects related to $WORK so I am not sure when will I be able to look again at x11vnc

*** Bug 460465 has been marked as a duplicate of this bug. ***

I think the choice to make minilzo a shared lib is probably not really what the authors of lzo or the consumer software intended. The idea was to have a one file statically swallowed in build procedures w/o any further ties to the lzo project. Now if the API/ABI of minilzo changes the other software is dependent on being patched up to work with it.

Just looking at how this issue is holding up a useful package for 6 months (!) and the amount of patching one needs to remove the internal minilzo. I hardly think that upstream will accept any of this patching (did anyone try to send them upstream). Next updates of libvncserver/x11vnc will probably need to adjust/extend the minilzo patching again.

And all that for blindly adhering to guidelines? I'm sure if presented to the FPC they would probably ack the use of minilzo as is. Fedora is currently even more upstream closer than 6 months before, so chances are that upstream proximity will win over other guidelines.

And yes, I did have a check on how to remove minilzo in x11vnc and just see the work involved. Trivial OTOH, but not a small workload to maintain on the other.

Axel, I agree with you. Manuel, what do you think?

I am a bit reluctant but I will not oppose to it, as long as using a static version of minilzo is blessed by the powers-in-charge. I suggest adding this request to the agenda of the next IRC meeting.

Is there any movement???

If you don't want maintain it anymore, please, close bug as WONTFIX, and I'll open new request with it.

Pavel, thank you - go ahead.

Manuel, thank you.

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