[Bug 603481] Review Request: freerdp - remote desktop protocol client

[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=603481

--- Comment #10 from Mads Kiilerich <mads at kiilerich.com> 2010-07-22 21:41:49 EDT ---
(In reply to comment #9)

It is late/early here, so an incomplete response follows. I have updated the
spec at http://kiilerix.fedorapeople.org/freerdp.spec . I will test and review
it again and provide rpms "tomorrow". Comments are welcome before that.

> - Whenever you change something, increase the release and add a changelog
> entry, even during review.

Yes. I was lazy because nobody had looked at it yet and no review was started.

> - The header of the spec could be more legible. Some lines are indented, most
> are not. I suggest to use the template from rpmdev-newspec.

It was based on the old spec inherited from rdesktop. I have now aligned it
more with newspec and your package so we can see the important differences.

> - I don't think that xfreerdp should be a separate package. xfreerdp doesn't
> add additional dependencies and is only 53K, so there is not much so save.

Your "freerdp" package also only contains xfreerdp, AFAICS. That should be the
only package that has dependencies to X, so I think it should be optional.

Can you please clarify?

> People should be able to run "yum install freerdp" to get the binary, just like
> it was with rdesktop. It's also hard for people to find the freerdp component
> in bugzilla when they want to file a bug against xfreerdp.

People should be able to install the freerdp package and get the freerdp
binary. But there is no freerdp binary. It is called xfreerdp. Users should
thus install the xfreerdp package to get xfreerdp.

I would have preferred if the binarys name was freerdp, just like firefox is
called firefox everywhere. But now the binary is xfreerdp.

Comments?

> - What is the use of the libs package? If you are making a libs package, you
> should IMHO merge it with the plugins package that contains libraries too. 

libs contains the core functionality of freerdp. It consists of ordinary
dynamic libraries that can be linked into an application (such as xfreerdp or
remmina) and some helper stuff.

Plugins are optional parts that can be loaded by the core libraries. They
implement optional "channels" in the rdp protocol and can thus handle sound,
printing, filetransfer, smartcard, etc. They have dependencies to other
libraries, but they don't have to be used, and other plugins can provide the
same or other functionality. 

For use on a minimal livecd it could perhaps be nice to have sound but avoid
dependencies to cups and pcsc, so we might want to split it up even more.

Comments?

> Then
> you could spilt the noarch and the arched content, but I don't think this adds
> much value here.

96 k? Agreed, could be done, but no point.

> - Subpackages are missing Group tag, thus they will all be
> Applications/Communications instead of System Environment/Libraries,
> Development/Libraries or whatever is appropriate. 

Fixed

> - There are some BuildRequires missing. When I build your package in mock, I
> see
> 
> checking for PCSCLITE... no
> checking for old version of PCSC... no
> [...]
> checking dmedia/audio.h usability... no
> checking dmedia/audio.h presence... no
> checking for dmedia/audio.h... no
> checking sys/audioio.h usability... no
> checking sys/audioio.h presence... no
> checking for sys/audioio.h... no
> checking for LIBAO... no
> checking for ALSA... no
> checking for LIBSAMPLERATE... no
> [...]
> checking for IceConnectionNumber in -lICE... no
> 
> Thus you should add pcsc-lite-devel, libao-devel, alsa-lib-devel,
> libsamplerate-devel and libICE-devel.

configure contains a lot of cruft from rdesktop. I might fix it upstream when I
come to it - unless somebody else does first.

smartcard doesn't work at all ... yet.  channels/rdpdr/smartcard/scard.c is
dead. It was a bug that I had included --enable-smartcard.

libao-devel: channels/rdpsnd/rdpsnd_libao.c is dead

alsa-lib-devel: Fixed - I had missed that

libsamplerate: only used by channels/rdpsnd/rdpsnd_dsp.c which is dead

libICE: no clue why there is an option for it, but it is apparently not used 

(vncserver seems to dead too ... if it ever was alive)

> - Require pcscd-lite because otherwise only pcsc-libs will pulled in.

^^^

> - Requires: pkgconfig s no longer needed for the devel package, rpmbuild will
> determine that autumatically.

Fixed

> - --with-ipv6 is not needed, it is default.

Fixed

> - --with-sound and --with-crypto=openssl are not needed ether if the build
> requirements are correct. However you might want to keep it to spot build
> problems.

(I have patches for NSS and will switch to use that when possible. Still
polishing the patches. I also plan to implement pulseaudio - and smart card.)

> - add --disable-static to prevent building of *.a files.

Fixed (I was removing them together with .la files instead).

> - I have added --with-dfb and built dfbfreerdp as a separate package to avoid X
> deps.

That is far from ready and not suitable for packaging yet.

> - add more documentation please

There is no documentation. The docs in the tar is so outdated and misleading
that they are worse than nothing.

> - merge whatever you think makes sense from my package    

Mostly done

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