Bug 254056 - Review Request: e16 - The Enlightenment window manager, DR16
Review Request: e16 - The Enlightenment window manager, DR16
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 254057 254058 254060
  Show dependency treegraph
 
Reported: 2007-08-23 16:34 EDT by Terje Røsten
Modified: 2008-03-27 14:38 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-27 14:38:58 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Terje Røsten 2007-08-23 16:34:45 EDT
Spec URL: http://terjeros.fedorapeople.org/e16/e16.spec
SRPM URL: http://terjeros.fedorapeople.org/e16/e16-0.16.8.9-1.fc8.src.rpm
Description:
Enlightenment is a window manager for the X Window System that
is designed to be powerful, extensible, configurable and
pretty darned good looking! It is one of the more graphically
intense window managers.

Enlightenment goes beyond managing windows by providing a useful
and appealing graphical shell from which to work. It is open
in design and instead of dictating a policy, allows the user to 
define their own policy, down to every last detail.

This package will install the Enlightenment window manager.
Comment 1 Kevin Fenzi 2007-09-13 00:15:27 EDT
I'd be happy to review this package. Look for a full review in a bit. 
Comment 2 Kevin Fenzi 2007-09-13 00:55:32 EDT
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
See below - License
See below - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
8d27553ae9c582a9d331ea4077063a14  e16-0.16.8.9.tar.gz
8d27553ae9c582a9d331ea4077063a14  e16-0.16.8.9.tar.gz.1
See below - BuildRequires correct
OK - Spec handles locales/find_lang
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Doc subpackage needed/used.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should have dist tag
OK - Should package latest version

Issues:

1. Some of the source files appear to be GPLv2+
epp/cpperror.c
epp/cppalloc.c
epp/cppexp.c
epp/cpphash.c
epp/cpplib.c
epp/cpplib.h
epp/cppmain.c

From a quick look, those source files all compile to the epp binary.

The rest are BSDish, but not matching exactly any of the examples on the wiki.
I am a bit concerned with the second paragraph:

"  The above copyright notice and this permission notice shall be included in
  all copies of the Software, its documentation and marketing & publicity
  materials, and acknowledgment shall be given in the documentation, materials
  and software packages that this Software was used."

In addition there is a copy of the Bitsteam Vera Fonts with their own license
inside the "winter.etheme" tar.gz thats in the e16 tar.gz. Can that be made
somehow to use the already existing Vera package?

I will have spot check it over...

2. Some possible missing BuildRequires:

checking X11/SM/SMlib.h usability... no
checking X11/SM/SMlib.h presence... no
checking for X11/SM/SMlib.h... no
checking for SmcOpenConnection in -lSM... no
checking for XFT... no
checking for XineramaQueryExtension in -lXinerama... no
configure: WARNING: Xinerama support was requested but not found
checking for XF86VidModeQueryExtension in -lXxf86vm... no
configure: WARNING: Zoom support was requested but not found
checking for XRRQueryExtension in -lXrandr... no
configure: WARNING: RandR support was requested but not found
checking for X11/extensions/Xrandr.h... no
checking for XCompositeQueryExtension in -lXcomposite... no
checking for X11/extensions/Xcomposite.h... no
checking for X11/extensions/Xdamage.h... no
checking for X11/extensions/Xfixes.h... no
checking for X11/extensions/Xrender.h... no
configure: WARNING: Composite support was requested but required component was
not found
checking for mass_quantities_of_bass_ale in -lFridge... no
checking for mass_quantities_of_any_ale in -lFridge... no
Warning: No ales were found in your refrigerator.
  We highly suggest that you rectify this situation immediately.

3. rpmlint says:

e16.i386: E: zero-length /usr/share/e16/themes/winter/slideouts/slideouts.cfg
e16.i386: E: zero-length /usr/share/e16/themes/winter/buttons/buttons.cfg

Not sure if those can be removed, or if they are needed by that theme...

e16.i386: E: invalid-soname /usr/lib/libe16_hack.so libe16_hack.so

Does that so file need to be in /usr/lib?

e16.i386: W: file-not-utf8 /usr/share/doc/e16-0.16.8.9/ChangeLog
e16.i386: W: file-not-utf8 /usr/share/doc/e16-0.16.8.9/AUTHORS

Might run 'iconv' on those?
Comment 3 Terje Røsten 2007-09-17 12:48:57 EDT
> 1. Some of the source files appear to be GPLv2+

GPLv2+ added to license.

> In addition there is a copy of the Bitsteam Vera Fonts with their own license
> inside the "winter.etheme" tar.gz thats in the e16 tar.gz. Can that be made
> somehow to use the already existing Vera package?

Removed ttfonts dir, created a symlink and added bitstream-vera 
to req, seems to work.

 
> I will have spot check it over...

Ok, thanks.


> 2. Some possible missing BuildRequires:

Tried to add some X related devel packages, better now?
 
> 3. rpmlint says:
> 
> e16.i386: E: zero-length /usr/share/e16/themes/winter/slideouts/slideouts.cfg
> e16.i386: E: zero-length /usr/share/e16/themes/winter/buttons/buttons.cfg
> 
> Not sure if those can be removed, or if they are needed by that theme...

Tried that, not a crash, but a freeze, better leave these.
 
> e16.i386: E: invalid-soname /usr/lib/libe16_hack.so libe16_hack.so
> 
> Does that so file need to be in /usr/lib?

Don't know, however e16 seems to be happy without this lib, maybe
needed by the epplets?
 
> e16.i386: W: file-not-utf8 /usr/share/doc/e16-0.16.8.9/ChangeLog
> e16.i386: W: file-not-utf8 /usr/share/doc/e16-0.16.8.9/AUTHORS
>
> Might run 'iconv' on those?

Fixed.

SPEC: http://terjeros.fedorapeople.org/e16/e16.spec
SRPM: http://terjeros.fedorapeople.org/e16/e16-0.16.8.9-2.fc7.src.rpm

Comment 4 Terje Røsten 2007-09-17 13:04:11 EDT
> Tried to add some X related devel packages, better now?

Added some more, did not bump release:

SPEC: http://terjeros.fedorapeople.org/e16/e16.spec
SRPM: http://terjeros.fedorapeople.org/e16/e16-0.16.8.9-2.fc7.src.rpm


Comment 5 Tom "spot" Callaway 2007-09-17 13:14:13 EDT
A license with virtually the same text is pending review at the FSF right now,
so I'm slapping FE-Legal on this one until it is resolved... sorry.
Comment 6 Terje Røsten 2007-09-17 13:27:11 EDT
> A license with virtually the same text is pending review at the FSF right now,
> so I'm slapping FE-Legal on this one until it is resolved... sorry.

Oki, thanks for the help so far.
Comment 7 Terje Røsten 2007-10-19 16:52:25 EDT
New upstream version (which solves the libhach issue):

- 0.16.8.10
- libhack has moved to e16 subdir

Updated package here:

SPEC: http://terjeros.fedorapeople.orgke16/e16.spec
SRPM: http://terjeros.fedorapeople.org/e16/e16-0.16.8.10-1.fc7.src.rpm

BTW: any progress on the license issue?

Comment 8 Terje Røsten 2007-10-19 17:01:01 EDT
> SPEC: http://terjeros.fedorapeople.orgke16/e16.spec

Sorry, it's
  
  SPEC: http://terjeros.fedorapeople.org/e16/e16.spec
Comment 9 Kevin Fenzi 2007-11-30 23:14:03 EST
Spot: Any word back on the license here?
Comment 10 Rudolf Kastl 2008-03-22 20:19:10 EDT
ping. there hasnt been any activity since months now.
Comment 11 Kevin Fenzi 2008-03-22 20:56:53 EDT
It's still pending legal review from the FSF. 

Hopefully they will get around to reviewing it soon... 
Comment 12 Tom "spot" Callaway 2008-03-23 10:13:22 EDT
OK, e16 license came back as free but GPL-incompatible (v2 and v3), due to the
fact that notices and an acknowledgement must appear in "marketing and publicity
materials", which puts it in the BSD-advertising-clause category.

Since the epp bits compile into their own, independent binary, and there is no
source code linking or sharing, this is fine to distribute as is. Please use:

License: BSD with advertising and GPLv2+

It is worth noting that some of the e16 upstream components have fixed this
problem in their license:

(http://www.enlightenment.org/viewvc/misc/geist/COPYING?r1=1.2&r2=1.3&pathrev=HEAD&view=patch)

I would strongly suggest reaching out to the copyright holders everytime we see
this license and asking them if they're willing to make the same license
changes. While its not a problem for this specific package, it could easily make
other works which have GPL and e16 code linked together, totally undistributable
by Fedora (or anyone, really).

Lifting FE-Legal.
Comment 13 Terje Røsten 2008-03-23 16:37:07 EDT
Great news spot, thanks!

New package ready:
- 0.16.8.12
- fix license (thanks spot and kevin!)
- enable dbus and pango
- simplity setup and modify desc
- try to preserve timestamps

spec: http://terjeros.fedorapeople.org/e16/e16.spec
srpm: http://terjeros.fedorapeople.org/e16/e16-0.16.8.12-1.fc8.src.rpm

Comment 14 Tom "spot" Callaway 2008-03-24 17:12:43 EDT
Actually, after more thought, I want the License to be:

License: MIT with advertising and GPLv2+

Sorry for the confusion. This is what I get for doing Licensing after midnight. :)
Comment 16 Kevin Fenzi 2008-03-25 19:39:01 EDT
ok, first off... per spot's comment, would you be willing to ping upstream about
changing this license? Not a blocker, but it would be nice to fix it up moving
forward if possible. ;) 

rpmlint now says: 

e16.i386: E: zero-length /usr/share/e16/themes/winter/slideouts/slideouts.cfg
e16.i386: E: zero-length /usr/share/e16/themes/winter/buttons/buttons.cfg

Can leave those as we mentioned before... 

e16.i386: W: dangling-relative-symlink /usr/share/e16/themes/winter/ttfonts
../../../fonts/bitstream-vera

You sure that link is right? I think it might need another ../ there?

e16.i386: W: invalid-license MIT with advertising
e16.src: W: invalid-license MIT with advertising
e16-debuginfo.i386: W: invalid-license MIT with advertising

Can be ignored. 

The build options look ok for the most part, but I see: 

Experimental options - DO NOT USE unless you know what you are doing
  Compile with ecore/ecore_x ... no
  GLX .......................... no
  ScreenSaver .................. no
  D-Bus ........................ yes

Is D-Bus stable enough to use? If so, how about the others? 
Surely not a blocker. 

The only thing I see off hand to doublecheck is the link to the font. 
Can you check that and make sure it's right before checkin?

Otherwise, this package looks ok, and is APPROVED. 
Comment 17 Terje Røsten 2008-03-26 17:42:41 EDT
(In reply to comment #16)
> ok, first off... per spot's comment, would you be willing to ping upstream
> about changing this license? Not a blocker, but it would be nice 
> to fix it up moving forward if possible. ;) 

Will send a mail to Kim.
 
> 
> e16.i386: W: dangling-relative-symlink /usr/share/e16/themes/winter/ttfonts
> ../../../fonts/bitstream-vera
> 
> You sure that link is right? I think it might need another ../ there?

It's correct (from a live system):

$ namei /usr/share/e16/themes/winter/ttfonts
f: /usr/share/e16/themes/winter/ttfonts
 d /
 d usr
 d share
 d e16
 d themes
 d winter
 l ttfonts -> ../../../fonts/bitstream-vera
   d ..
   d ..
   d ..
   d fonts
   d bitstream-vera
> The build options look ok for the most part, but I see: 
> 
> Experimental options - DO NOT USE unless you know what you are doing
>   Compile with ecore/ecore_x ... no
>   GLX .......................... no
>   ScreenSaver .................. no
>   D-Bus ........................ yes
> 
> Is D-Bus stable enough to use? If so, how about the others? 
> Surely not a blocker. 

DBUS is not listed as experimental in "configure --help"  while the others are,
uhm...

 
> The only thing I see off hand to doublecheck is the link to the font. 
> Can you check that and make sure it's right before checkin?

Yes,
 
> Otherwise, this package looks ok, and is APPROVED. 

Thanks, Kevin!



New Package CVS Request
=======================
Package Name: e16
Short Description: The Enlightenment window manager, DR16
Owners: terjeros
Branches: F-7 F-8 F-9
InitialCC:
Cvsextras Commits: yes



Comment 18 Kevin Fenzi 2008-03-26 22:58:39 EDT
>DBUS is not listed as experimental in "configure --help"  while the others are,
>uhm...

Weird. Might just be a bug in the build/configure output... 
might mention that upstream as well?

cvs done.
Comment 19 Terje Røsten 2008-03-27 14:38:58 EDT
(In reply to comment #18)
> Weird. Might just be a bug in the build/configure output... 
> might mention that upstream as well?

dbus is experimental (info from upstream),
 
> cvs done.

Thanks. 

Now imported.


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