Bug 603481

great to see this package. I have just compiled it and it works great. This solves _alot_ of rdesktop bugs nobody cares about currently in Red Hat bugzilla...

Still works great. ;)

btw, I just let rpmlint ran over the resulting RPMs and got the following.

freerdp-devel.i386: W: spelling-error %description -l en_US unversioned -> reconversion, bioconversion, interconversion
freerdp-devel.i386: W: spelling-error %description -l en_US libfreerdp -> liberticide, subfreezing
freerdp-devel.i386: W: spelling-error %description -l en_US libfreerdpchanman 
freerdp-devel.i386: W: spelling-error %description -l en_US libfreerdpkbd 
freerdp-devel.i386: W: no-documentation
freerdp-libs.i386: W: spelling-error %description -l en_US libfreerdp -> liberticide, subfreezing
freerdp-libs.i386: W: spelling-error %description -l en_US libfreerdpchanman 
freerdp-libs.i386: W: spelling-error %description -l en_US libfreerdpkbd 
freerdp-libs.i386: W: spelling-error %description -l en_US plugins -> plug ins, plug-ins, plugging
freerdp-libs.i386: W: unstripped-binary-or-object /usr/lib/libfreerdpchanman.so.0.0.0
freerdp-libs.i386: W: unstripped-binary-or-object /usr/lib/libfreerdpkbd.so.0.0.0
freerdp-libs.i386: W: unstripped-binary-or-object /usr/lib/libfreerdp.so.0.0.0
freerdp-libs.i386: W: shared-lib-calls-exit /usr/lib/libfreerdp.so.0.0.0 exit
freerdp-libs.i386: W: shared-lib-calls-exit /usr/lib/libfreerdp.so.0.0.0 exit@@GLIBC_2.0
freerdp-plugins-standard.i386: W: unstripped-binary-or-object /usr/lib/freerdp/rdpdr.so
freerdp-plugins-standard.i386: W: unstripped-binary-or-object /usr/lib/freerdp/rdpsnd.so
freerdp-plugins-standard.i386: W: unstripped-binary-or-object /usr/lib/freerdp/cliprdr.so
freerdp-plugins-standard.i386: W: unstripped-binary-or-object /usr/lib/freerdp/printer.so
freerdp-plugins-standard.i386: W: unstripped-binary-or-object /usr/lib/freerdp/disk.so
freerdp-plugins-standard.i386: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 20 warnings.

Yes? Do you think any of these rpmlint warnings are relevant?

FWIW, I don't get the unstripped-binary-or-object warnings. What system are you on? You didn't use the packages from koji?

Spec URL: http://kiilerix.fedorapeople.org/freerdp.spec
SRPM URL: http://kiilerix.fedorapeople.org/freerdp-0.7.1-1.fc13.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2279026

the packages from koji infact do not have the problem with unstripped binaries...

I now only get:

freerdp-libs.i686: W: shared-lib-calls-exit /usr/lib/libfreerdp.so.0.0.0 exit
This library package calls exit() or _exit(), probably in a non-fork()
context. Doing so from a library is strongly discouraged - when a library
function calls exit(), it prevents the calling program from handling the
error, reporting it to the user, closing files properly, and cleaning up any
state that the program has. It is preferred for the library to return an
actual error code and let the calling program decide how to handle the
situation.

Otherwise it is great to see an rdp client with active development. As my university has moved all servers to Windows 2008 R2 you really made my experience better.

Thanks!

(In reply to comment #4)
> freerdp-libs.i686: W: shared-lib-calls-exit /usr/lib/libfreerdp.so.0.0.0
> exit

Yes, that is a bad design, and we will fix it upstream eventually. But for now that is how this API looks like upstream, so that is how it is packaged. AFAIK the packaging guidelines doesn't say anything about this.

The rpmlint warning shared-lib-calls-exit usually is not a review blocker as long as the functionality of the app doesn't suffer from that. It's just a sign of upstream's bad design. It should eventually be fixed upstream.

*** Bug 616193 has been marked as a duplicate of this bug. ***

I will merge with whatever there might be in bug 616193 and update to 0.7.3 (which will be released in a couple of days).

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

(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

I have updated to 0.7.3 and verified that pylint has no relevant complaints.

Spec URL: http://kiilerix.fedorapeople.org/freerdp.spec
SRPM URL: http://kiilerix.fedorapeople.org/freerdp-0.7.3-1.fc13.src.rpm

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

(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. ;)

> I'm surprised to see the libs still link against libpcsclite.so.1 and
> linux-vdso.so.1 they were available during build.

Which libs linked against it? I assume that rpmlint complained that they were unused?

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

I think my main comment to the package naming/separation is that upstream consider the library the primary product of the project. xfreerdp is just a demo-sample-toy-playground. Remmina (and similar projects if there are any) are the primary users of the library.

Ok, so remembering the arguments and how you and I did it I now tend to prefer:

a "freerdp" package that contains /usr/bin/xfreerdp and provides "xfreerdp".

a "freerdp-libs" package that contains the main libs ... and perhaps everything else

a "freerdp-libs-devel" with headers for the libs (though it seems like it usually is done as "freerdp-devel"?)

I will try to make up my mind ...

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

Implementing MS protocols is actually not very funny ;-)

(In reply to comment #13)
> Which libs linked against it? I assume that rpmlint complained that they were
> unused?

You are right, libpcsclite.so.1 is unused (although rpmlint didn't complain).

> I think my main comment to the package naming/separation is that upstream
> consider the library the primary product of the project. xfreerdp is just a
> demo-sample-toy-playground. Remmina (and similar projects if there are any) are
> the primary users of the library.

Hmm, I was under the impression that xfreerdp was supposed to be the counterpart of the rdesktop binary and the main user.

> Ok, so remembering the arguments and how you and I did it I now tend to prefer:
> 
> a "freerdp" package that contains /usr/bin/xfreerdp and provides "xfreerdp".
> 
> a "freerdp-libs" package that contains the main libs ... and perhaps everything
> else
> 
> a "freerdp-libs-devel" with headers for the libs (though it seems like it
> usually is done as "freerdp-devel"?)

Sounds fine for me. The devel package should be named freerdp-devel and adding a virtual "Provides: xfreerdp" is a good idea, although it's not strictly needed.

Regarding the plugins, the package should IMHO just be called freerdp-plugins. Or you package them completely separated as freerdp-plugin-foo.

> Implementing MS protocols is actually not very funny ;-)    

Some people are just masochistic. ;)

(In reply to comment #14)
> Regarding the plugins, the package should IMHO just be called freerdp-plugins.
> Or you package them completely separated as freerdp-plugin-foo.

Couldn't the plugins be included in the libs package together with the libs linked directly from xfreerdp/remmina? You almost argued/agreed that everybody needed everything anyway, so fine-grained packaging had no benefit?

I settled on freerdp providing xfreerdp, freerdp-libs with the dynamic libraries, freerdp-plugins for "optional" plugins to libfreerdpchanman from freerdp-libs, and freerdp-devel.

Spec URL: http://kiilerix.fedorapeople.org/freerdp.spec
SRPM URL: http://kiilerix.fedorapeople.org/freerdp-0.7.4-1.fc13.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2424438

Good to see that this already in progress. freerdp worked out-of-the-box with Win7, whereas rdesktop-1.6.0 does not (only the latest SVN code does).

FYI: I have filed an enhancement request to KDEs krdc to support freerdp:

  <https://bugs.kde.org/show_bug.cgi?id=250979>

Hi,
 Was looking at this today as am interested. If building with an 

 freerdp.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/xfreerdp ['/usr/lib64']


 looks to have been introduced in the rpmlint.

 See: http://fedoraproject.org/wiki/Packaging/Guidelines#Beware_of_Rpath

 p.s When this is certified I'd be keen to see an EPEL release as well.

Steve.

(In reply to comment #18)
>  Was looking at this today as am interested. If building with an 
> 
>  freerdp.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/xfreerdp
> ['/usr/lib64']

What platform are you using? There is no such warning on F13 i686.

(In reply to comment #19)
> (In reply to comment #18)
> >  Was looking at this today as am interested. If building with an 
> > 
> >  freerdp.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/xfreerdp
> > ['/usr/lib64']
> 
> What platform are you using? There is no such warning on F13 i686.

F13 x86_64. 

Trying to get rid of the error since not your platform:

--disable-rpath does not work 

however adding 

sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool

after %configure does the job.

%build
%configure --disable-static --with-sound --with-crypto=openssl
sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool
make %{?_smp_mflags}


rpmlint now looks clean other than 

freerdp-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libfreerdp.so.0.0.0 exit.5

as mentioned above.

Right, my testing had showed that it wasn't a problem on i686, but it turns out to be x86_64-specific.

Spec URL: http://kiilerix.fedorapeople.org/freerdp.spec
SRPM URL: http://kiilerix.fedorapeople.org/freerdp-0.7.4-2.fc13.src.rpm

Updated to 0.8.1

Spec URL: http://kiilerix.fedorapeople.org/freerdp.spec
SRPM URL: http://kiilerix.fedorapeople.org/freerdp-0.8.1-1.fc14.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2583728

Sorry it took so long, this dropped of my radar.

(In reply to comment #16)
> I settled on freerdp providing xfreerdp, freerdp-libs with the dynamic
> libraries, freerdp-plugins for "optional" plugins to libfreerdpchanman from
> freerdp-libs, and freerdp-devel.

Sounds good to me.


REVIEW FOR 89b239734db4e419925af122ef5d0301  freerdp-0.8.1-1.fc14.src.rpm

OK - MUST: $ rpmlint /var/lib/mock/fedora-14-x86_64/result/*.rpm
freerdp.src: W: spelling-error %description -l en_US xrdp -> xref, hardpan, Oxnard
freerdp.src: W: spelling-error %description -l en_US rdesktop -> desktop, r desktop, copydesk
freerdp.x86_64: W: spelling-error %description -l en_US xrdp -> xref, hardpan, Oxnard
freerdp.x86_64: W: spelling-error %description -l en_US rdesktop -> desktop, r desktop, copydesk
freerdp-devel.x86_64: W: spelling-error %description -l en_US libs -> lobs, lib, lis
freerdp-devel.x86_64: W: no-documentation
freerdp-libs.x86_64: W: spelling-error %description -l en_US libfreerdp -> liberticide, subfreezing
freerdp-libs.x86_64: W: spelling-error %description -l en_US libfreerdpchanman 
freerdp-libs.x86_64: W: spelling-error %description -l en_US plugins -> plug ins, plug-ins, plugging
freerdp-libs.x86_64: W: spelling-error %description -l en_US libfreerdpkbd 
freerdp-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libfreerdp.so.0.0.0 exit.5
freerdp-plugins.x86_64: W: no-documentation
6 packages and 0 specfiles checked; 0 errors, 12 warnings.

- Spelling errors can be ignored, the spelling is correct.
- No documentation can be ignored, the docs are in another subpackage
- shared-lib-calls-exit should be fixe upstream. Not a blocker.

OK - MUST: named according to the Package Naming Guidelines
OK - MUST: spec file name matches the base package %{name}
OK - MUST: package meets the Packaging Guidelines
OK - MUST: Fedora approved license and meets the Licensing Guidelines (GPLv2+)
OK - MUST: License field in spec file matches the actual license
OK - MUST: license file included in %doc
OK - MUST: spec is in American English
OK - MUST: spec is legible
FIX - MUST: sources do not match the upstream source by MD5 

- Upstream: 8a265ce267ea6508d30db29fd3c3c037
- SRPM: 1e64b766874966004c07db12fe73dde8

OK - MUST: successfully compiles and builds into binary rpms on x86_64
N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
OK - MUST: all build dependencies are listed in BuildRequires.
N/A - MUST: handles locales properly with %find_lang
OK - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun (freerdp-libs).
OK - MUST: Package does not bundle copies of system libraries.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
OK - MUST: owns all directories that it creates
OK - MUST: no duplicate files in the %files listing
OK - MUST: Permissions on files are set properly, includes %defattr(...)
OK - MUST: consistently uses macros
OK - MUST: package contains code, or permissable content
N/A - MUST: Large documentation files should go in a -doc subpackage
OK - MUST: Files included as %doc do not affect the runtime of the application
OK - MUST: Header files are in -devel package
N/A - MUST: Static libraries must be in a -static package
OK - MUST: library files that end in .so are in the -devel package.
OK - MUST: devel packages requires the freerdp-libs package using a fully versioned dependency
OK - MUST: The package does not contain any .la libtool archives.
OK - The package contains a GUI application, but as xfreerdp cannot be called without arguments, the desktop file is useless
OK - MUST: package does not own files or directories already owned by other packages.
OK - Should: at the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT.
OK - MUST: all filenames valid UTF-8


SHOULD Items:
OK - SHOULD: Source package includes license text(s) as a separate file.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: builds in mock.
OK - SHOULD: compiles and builds into binary rpms on all supported architectures (tested in koji).
OK - SHOULD: functions as described.
OK - SHOULD: Scriptlets are sane.
OK - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
OK - SHOULD: pkgconfig(.pc) files are placed in -devel pkg
OK - SHOULD: no file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin
OK - SHOULD: package should contain man pages for binaries/scripts.

Other items:
OK - latest stable version
OK - SourceURL valid
OK - Compiler flags ok
OK - Debuginfo complete
OK - SHOULD: package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
FIX - SHOULD: -devel package contains a pkgconfig(.pc) files must 'Requires: pkgconfig'.


Please download the source tarball again to make sure it's timestamp and md5 matches. Fix the points marked with FIX before build and consider the package APPROVED.

(In reply to comment #23)

Thanks for the review. But ... please comment on this:

> FIX - MUST: sources do not match the upstream source by MD5 
> 
> - Upstream: 8a265ce267ea6508d30db29fd3c3c037
> - SRPM: 1e64b766874966004c07db12fe73dde8

I get 1e64... for http://downloads.sourceforge.net/freerdp/freerdp-0.8.1.tar.gz no matter what I do. How/where do you get 8a26...?

> FIX - SHOULD: -devel package contains a pkgconfig(.pc) files must 'Requires:
> pkgconfig'.

AFAICS the following says that new packages shouldn't require it explicitly.
http://fedoraproject.org/wiki/Packaging:Guidelines#Pkgconfig_Files
(http://fedoraproject.org/wiki/PackagingDrafts/PkgconfigAutoRequires)

> > FIX - SHOULD: -devel package contains a pkgconfig(.pc) files must 'Requires:
> > pkgconfig'.
> 
> AFAICS the following says that new packages shouldn't require it explicitly.
> http://fedoraproject.org/wiki/Packaging:Guidelines#Pkgconfig_Files
> (http://fedoraproject.org/wiki/PackagingDrafts/PkgconfigAutoRequires)

you require it explicitly for EPEL4 and 5 only where it is not computed.

(In reply to comment #24)
> I get 1e64... for http://downloads.sourceforge.net/freerdp/freerdp-0.8.1.tar.gz
> no matter what I do. How/where do you get 8a26...?

I used spectool and the result I got was indeed 8a26... Must have been a bad download. I have tried 3 times and always got 1e64...

> AFAICS the following says that new packages shouldn't require it explicitly.
> http://fedoraproject.org/wiki/Packaging:Guidelines#Pkgconfig_Files
> (http://fedoraproject.org/wiki/PackagingDrafts/PkgconfigAutoRequires)

That page is still a draft and it only says that *do*not*need* it but not that you *should*not require it. As it is stil required for EPEL 4 and 5 and someone already indicated he wants to maintain this for EPEL, I suggest to add it.

Anyway, not a blocker, please go ahead with the SCM admin request.

New Package SCM Request
=======================
Package Name: freerdp
Short Description: remote desktop protocol client
Owners: kiilerix
Branches: f14 el4 el5 el6
InitialCC:

Git done (by process-git-requests).

freerdp-0.8.1-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/freerdp-0.8.1-2.fc14

Great job! Thank you so much to Mads Kiilerich and all involved. I'm so happy to see this package in Fedora!

freerdp-0.8.1-2.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update freerdp'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/freerdp-0.8.1-2.fc14

freerdp-0.8.1-2.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Hi Mads,

 Are you able to create EPEL packages also?

Steve.

(In reply to comment #33)
> Hi Mads,
> 
>  Are you able to create EPEL packages also?
> 
> Steve.
Yes, that would be great.
I was able to rebuild freerdp-0.8.1-2.fc14 (rpmbuild --rebuild) on RHEL 6 and it works fine.

I'd like to ask for the F-13 branch, if possible.

(In reply to comment #35)
> I'd like to ask for the F-13 branch, if possible.

ask if kiilerix can maintain the F-13 branch, if he don`t want you can ask for cvs in this or a new bug and maintain it.

Package Change Request
======================
Package Name: freerdp
New Branches: f13

EL5 packages has been tagged for testing.

EL4 seems to be harder - see build failure on http://koji.fedoraproject.org/koji/taskinfo?taskID=2613625 . If anyone cares and can come up with a simple fix then I will maintain it ;-)

el4 fails to build due some dependencies, if you discover what dependencies replaces the packages bellow tell me.

DEBUG util.py:268:  No Package Found for libXcursor-devel
DEBUG util.py:268:  No Package Found for libXfixes-devel
DEBUG util.py:268:  No Package Found for libX11-devel
DEBUG util.py:268:  No Package Found for libXrender-devel

The SCM request is not valid; it specifies no owners.  Please do not deviate
from the procedure specified in
http://fedoraproject.org/wiki/Package_SCM_admin_requests

Package Change Request
======================
Package Name: freerdp
New Branches: f13
Owners: kiilerix

Git done (by process-git-requests).

/usr/share/icons/hicolor/*/apps/freerdp.png have the wrong permissions. Should be 644 instead of 755

Please file a new bug report instead of commenting on this one.