Bugzilla will be upgraded to version 5.0 on a still to be determined date in the near future. The original upgrade date has been delayed.
Bug 616193 - Review Request: freerdp - X Remote Desktop Protocol Client
Review Request: freerdp - X Remote Desktop Protocol Client
Status: CLOSED DUPLICATE of bug 603481
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-19 15:56 EDT by Christoph Wickert
Modified: 2010-08-10 19:34 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-07-22 06:47:55 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Christoph Wickert 2010-07-19 15:56:32 EDT
Spec URL: http://cwickert.fedorapeople.org/review/freerdp.spec
SRPM URL: http://cwickert.fedorapeople.org/review/freerdp-0.7.2-1.fc14.src.rpm
Description: FreeRDP is a X Remote Desktop Protocol Client for Windows Terminal Servers. It is a fork of the rdesktop project that intends to rapidly start moving forward and implement features that rdesktop lacks the most.
Comment 1 Terje Røsten 2010-07-20 16:23:47 EDT
Hi Christoph, you might want to review  bug #603481.
Comment 2 Christoph Wickert 2010-07-22 06:47:55 EDT
I wonder why I didn't find it when I packaged freerdp.

*** This bug has been marked as a duplicate of bug 603481 ***
Comment 3 Mads Kiilerich 2010-07-22 21:50:07 EDT
Some brief comments from reading through the spec follows. Some of it is duplicate of what is said on bug 603481, but some of it might explain something (not) there.

> License:        BSD and GPLv2+

The project as whole is GPLv2+ even though some parts "just" are GPL-compatible. (Just like glibc doesn't mention BSD despite /usr/share/doc/glibc-2.12/LICENSES.)

> %description
> FreeRDP is a X Remote Desktop Protocol Client for Windows Terminal Servers. 
> It is a fork of the rdesktop project that intends to rapidly start moving 
> forward and implement features that rdesktop lacks the most.

Actually, FreeRDP is the project, libfreerdp is the library, and xfreerdp is the X client. (I have tried to convince upstream to create a simple client on each platform and call it freerdp but haven't succeeded yet.) The way it is I think it would be least confusing if the package was called xfreerdp like the binary.

> BuildRequires:  directfb-devel

The directfb port is far from ready yet and shouldn't be packaged

> BuildRequires:  pcsc-lite-devel

configure is lying - that isn't implemented yet / any longer.

> BuildRequires:  libsamplerate-devel
> BuildRequires:  libao-devel

> # not enabled by default cause it does not yet work
> %{?_with_libvncserver:BuildRequires: libvncserver-devel}  

Also old cruft that doesn't apply

> Requires:       pcsc-lite

N/A

> %package        directfb

N/A

> %package        common
> Summary:        Common libraries, keymaps and plugins for %{name}
> Group:          System Environment/Libraries
>
> %description    common
> The %{name}-common package contains common libraries, keymaps and plugins for 
> %{name} and %{name}-directfb.

It is mostly the core freerdp lib, so shouldn't it be called something with lib instead?

I also find it confusing that the -devel package is for a -common package.

> %prep
> %setup -q
>
>
> %build
> %configure --enable-smartcard --with-dfb

N/A N/A

> --disable-static \

good idea

>     %{?_with_libvncserver:--with-libvncserver}
N/A

> 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

Are they really needed? AFAICS my packages without this hack don't have any issues.

> %install
> rm -rf $RPM_BUILD_ROOT
> make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p'

AFAIK -p isn't required by the guidelines. Just doing it on the packages where the maintainer cares seems a bit odd.

> %doc AUTHORS COPYING README doc/{ChangeLog,TODO,rdp-keyboard.odg,*.txt}

Most of these files are outdated and no longer applies
Comment 4 Christoph Wickert 2010-07-28 19:59:39 EDT
I have already answered to your comments in bug 603481, but just for the record. I'm leaving out the parts where I agree.

(In reply to comment #3)
> The project as whole is GPLv2+ even though some parts "just" are
> GPL-compatible. (Just like glibc doesn't mention BSD despite
> /usr/share/doc/glibc-2.12/LICENSES.)

I was under the impression that certain libs were BSD (only). As soon as something BSD licensed builds into a separate library or binary, BSD must be mentioned explicitly. AFAICS this is the case here.

> Actually, FreeRDP is the project, libfreerdp is the library, and xfreerdp is
> the X client. (I have tried to convince upstream to create a simple client on
> each platform and call it freerdp but haven't succeeded yet.) The way it is I
> think it would be least confusing if the package was called xfreerdp like the
> binary.

See my comment in bug 603481.

> It is mostly the core freerdp lib, so shouldn't it be called something with lib
> instead?

Not sure, it also includes the keyboard definitions.

> I also find it confusing that the -devel package is for a -common package.

Huh?
> > 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
> 
> Are they really needed? AFAICS my packages without this hack don't have any
> issues.

I'm getting rpaths when I build this locally, this doesn't happen in koji or mock tough.

> > make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p'
> 
> AFAIK -p isn't required by the guidelines. Just doing it on the packages where
> the maintainer cares seems a bit odd.

The guidelines mention to preserve timestamps. Things like the keyboard defintions that are not getting compiled should IMHO have the upstream timestamp.
Comment 5 Mads Kiilerich 2010-07-28 20:40:46 EDT
(In reply to comment #4)

I'm trying to extract as much knowledge and opinion from your packaging as possible ;-)

> I was under the impression that certain libs were BSD (only). As soon as
> something BSD licensed builds into a separate library or binary, BSD must be
> mentioned explicitly. AFAICS this is the case here.

Can you be more specific? Which library/binary, and where are the statements of BSD license?

> > It is mostly the core freerdp lib, so shouldn't it be called something with lib
> > instead?
> 
> Not sure, it also includes the keyboard definitions.

Yes, but that is data for libfreerdpkbd and not intended to be used or seen in other ways.

> > I also find it confusing that the -devel package is for a -common package.
> 
> Huh?

I would expect "foo-devel" to contain the headers etc for libraries in "foo". The devel stuff for "foo-common" should be in "foo-common-devel"?

> > > 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
> > 
> > Are they really needed? AFAICS my packages without this hack don't have any
> > issues.
> 
> I'm getting rpaths when I build this locally, this doesn't happen in koji or
> mock tough.

You don't know why?

I will investigate further.

> > > make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p'
> > 
> > AFAIK -p isn't required by the guidelines. Just doing it on the packages where
> > the maintainer cares seems a bit odd.
> 
> The guidelines mention to preserve timestamps. Things like the keyboard
> defintions that are not getting compiled should IMHO have the upstream
> timestamp.    

Yes, "consider" preserving timestamps. Something less vague could be nice. Now it is up to my preference to focus on simple spec and legibility or on preserving timestamps - that is bad for consistency ...
Comment 6 Christoph Wickert 2010-07-29 04:47:21 EDT
(In reply to comment #5)
> Can you be more specific? Which library/binary, and where are the statements of
> BSD license?

Sorry, it's actually MIT, to be precise https://fedoraproject.org/wiki/Licensing/MIT#Modern_Style_with_sublicense 
* in asn1 everything is BSD
* in channels/cliprdr everything is MIT
* in channels/common everything it MIT except chan_stream.{c,h} (GPLv2+)
* in channels/rdpdr everything is GPLv2+, except rdpdr_main.c (MIT)
* in channels/rdpsnd we have rdpsnd.h/rdpsnd_dsp.h (MIT) and rspsnd_alsa.c/rdpsnd_main.c (GPLv2+)
* in include/freerdp everything is MIT except kbd.h and rdpset.h (GPLv2+)
* in libfreerdp everything is GPLv2+ except frdp.h (MIT)
* libfreerdpchanman is MIT
* in libfreerdpkbd everything is GPLv2+
* in X11 everything is MIT

I think that xfreedrp is MIT, so if there is a separate package for xfreerdp, it should have MIT as license tag.
The rest is a mess, but I think it will come down to GPLv2+. The plugins are compiling against the things from common, right?

> I would expect "foo-devel" to contain the headers etc for libraries in "foo".
> The devel stuff for "foo-common" should be in "foo-common-devel"?

Now I see what you mean, but usually we only have one foo-devel package.

> > I'm getting rpaths when I build this locally, this doesn't happen in koji or
> > mock tough.
> 
> You don't know why?

I'm sorry, I don't know. But I'm having the same problem with other packages too.

> > The guidelines mention to preserve timestamps. Things like the keyboard
> > defintions that are not getting compiled should IMHO have the upstream
> > timestamp.    
> 
> Yes, "consider" preserving timestamps. Something less vague could be nice. Now
> it is up to my preference to focus on simple spec and legibility or on
> preserving timestamps - that is bad for consistency ...    

So how about using -p in general, then it's consistent again? ;) I wouldn't consider this a blocker during the review. Do as you like.
Comment 7 Mads Kiilerich 2010-08-10 19:34:37 EDT
(In reply to comment #6)
> > Can you be more specific? Which library/binary, and where are the statements of
> > BSD license?
> 
> Sorry, it's actually MIT, to be precise
> https://fedoraproject.org/wiki/Licensing/MIT#Modern_Style_with_sublicense 
> * in asn1 everything is BSD
> * in channels/cliprdr everything is MIT
> * in channels/common everything it MIT except chan_stream.{c,h} (GPLv2+)
> * in channels/rdpdr everything is GPLv2+, except rdpdr_main.c (MIT)
> * in channels/rdpsnd we have rdpsnd.h/rdpsnd_dsp.h (MIT) and
> rspsnd_alsa.c/rdpsnd_main.c (GPLv2+)
> * in include/freerdp everything is MIT except kbd.h and rdpset.h (GPLv2+)
> * in libfreerdp everything is GPLv2+ except frdp.h (MIT)
> * libfreerdpchanman is MIT
> * in libfreerdpkbd everything is GPLv2+
> * in X11 everything is MIT
> 
> I think that xfreedrp is MIT, so if there is a separate package for xfreerdp,
> it should have MIT as license tag.
> The rest is a mess, but I think it will come down to GPLv2+. The plugins are
> compiling against the things from common, right?

Yes, so everything is either BSD or MIT or GPLv2+, but the xfreerdp and all libs contains or links against GPLv2+ headers and libraries, so effectively everything is GPLv2+, right?

Note You need to log in before you can comment on or make changes to this bug.