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

[bugzilla at redhat.com]

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 #43 from Pavel Alexeev (aka Pahan-Hubbitus) <pahan at hubbitus.info>  2009-08-04 06:38:28 EDT ---
(In reply to comment #40)
> The spec file should be easily readable without any specific tab width
> settings. Please use either a standard tab width or convert it to spaces.
You are first reviewer what don't accept that. File is easy readable. This
style of formating not covered any guidelines, as I can understand (kick me, if
I wrong) and I want leave it as it is.

> I'm not aware of any explicit documentation which requires "Source0", however
> this is also some kind of standard in all Fedora packages. E.g. see the
> examples here:
> http://fedoraproject.org/wiki/Packaging/SourceURL
Only examples? I'm make decision what this is some kind of maintainer
preferables only such as using $RPM_BUILD_ROOT or %{buildroot}, rm or %{__rm}
etc...

> Since this seems to be discouraged in Fedora, please don't do it. The
> guidelines don't explicitly forbid this, but at least for the Requires field it
> is discouraged: http://fedoraproject.org/wiki/Packaging/Guidelines#Requires
This covered small later:
http://fedoraproject.org/wiki/Packaging/Guidelines#File_Dependencies
Cite from it:
Rpm gives you the ability to depend on files instead of packages. Whenever
possible you should avoid file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin. Using file dependencies outside of those directories
requires yum (and other depsolvers using the repomd format) to download and
parse a large xml file looking for the dependency. Helping the depsolvers avoid
this processing by depending on the package instead of the file saves our end
users a lot of time.
=====/ End cite ====

So, key there "end users". Small later in this guidelines you can also find
useful example when it is preferable. Nothing there about BuildRequires! So I
don't see what it would be discouraged.

> > > Use the macros consistently - one plain "rm" leaked in although you've used
> > > "%{__rm}" in all other places...  
> > Ok.
> 
> There is one missing where your removed the prebuilt clients.
Upps. Sorry, I just don't update spec.
Now I also replace by macroses other things like mv, sed, ln...

> 
> Some more remarks:
> 
> Would it be possible to link it against the regular liblzo even for the Fedora
> package? This would save us one condition.
It is possible - http://koji.fedoraproject.org/koji/taskinfo?taskID=1579635
But it some sort of hack. Are you sure what we should do it?

> Additional if it would be possible to create a patch which would make this a
> compile option to switch between minilzo (which is designed to be internal) and
> external lzo then this patch would be hopefully acceptable for upstream.
There I agree with you - such option would be appreciated. But it requires some
additional times, and I wasn't planing do that. May be in the future.

> The part of the %prep section which changes the encoding is not correctly
> indented.
Hm... What exactly? Everything seems correctly intended for me.


http://hubbitus.net.ru/rpm/Fedora11/x11vnc/x11vnc-0.9.8-7.fc11.src.rpm
http://hubbitus.net.ru/rpm/Fedora11/x11vnc/x11vnc.spec

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