Bug 448337 - Review Request: enlightenment - a next generation desktop shell
Review Request: enlightenment - a next generation desktop shell
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-25 20:24 EDT by Pavel Shevchuk
Modified: 2008-09-01 18:43 EDT (History)
5 users (show)

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


Attachments (Terms of Use)

  None (edit)
Description Pavel Shevchuk 2008-05-25 20:24:06 EDT
Spec URL: http://rpm.scwlab.com/e-for-rawhide/enlightenment.spec
SRPM URL: http://rpm.scwlab.com/e-for-rawhide/enlightenment-0.16.999.043-1.fc9.src.rpm

Description:
Enlightenment 0.17 is the next generation of UNIX graphical environments. It is
not just a window manager, but it is also a desktop shell. A desktop shell
means, a window manager plus a file manager, plus configuration utilitys all in
one. They are not separated as usual.

Vera font is being stripped out because:
1) It's original bitstream version, it doesn't include national symbols
2) Its license is different from E license and i don't want License field to exceed 80 chars :)
3) Bundling font is just stupid.

I moved bit of documentation from /usr/share/enlightenment/doc to /usr/share/doc/enlightenment-devel because it's better place for it.

rpmlint rants about two suid binaries. One of them is sudo-like thing used by e to shutdown/reboot, other is cpufreq plugin. sudo-thing could be replaced with proper consolekit/policykit integration, i added this to my TODO.

<emo mode>This sole package was reason for me to spend 2 weeks on learning rpm, become sponsored packager and then spend another month on packaging needed E libraries. I already received few emails from people anticipating E inclusion in fedora.</emo mode>
Comment 1 Dr. Gregory R. Kriehn 2008-05-25 21:17:55 EDT
Have you considered enabling additional functionality, such as using pam,
gettext, curl, openssl, SDL, libXcursor, libXdamage, libXcomposite, etc?

Please see the following spec file as an example:

SPEC:  http://optics.csufresno.edu/fedora/extras/9/SPECS/enlightenment.spec

There is a lot of functionality that you have disabled by default, but would be
easy to include, and I believe should be.  Comments?
Comment 2 Pavel Shevchuk 2008-05-25 21:29:06 EDT
I guess i have everything enabled already. 

Locale change works, curl and openssl are enabled in ecore, sdl packend need is 
questionable (but i may enable it in future), cursors get themed, composite 
radiobutton is available, screen lock works.
Comment 3 Mamoru TASAKA 2008-07-02 13:56:44 EDT
For 0.16.999.043-1:

! Question
  - Would you explain what .edj files are? Can this be freely
    (re)distributable?

* Multiple issues on -devel subpackage
  ** Requires
     - efeet-devel is also needed for Requires of -devel subpackage
       (see e.h)

  ** pkgconfig file
--------------------------------------------------------------
$ pkg-config --libs enlightenment
/usr/lib/enlightenment/modules  
--------------------------------------------------------------
     - This is perhaps wrong (pkg-config --libs foo should usually
       return -Ldir -lfoo -baa .....)

  ** "config.h" inclusion
     - From %_includedir/%name/e.h:
--------------------------------------------------------------
     4  #ifndef E_H
     5  #define E_H
     6  
     7  #include "config.h"
     8  
--------------------------------------------------------------
       but 1. config.h is not installed and 2. config.h must not be
       needed for installed header files.

* %prep
  - Perhaps the following is simpler:
---------------------------------------------------------------
%setup -q
# strip out bundled vera font, it doesn't support national glyphs
rm -r data/fonts
sed -i \
	-e '\|CONFIG_FILES="\$CONFIG_FILES data/fonts/Makefile"|d' \
	-e 's|data/fonts/Makefile||' \
	configure ; chmod +x configure
sed -i -e 's|fonts||' data/Makefile.in
sed -i -e '\|\.ttf|d' data/{init,themes}/default.edc
---------------------------------------------------------------

* Duplicate files
  - Please check if the following files are really needed.
    They are also installed as %doc.
---------------------------------------------------------------
/usr/share/enlightenment/AUTHORS
/usr/share/enlightenment/COPYING
---------------------------------------------------------------

* Directory ownership issue
  - Please make it sure that all directories created when installing
    this package are correctly owned by this package.
    Example:
---------------------------------------------------------------
[tasaka1@localhost ~]$ rpm -qf /usr/lib/enlightenment/preload/e_precache.so 
enlightenment-0.16.999.043-1.fc10.i386
[tasaka1@localhost ~]$ rpm -qf /usr/lib/enlightenment/preload/              
file /usr/lib/enlightenment/preload is not owned by any package
[tasaka1@localhost ~]$ rpm -qf /usr/lib/enlightenment/        
file /usr/lib/enlightenment is not owned by any package
---------------------------------------------------------------

* setuid files
  - Please explain why two binaries must have setuid bits.
    Also please consider if pam mechanism or so can replace this.
Comment 4 Mamoru TASAKA 2008-07-09 09:37:39 EDT
ping?
Comment 5 Pavel Shevchuk 2008-07-09 13:26:23 EDT
Pong. Sorry, i'm very busy at work now. I will fix pkg this weekend. 
Comment 6 Pavel Shevchuk 2008-07-26 12:18:43 EDT
* Sat Jul 26 2008 Pavel "Stalwart" Shevchuk <stlwrt@gmail.com> - 0.16.999.043-2
- Added missing efreet-devel require to enlightenment-devel
- Removed broken enlightenment_sys
- Removed more improperly placed docs
- Fixed directory ownership

Spec: http://rpm.scwlab.com/e-for-rawhide/2/enlightenment.spec
SRPM: http://rpm.scwlab.com/e-for-rawhide/2/
enlightenment-0.16.999.043-2.fc10.src.rpm
RPMs for rawhide-x86_64: http://rpm.scwlab.com/e-for-rawhide/2/

>Would you explain what .edj files are? Can this be freely (re)distributable?
.edj are enlightened skins, which consist of images and some bytecode. They're 
generated from stuff in source tarball which has same license as C code

>efeet-devel is also needed for Requires of -devel subpackage (see e.h)
fixed

>pkgconfig file
is like this upstream and it somehow works when building additional modules, 
will report to developers

>"config.h" inclusion
no idea, will also be reported

>%prep - Perhaps the following is simpler
thanks, replaced my lame code with yours proper

>Duplicate files
fixed

>Directory ownership issue
fixed

>Please explain why two binaries must have setuid bits.
cpufreq module changes governors directly, not depending on bulky power 
management daemons like gnome/kde
removed enlightenment_sys bits which don't seem to work (deprecated?)
Comment 7 Mamoru TASAKA 2008-07-29 15:05:12 EDT
Sorry for late reply..

For -2:
* Directory ownership issue
  - %{_includedir}/%{name}/ is not yet owned by any packages.

* pkgconfig issue
  - For now would you fix enlightenment.pc.in like following?
-------------------------------------------------------
.....
Version: @VERSION@
Libs: -L$libdir
Libs.private: 
.....
-------------------------------------------------------

* config.h inclusion
  - This issue must be fixed.
    A quick workaround is to include config.h as unique and arch-dependent 
    name and modify e.h so that e.h includes such arch-dependent header file.
    ref:
   
https://fedoraproject.org/wiki/PackagingDrafts/MultilibTricks#myautoconf.h_files_with_a_size_in_them
    Also see %_includedir/openssl/opensslconf.h in openssl-devel.

    So a example of quick workaround follows:
    1. Modify e.h (in the source tarball it is src/bin/e.h) as:
--------------------------------------------------------
/*
 * vim:ts=8:sw=3:sts=8:noexpandtab:cino=>5n-3f0^-2{2
 */
#ifndef E_H
#define E_H

#include "e_config_arch.h"

#define USE_IPC
#if 0
.........
--------------------------------------------------------
    2. Prepare e_config_arch.h (as %SOURCE? , say %SOURCE1) as
--------------------------------------------------------
#ifndef E_CONFIG_ARCH_H
#define E_CONFIG_ARCH_H

#if defined(__i386__)
#include "e_config-i386.h"
#elif defined(__ia64__)
#include "e_config-ia64.h"
#elif defined(__powerpc64__)
#include "e_config-ppc64.h"
#elif defined(__powerpc__)
#include "e_config-ppc.h"
#elif defined(__s390x__)
#include "e_config-s390x.h"
#elif defined(__s390__)
#include "e_config-s390.h"
#elif defined(__sparc__) && defined(__arch64__)
#include "e_config-sparc64.h"
#elif defined(__sparc__)
#include "e_config-sparc.h"
#elif defined(__x86_64__)
#include "e_config-x86_64.h"
#else
#error "This enlightment header files do not work your architecture?"
#endif

#endif
--------------------------------------------------------
   3. Then:
--------------------------------------------------------
%prep
%setup -q
%patch.....
install %SOURCE1 src/bin/
.....
.....
%build
%configure --disable-static
cp -p config.h e_config-%{_arch}.h
make %{?_smp_mflags}

%install
....
make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p'
......
install -cpm 644 %SOURCE0 e_config-%{_arch}.h %{_includedir}/%{name}/

%files
......
--------------------------------------------------------

* multilib conflict
  - And some reviewers say that shell scripts in foo-devel package should
    not differ between different architectures (see:
    https://fedoraproject.org/wiki/PackagingDrafts/MultilibTricks )

    Would you _consider_ to fix %name-config.in (in src tarball) like below?
    ! Note this is _optional_ and _not mandatory_
---------------------------------------------------------
#!/bin/sh

prefix=@prefix@
exec_prefix=@exec_prefix@
exec_prefix_set=no
libdir=/usr/lib

if [ -z $ARCH ] ; then ARCH=$(uname -m) ; fi
case $ARCH in
	x86_64 | ia64 | s390 )
		libdir=/usr/lib64
		;;
esac

usage="\
.....
.....
    --libs)
      libdirs="-L${libdir}"
      echo $libdirs
      ;;
......
(and same below)
---------------------------------------------------------
Comment 8 Mamoru TASAKA 2008-08-08 11:33:10 EDT
ping?
Comment 9 Pavel Shevchuk 2008-08-08 11:52:57 EDT
Will fix on this weekend
Comment 10 Mamoru TASAKA 2008-08-14 10:09:54 EDT
ping again?
Comment 11 Mamoru TASAKA 2008-08-25 12:07:16 EDT
ping again?
Comment 12 Pavel Shevchuk 2008-08-31 18:26:10 EDT
Sorry for huge delay =(

* Sat Aug 30 2008 Pavel "Stalwart" Shevchuk <stlwrt@gmail.com> - 0.16.999.043-3
- One more directory ownership fix
- Backported enlightenment.pc.in from trunk
- Strip unneeded config.h include from e.h

Spec: http://rpm.scwlab.com/fedora/e17/9/i386/enlightenment-0.16.999.043-3/enlightenment.spec
SRPM: http://rpm.scwlab.com/fedora/e17/9/i386/enlightenment-0.16.999.043-3/enlightenment-0.16.999.043-3.fc9.src.rpm
YUM repository for f9, i386 and x86_64:
[scwlab-f-e17]
name=Stalwart Fedora - E17
baseurl=http://rpm.scwlab.com/fedora/e17/$releasever/$basearch
gpgcheck=0
enabled=1


-------
I hacked up patch for config.h just to realize that config.h isn't actually needed to build stuff that depends on e17. =( So i just strip config.h out of e.h now.
I'm not sure deriving from upstream enlightenment-config is really needed, so i didn't in -3. If you insist, i could do -4
Comment 13 Mamoru TASAKA 2008-09-01 10:46:35 EDT
Okay.

-----------------------------------------------------------------------
       This package (enlightenment) is APPROVED by mtasaka
-----------------------------------------------------------------------
Comment 14 Pavel Shevchuk 2008-09-01 15:28:15 EDT
New Package CVS Request
=======================
Package Name: enlightenment
Short Description: A next generation desktop shell
Owners: stalwart
Branches: F-8 F-9
InitialCC:
Cvsextras Commits: yes
Comment 15 Kevin Fenzi 2008-09-01 17:13:45 EDT
cvs done.
Comment 16 Pavel Shevchuk 2008-09-01 18:43:58 EDT
Kudos to Kevin, arigatos to Mamoru. E17 is now in rawhide ;)

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