[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

Christoph Wickert <cwickert at fedoraproject.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|nobody at fedoraproject.org    |cwickert at fedoraproject.org
               Flag|                            |fedora-review?

--- Comment #9 from Christoph Wickert <cwickert at fedoraproject.org> 2010-07-22 13:48:22 EDT ---
Thanks Mads,

you could also update to 0.7.2, so we can continue this review in the meantime.

Too bad I didn't see this review when I did my package. My spec looks different
from yours and of course I think mine is better. ;)

Some comments on your package:

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

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

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

- 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. Then
you could spilt the noarch and the arched content, but I don't think this adds
much value here.

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

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

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

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

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

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

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

- add more documentation please

- merge whatever you think makes sense from my package

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