Bug 182173 - Review Request: eterm - a color vt102 terminal emulator
Summary: Review Request: eterm - a color vt102 terminal emulator
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ed Hill
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 182175
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-02-20 21:19 UTC by Terje Røsten
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-11-26 15:50:34 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Terje Røsten 2006-02-20 21:19:44 UTC
Spec file: http://web.phys.ntnu.no/~terjeros/eterm/eterm.spec
SRPM : http://web.phys.ntnu.no/~terjeros/eterm/eterm-0.9.3-1.src.rpm
Description: Eterm is a color vt102 terminal emulator intended as a replacement for xterm.

Depends on libast which is also submitted.

Comment 1 Ed Hill 2006-03-05 22:04:52 UTC
Hi Terje, this is not a full review, just a few observations:

good:
 + source matches upstream

needswork:
 - BuildRoot should be:
   %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 - rpmlint reports:
   E: eterm explicit-lib-dependency libast
   E: eterm binary-or-shlib-defines-rpath 
      /usr/bin/Esetroot ['/usr/lib', '/usr/lib/Eterm']
   E: eterm binary-or-shlib-defines-rpath 
      /usr/bin/Eterm ['/usr/lib', '/usr/lib/Eterm']

amusing:
 + from the ./configure output:
     checking for life_signs in -lKenny... no
       Oh my god, they killed Kenny!  You bastards!

probably not acceptable in FC or FE:
 - from the Eterm-0.9.3/src/command.c file:

    * Copyright 1992 John Bovey, University of Kent at Canterbury.
    *
    * You can do what you like with this source code as long as
    * you don't try to make money out of it and you include an
    * unaltered copy of this message (including the copyright).

Please contact upstream and inquire about the overall license.  The vast
majority of the code is BSD-style (per Michael Jennings) but the license on 
the command.c file explicitly forbids "making money" and AFAICT this is not 
acceptable for FC or FE (that is, folks should be allowed to sell copies of 
Fedora-packaged software if they desire).

Comment 2 Terje Røsten 2006-03-06 15:51:31 UTC
>  - BuildRoot should be:
>    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Fixed.

>    E: eterm explicit-lib-dependency libast

Fixed.

>    E: eterm binary-or-shlib-defines-rpath 
>       /usr/bin/Esetroot ['/usr/lib', '/usr/lib/Eterm']
>    E: eterm binary-or-shlib-defines-rpath 
>       /usr/bin/Eterm ['/usr/lib', '/usr/lib/Eterm']

Is this a showstopper? Don't know how to fix this, any ideas?
 
> Please contact upstream and inquire about the overall license.  The vast
> majority of the code is BSD-style (per Michael Jennings) but the license on 
> the command.c file explicitly forbids "making money" and AFAICT this is not 
> acceptable for FC or FE (that is, folks should be allowed to sell copies of 
> Fedora-packaged software if they desire).

Ok, I sent John a email, here is his respons:
 
   The code must originate from xvt, a lightweight replacement for 
   xterm that I wrote many years ago. It was used by Rob Nation as
   the basis for rxvt and it looks like it has found its way into 
   other terminal emulators. The comment about 'not making any money'
   was the original license but xvt has also been released in debian
   with a gpl license.

   I am happy for any terminal emulator code of mine to be on a gpl
   license.


Updated package available here:

Spec:
 http://web.phys.ntnu.no/~terjeros/eterm/eterm.spec
SRPM:
 http://web.phys.ntnu.no/~terjeros/eterm/eterm-0.9.3-2.src.rpm


Comment 3 Ed Hill 2006-03-06 19:24:11 UTC
Hi Terje, I think the rpath error is a blocker (can anyone else comment 
here?) and will look into it some more.  Also, I'll do a thorough review 
as soon as I can.

Please try to get the license issues sorted out *within* the upstream 
source since they are a blocker.   If the upstream maintainers clarify 
things (say, if they put a single COPYING or LICENSE file that makes it 
clear what the overall terms with--with *no* inconsistencies in the 
individual files), that would be ideal.

Comment 4 Terje Røsten 2006-03-16 18:07:36 UTC
I sent a mail to Michael some time ago, however no feedback yet.

You have seen the comments on #182175?

Uhm, seems like this is a mess:

src/command.{c,h}: Copyright John Bovey, not make money or GPL.
src/grkelot.{c,h}: Copyright Angelo Haritsis, unknown.
src/libscream.c  : Copyright Azundris, LGPL.
src/netdisp.c    : Copyright Chuck Blake/mj olesen, not make money thingie.
src/{scream.h,screancfg.h},     : Azundris, BSD.
src/screen.h     : Robert Nation and mj olesen, ?.

the rest : Copyright Michael Jennings, BSD(?).

BTW: I think Eterm was shipped in RHL 9 and is included in Debian 3.1.


Comment 5 Ed Hill 2006-03-16 19:07:31 UTC
Hi Terje, I'm not sure how to proceed here.  It looks like parts of the 
source are licensed under conflicting terms.  So, what to do?  I think we 
should ask for feedback and will send an email today...


Comment 6 Ed Hill 2006-03-17 19:39:22 UTC
Hi Terje, I've contacted the debian eterm maintainer to point out the 
license mess within the source files (as was suggested on the fedora-
extras-list).  I'll leave this bug open for a couple weeks in the hope 
that upstream will respond to you and will sort it out (the files that 
have license problems are a fraction of the overall code so perhaps 
they can be removed or replaced?).  If not, the package will be rejected.

Not much else that we can do here...!

Comment 7 Terje Røsten 2006-04-22 17:12:26 UTC
Ok, I give up for now.

No Eterm package in Extras, sorry.


Comment 8 Ed Hill 2006-04-23 03:05:18 UTC
Hi Terje,

Its not at all your fault.  Thank you for trying to package it.

Its a shame that upstream could not or would not sort out the licenses. 
I liked Eterm and used it and Enlightenment for a few years.  When I 
started, I had no idea this would be the outcome of this review.  Its 
not what I wanted.  Bummer.

Comment 9 Michael Jennings (KainX) 2006-08-23 18:57:46 UTC
(In reply to comment #3)
> Hi Terje, I think the rpath error is a blocker (can anyone else comment 
> here?)

That is one of the "stupid policies" I mentioned in the other bug.  There is
nothing inherently wrong with rpath, and blindly rejecting packages because of
it is ridiculous.  Particularly since the paths Eterm uses are NOT WRITEABLE.

The "fix" (I use the term loosely) is to edit Makefile.am to remove the -rpath
parameter.

> Please try to get the license issues sorted out *within* the upstream 
> source since they are a blocker.   If the upstream maintainers clarify 
> things (say, if they put a single COPYING or LICENSE file that makes it 
> clear what the overall terms with--with *no* inconsistencies in the 
> individual files), that would be ideal.

Eterm 0.9.4, which has just been released, has the appropriate LICENSE file to
clarify the situation.

(In reply to comment #4)
> I sent a mail to Michael some time ago, however no feedback yet.

Had I *actually* been contacted about this, I would've taken action sooner.  As
it is, no one who has posted on this bug contacted me about it, nor did the
Debian maintainer or any debian developer.  It wasn't until a USER named Nolius
dropped me an e-mail with a link to the eterm package news page that I heard of
this issue.

Shame on you both.

(In reply to comment #8)
> Its a shame that upstream could not or would not sort out the licenses. 

I did, and I would've done it sooner had either of you e-mailed me directly
about it.

I was e-mailed about the other bug, WRT LibAST, but not Eterm.


Comment 10 Terje Røsten 2006-09-02 16:42:22 UTC
New package based on 0.9.4 with

 o rpath problem fixed
 o LICENSE taken from upstream package

SPEC: http://web.phys.ntnu.no/~terjeros/eterm/eterm.spec
SRPM: http://web.phys.ntnu.no/~terjeros/eterm/eterm-0.9.4-1.src.rpm

Comment 11 Ed Hill 2006-10-02 03:22:01 UTC
Hi Terje, the license issue does appear to be cleaned up.  The
no-money one is gone but the LGPL-ed bits remain.  I'm no lawyer
but it seems OK to link together the LGPL-ed parts with BSD code.

There are some suspicious bits such as:

 1) literally hundreds of "pointer targets ... differ in signedness"
    warnings which are worrisome but perhaps ignorable

 2) there appear to be some missing BuildRequires and/or some missing
    functionality such as:

      checking for Etwin support...
        checking for Tw_Open in -lTw... no
      configure: WARNING: *** Twin support has been
        disabled because libTw was not found ***

    and I think libXmu-devel needs to be a BR since I don't see how it
    gets pulled in by any of the other BRs.  Please take a look.

In any case, the remaining review items are:

 + source matches upstream
 + license now appears to be OK and is correctly included
 + builds on FC5 i386
 + rpmlint reports no errors or warnings
 + package and spec naming OK
 + spec is legible
 + builds on FC5 i386
 + no locales
 + shared libs OK
 + not relocatable
 + dir ownership looks good
 + no file dupes
 + permissions OK
 + has %clean
 + consistent use of macros
 + code not content (although there are a number of background
   pixmaps that could be split off into a separate package if
   one desires)
 + docs are small and not needed for execution
 + no static, *.la, or devel libs
 + no headers or pkgconfig
 + has desktop file with desktop-file-install which appears sane

It'll be easy enough to sort out the BuildRequires with mock as soon
as libast is in Extras so we can leave that for later.

And I don't see any remaining blockers so its APPROVED.



Comment 12 Ed Hill 2006-10-14 16:30:17 UTC
Hi Terje, are you still interested in this package?

Comment 13 Terje Røsten 2006-11-24 20:23:54 UTC
>  1) literally hundreds of "pointer targets ... differ in signedness"
>     warnings which are worrisome but perhaps ignorable

I have used Eterm for years without problems...
 
>  2) there appear to be some missing BuildRequires and/or some missing
>     functionality such as:
> 
>       checking for Etwin support...
>         checking for Tw_Open in -lTw... no
>       configure: WARNING: *** Twin support has been
>         disabled because libTw was not found ***

I believe this is support for a Twin - a textmode window environment,
(http://twin.sf.net), this issue cannot be a blocker.
Warning is gone with --disable-etwin.
 
>     and I think libXmu-devel needs to be a BR since I don't see how it
>     gets pulled in by any of the other BRs.  Please take a look.

Yeah, added pcre-devel too. And fixed the desktop file.

> It'll be easy enough to sort out the BuildRequires with mock as soon
> as libast is in Extras so we can leave that for later.

libast is in FE now :-) 

New files:
SPEC: http://web.phys.ntnu.no/~terjeros/eterm/eterm.spec
SRPMS: http://web.phys.ntnu.no/~terjeros/eterm/eterm-0.9.4-2.fc6.src.rpm



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