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

Tue Jul 21 20:51:47 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 #23 from Christian Krause <chkr at plauener.de>  2009-07-21 16:51:45 EDT ---
Thanks for the new package.

(In reply to comment #13)
> (In reply to comment #8)
> > - lots of rpmlint errors:
> > * wrong permissions of some of the *.c files, please change the permission to
> > 644 in the %prep section
> Excuse me - I don't understand you. *.c files it is sources and they only built
> and do not go in any packages. Why I shoud care about its permissions? And what
> mean "wrong" in this context? BTW rpmlint offcourse silent about it.

rpmlint RPMS/i386/x11vnc-debuginfo-0.9.8-3.fc10.i386.rpm 
x11vnc-debuginfo.i386: W: spurious-executable-perm
/usr/src/debug/x11vnc-0.9.8/libvncserver/rfbregion.c
x11vnc-debuginfo.i386: W: spurious-executable-perm
/usr/src/debug/x11vnc-0.9.8/libvncserver/auth.c
[...]

> > - the indentation seems to be broken - just open the file with vim or gedit
> No. Just I use tabsize = 5 spaces.

But IMHO it looks bad for nearly all people since most others use tabwidth = 8.
Just try to click on the link in your browser and the the lines will look
somehow displaced. I suggest to convert it to spaces since that's a good
compromise and it will look good for all people. ;-)

> > - correct the Source: line
> > * use Source0 for the primary source file
> Renamed. Is there any difference in it? I remember it may have sense in
> patches... in sources too?

No, it's just a convenience / some kind of standard in Fedora.

> > - check for spelling mistakes in the comments ;-) ("pathc")
> I'm try, seriously. My English skills is very low. So do not hesitate point me
> on such errors too. 

No problem - I'm also not an native English speaker - if in doubt just load the
specfile into gedit and turn on the automatic spell checker and roughly scan
for mistakes... ;-)

> > - would it be an option to use the system's libvncserver library instead of the
> > internal one? (--use-system-libvncserver
> This is very interesting question. I think it is will be "right" solution.
> Additionally it completely solve minilzo problem as I can understand.
> I try use it now (add --with-system-libvncserver). Add BR libvncserver-devel
> But I have errors (
> http://koji.fedoraproject.org/koji/getfile?taskID=1487784&name=build.log )
> several "undefined reference" like:
> /builddir/build/BUILD/x11vnc-0.9.8/x11vnc/connections.c:3161: undefined
> reference to `rfbUnregisterTightVNCFileTransferExtension'
> x11vnc-connections.o: In function `client_gone':
> /builddir/build/BUILD/x11vnc-0.9.8/x11vnc/connections.c:726: undefined
> reference to `rfbRegisterTightVNCFileTransferExtension'
> x11vnc-help.o:make[3]: Leaving directory
> `/builddir/build/BUILD/x11vnc-0.9.8/x11vnc'
>  In function `print_help':
> /builddir/build/BUILD/x11vnc-0.9.8/x11vnc/help.c:4862: undefined reference to
> `rfbRegisterTightVNCFileTransferExtension'
> 
> Disable this option now, but continue investigation there...

I consider this more important than the lzo issue, since libvncserver is
intended to be a shared library. When tightvnc filetransfer is disabled, x11vnc
can be compilee without any problems:

./configure --with-system-libvncserver --without-tightvnc-filetransfer

> > > Epel lzo do not contain minilzo variant of package:
> > [...]
> > > So, should we try patch it use lzo instead?  
> > Yes, pretty please.  
> Yeh, ok, I'll try do it.  

I agree this would be the best solution. If you like you can try to implement
it using a similar configure option as for libvncserver. This patch could then
be sent upstream and hopefully applied so that problem would be gone for
further releases.

> > > And
> > > please note, this is only performance wise on build stage it is not related in
> > > any user installation of package.
> > You are correct. But why make the builders cry if the solution is so simple ?
> Why cry??? If you build this package at home, its will be satisfied once. Most
> of builds I do on koji, there also no problem - plus few seconds...
> 
> What problem you a inspired with it?

I've asked on #fedora-devel and I got the impression that file based build
requires are strongly discouraged. The main reason is the increased build time.
I think we must not only consider the increased build time for single
developers but also the increased load on the koji machines. 

Two additional minor remarks:

Please don't indent the whole for loop when changing file encoding. ;-)

Use the macros consistently - one plain "rm" leaked in although you've used
"%{__rm}" in all other places...

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