[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 #12 from Christoph Wickert <cwickert at fedoraproject.org> 2010-07-28 19:45:02 EDT ---
(In reply to comment #11)
> I have updated to 0.7.3 and verified that pylint has no relevant complaints.

You mean rpmlint, right? ;)

> cwickert, I hope that you will comment on my comments above.    

Sure Mads, I will, I was just busy with $dayjob.

(In reply to comment #10)
> (In reply to comment #9)
>
> 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.

Indeed, it contains xfreerdp, but it is optional. What makes you think it's
not? It's not required by any of the other packages. Whatever implements the
same functionality as xfreerdp would just require freerdp-common.

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

The important point is that everything is named freerdp: The project, the
homepage, the source tarball and the srpm. Everything is advertised as freerdp
and not xfreerdp. Thus, it should be able to install the package by running
'yum install freerdp'.

Imagine you want to file a bug against the xfreerdp package. How are users
supposed to know that they have to file the bug against the freerdp component
in bugzilla?

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

For the reasons outlined abouve, I still think the package should just be named
freerdp instead if xfreerdp. You have no freerdp binary package at all, so why
not just rename the package?

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

I'm a friend of fine grained packaging, but I'm not sure this makes sense in
this case. Both cups and pcsc are part of the livecd and the default install.

The thing I dislike most is the name of the subpackage. It should be named
freerdp-plugins-base or just freerdp-plugins to follow the normal naming. Or
you split them up even further into freerdp-plugin-cups, freerdp-plugin-pcsc,
...

Another problem is to make sure that an app like remmina gets the full set of
features that made them switch to freerdp. This can only be done by requiring
the plugins explicitly.

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

Fair enough-

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

OK, as for the other BuildRequries. I think someone needs to clean up the code
a little. I'm surprised to see the libs still link against libpcsclite.so.1 and
linux-vdso.so.1 they were available during build.

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

This means that xfreerdp is the only working implementation of freerdp, right?
One more reason to have it in the freerdp binary package.

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

Another thing to fix upstream. Yeah, I know, writing documentation sucks,
coding is more fun. ;)

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