This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 508483 - Review Request: ewl - Enlightenment Widget Library
Review Request: ewl - Enlightenment Widget Library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christian Krause
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-27 14:57 EDT by John Guthrie
Modified: 2009-07-08 21:36 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-08 21:36:14 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
chkr: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description John Guthrie 2009-06-27 14:57:22 EDT
Spec URL: http://www.guthrie.info/RPMS/f11/ewl-0.5.2.042-5.fc11.src.rpm
SRPM URL: http://www.guthrie.info/RPMS/f11/ewl.spec
Description: 
The Enlightenment Widget Library (EWL) is a high level toolkit providing all of
the widgets you'll need to create an enlightment application. The expansive
object oriented style API provides tools to easily expand widgets and containers
for new situations.

Among the wide variety of features EWL provides are:

    * Object, Widget and Container abstraction layers;
    * A variety of Containers for laying out widgets in arrangements such as
      boxes, tables and lists;
    * Simple widgets such as Buttons, Labels, Images and Progressbars;
    * Decorative Containers for wrapping borders and controls around widgets;
    * High level data abstractions including lists, expandable trees and combo
      boxes;
    * An extraordinarily flexible theming system;
    * High level abstractions to build applications quickly, such as file and
      color dialogs, as well as a menu system;
    * A flexible event system to allow application programmers to hook into
      nearly every change that occurs;
    * Abstracted EWL Engine backends allow for easily re-using portions of
      engines to support new platforms.
    * IO abstraction manager to enable mapping of mimetypes to widget
      representations;
    * EWL Test, a tutorial and testing application.
Comment 1 Christian Krause 2009-07-03 05:23:49 EDT
Hi John,

I was going to do a full review, however I've stumbled upon some major issues. Please can you solve them first before I do the complete review?

- the package contains a rather old snaphost from 1/2008 - can you package a newer snapshot e.g. from 6/2009?

- rpmlint reveals 139 warnings and errors, especially:

* formatting fo the description

* lots of example *.c files in the main package (they should be in the devel package)

* static libraries were packaged - usually they should be completely omitted unless there is a reason: http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries

* if the %post/%postun section only contains one line, it should be written like this:
%post -p /sbin/ldconfig

- using %ghost for *.so.x should not be necessary, 
%{_libdir}/*.so.* should be sufficient

- pkgconfig must be required by the devel pacakge since *.pc is packaged

- probably the tests should be moved into the devel package, too


Best regards,
Christian
Comment 2 John Guthrie 2009-07-03 23:40:29 EDT
(In reply to comment #1)
> Hi John,
> 
> I was going to do a full review, however I've stumbled upon some major issues.
> Please can you solve them first before I do the complete review?
> 
> - the package contains a rather old snaphost from 1/2008 - can you package a
> newer snapshot e.g. from 6/2009?

Not done yet.  Should be finished in a couple of days.

> - rpmlint reveals 139 warnings and errors, especially:

Doh!  I think I must have run rpmlint on the specfile and then absent-mindedly forgotten to check the RPM that I had just built.  Here is the latest output from rpmlint:

ewl.i586: W: shared-lib-calls-exit /usr/lib/libewl.so.1.0.0 exit@GLIBC_2.0
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
ewl-devel.i586: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

> * formatting fo the description

Fixed

> * lots of example *.c files in the main package (they should be in the devel
> package)

Fixed.

> * static libraries were packaged - usually they should be completely omitted
> unless there is a reason:
> http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries

Fixed.

> * if the %post/%postun section only contains one line, it should be written
> like this:
> %post -p /sbin/ldconfig

Fixed.

> - using %ghost for *.so.x should not be necessary, 
> %{_libdir}/*.so.* should be sufficient

Is this true even when the file is a symlink created by the %post section.  My thinking was that the *.so.1 files were created by the %post section, so they should be ghosted.

> - pkgconfig must be required by the devel pacakge since *.pc is packaged

One of the automatically generated requires for the devel package is /usr/bin/pkg-config.  Is that not sufficient?

> - probably the tests should be moved into the devel package, too
> 
> 
> Best regards,
> Christian  

I haven't posted the latest package.  I'll be posting it once I get a new version of the software.
Comment 3 Christian Krause 2009-07-04 03:53:24 EDT
(In reply to comment #2)
> (In reply to comment #1)
> > - the package contains a rather old snaphost from 1/2008 - can you package a
> > newer snapshot e.g. from 6/2009?
> 
> Not done yet.  Should be finished in a couple of days.

I've just had a deeper look into the download directories of enlightenment and unfortunately even if there are newer snaphot directories, they don't always contain all packages, only the updated ones. And indeed version 
0.5.2.042 seems to be the most recent one...  

I'm sorry for this - I should have looked more carefully yesterday...
Comment 4 Michael Schwendt 2009-07-04 07:59:12 EDT
> My thinking was that the *.so.1 files were created by the %post
> section, so they should be ghosted.

We run ldconfig primarily to update the dynamic linker's cache file. ldconfig creates the *.so.N symlink, if it isn't present already (or it changes it, if a newer library version is found). However, the *.so.1 symlink ought to be included in your package already. If "make install ..." doesn't create it, you can do it yourself:

  %install
  ...
  make install ...
  /sbin/ldconfig -n %{buildroot}%{_libdir}/


> One of the automatically generated requires for the devel package is
> /usr/bin/pkg-config.  Is that not sufficient?

It is, but only for Fedora >= 11.
Comment 5 John Guthrie 2009-07-05 00:41:02 EDT
(In reply to comment #1)
> - probably the tests should be moved into the devel package, too

Fixed.
Comment 6 John Guthrie 2009-07-05 00:42:26 EDT
(In reply to comment #4)
> > My thinking was that the *.so.1 files were created by the %post
> > section, so they should be ghosted.
> 
> We run ldconfig primarily to update the dynamic linker's cache file. ldconfig
> creates the *.so.N symlink, if it isn't present already (or it changes it, if a
> newer library version is found). However, the *.so.1 symlink ought to be
> included in your package already. If "make install ..." doesn't create it, you
> can do it yourself:
> 
>   %install
>   ...
>   make install ...
>   /sbin/ldconfig -n %{buildroot}%{_libdir}/

Fair enough.  Fixed.

> > One of the automatically generated requires for the devel package is
> > /usr/bin/pkg-config.  Is that not sufficient?
> 
> It is, but only for Fedora >= 11.  

Fixed for Fedora <= 10.
Comment 7 John Guthrie 2009-07-05 00:46:43 EDT
I have posted a new version of the package.  It adds a pkgconfig requirement to the devel package if running on Fedora 10 or earlier.  Here is the rpmlint output:

ewl.i586: W: shared-lib-calls-exit /usr/lib/libewl.so.1.0.0 exit@GLIBC_2.0
ewl-devel.i586: W: no-documentation
2 packages and 0 specfiles checked; 0 errors, 2 warnings.

Here are the relevant URLs for the new version:

http://www.guthrie.info/RPMS/f11/ewl.spec
http://www.guthrie.info/RPMS/f11/ewl-0.5.2.042-7.fc11.src.rpm
Comment 8 Christian Krause 2009-07-05 19:45:06 EDT
Thanks for the new package. I've fully reviewed the package now. In general it looks quite good, there are only some minor TODO items left:

* rpmlint: OK
rpmlint SPECS/ewl.spec SRPMS/ewl-0.5.2.042-7.fc10.src.rpm RPMS/i386/ewl-*

ewl.i386: W: shared-lib-calls-exit /usr/lib/libewl.so.1.0.0 exit@GLIBC_2.0
ewl-devel.i386: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 2 warnings.

According to rpmlint's help the usage of exit in libraries is discouraged since the calling application can not handle the error. However, in this case it seems to be a design decisions of the enlightenment developers and other libraries like ecore suffer from the same problem. This will not block the review. But depending on your involvement with upstream it would probably be worth to ask them for the reasons and/or explain them why its discouraged.

* naming: OK
- name matches upstream
- spec file name matches package name

* sources: OK
- md5sum: 385082d91eb112671a5c64af295da91d  ewl-0.5.2.042.tar.gz
- sources matches upstream
- Source0 tag ok
- spectool -g  works

* License: TODO
- License in spec file does not match the actual license (COPYING looks like a variant of the MIT license)
- however, the included spec file mentiones BSD
- the enlightenment authors mentioned usually only BSD as the license of the
related projects
- I've asked fedora-legal for clarification and got a response that the following license field should be used:
License: MIT with advertising
https://www.redhat.com/archives/fedora-legal-list/2009-July/msg00003.html
- license file packaged

* package containing *.pc files must "Requires: pkgconfig": TODO
- IMHO the usage of %if conditions should be omitted if not really needed
- even if this may be questionable or whether in this case it would be
meaningful, the packaging rules are indisputable here:
http://fedoraproject.org/wiki/Packaging/Guidelines#Pkgconfig_Files
please require pkgconfig unconditionally in the -devel package

* spec file written in English and legible: OK

* compilation: TODO
- supports parallel build
- RPM_OPT_FLAGS are correctly used
- it would be better not to build the static libraries instead of deleting them later, please add a "--disable-static" and remove the deleting of the *.a files
- builds in mock (F10)
- builds in koji:
F12: https://koji.fedoraproject.org/koji/taskinfo?taskID=1454826
F11: https://koji.fedoraproject.org/koji/taskinfo?taskID=1454836
F10: https://koji.fedoraproject.org/koji/taskinfo?taskID=1454851

* BuildRequires: OK

* locales handling: OK (n/a)

* ldconfig in %post and %postun: OK

* package owns all directories that it creates: OK

* no files listed twice in %files: OK

* file permissions: OK
- %defattr used
- actual permissions in packages ok

* %clean section: OK

* macro usage: OK

* code vs. content: OK (only code)

* main package should not contain development related parts: TODO
/usr/lib/ewl/tests should be in -devel package

* large documentation into subpackage: OK (n/a)

* header files in -devel subpackage: OK

* static libraries in -static package: OK (n/a)

* *.so link in -devel package: OK

* - devel package requires base package using fully versioned dependency: OK

* packages must not contain *.la files: OK

* GUI applications must provide *.desktop file: OK (n/a)

* packages must not own files/dirs already owned by other packages: OK

* rm -rf $RPM_BUILD_ROOT at the beginning of %install: OK

* all filenames UTF-8: OK

* functional test: OK
- ewl_config (which uses libewl) in main package (simple tool to configure the themes etc.) works

* debuginfo sub-package: OK
- non-empty
- debuginfo file works together with gdb
Comment 9 John Guthrie 2009-07-05 21:54:06 EDT
(In reply to comment #8)
> Thanks for the new package. I've fully reviewed the package now. In general it
> looks quite good, there are only some minor TODO items left:

Thank you for taking the time to do such a thorough review.

> * License: TODO
> - License in spec file does not match the actual license (COPYING looks like a
> variant of the MIT license)
> - however, the included spec file mentiones BSD
> - the enlightenment authors mentioned usually only BSD as the license of the
> related projects
> - I've asked fedora-legal for clarification and got a response that the
> following license field should be used:
> License: MIT with advertising
> https://www.redhat.com/archives/fedora-legal-list/2009-July/msg00003.html
> - license file packaged

When I looked at the license, I initially mis-identified it as being an BSD license.  And then, like you saw as well, I saw other components of enlightenment with BSD licenses.  So that led me to believe that I really had put in the correct license.

Anyway, this is fixed.

> * package containing *.pc files must "Requires: pkgconfig": TODO
> - IMHO the usage of %if conditions should be omitted if not really needed
> - even if this may be questionable or whether in this case it would be
> meaningful, the packaging rules are indisputable here:
> http://fedoraproject.org/wiki/Packaging/Guidelines#Pkgconfig_Files
> please require pkgconfig unconditionally in the -devel package

Fair enough.  Fixed.
 
> * compilation: TODO
> - supports parallel build
> - RPM_OPT_FLAGS are correctly used
> - it would be better not to build the static libraries instead of deleting them
> later, please add a "--disable-static" and remove the deleting of the *.a files

Fixed.

> * main package should not contain development related parts: TODO
> /usr/lib/ewl/tests should be in -devel package

Fixed.

Here are the URLs for the new release:
http://www.guthrie.info/RPMS/f11/ewl.spec
http://www.guthrie.info/RPMS/f11/ewl-0.5.2.042-8.fc11.src.rpm
Comment 10 Christian Krause 2009-07-06 15:29:45 EDT
> > * License: TODO
> > - License in spec file does not match the actual license (COPYING looks like a
> > variant of the MIT license)
> > - however, the included spec file mentiones BSD
> > - the enlightenment authors mentioned usually only BSD as the license of the
> > related projects
> > - I've asked fedora-legal for clarification and got a response that the
> > following license field should be used:
> > License: MIT with advertising
> > https://www.redhat.com/archives/fedora-legal-list/2009-July/msg00003.html
> > - license file packaged
> 
> When I looked at the license, I initially mis-identified it as being an BSD
> license.  And then, like you saw as well, I saw other components of
> enlightenment with BSD licenses.  So that led me to believe that I really had
> put in the correct license.
> 
> Anyway, this is fixed.

Sorry, not 100% - the line must be exactly as I wrote:

License: MIT with advertising

> > * compilation: TODO
> > - supports parallel build
> > - RPM_OPT_FLAGS are correctly used
> > - it would be better not to build the static libraries instead of deleting them
> > later, please add a "--disable-static" and remove the deleting of the *.a files
> 
> Fixed.

Please remove the commented lines completely:
# Removing .a files
#find $RPM_BUILD_ROOT -name '*.a' -exec rm '{}' \;

> > * main package should not contain development related parts: TODO
> > /usr/lib/ewl/tests should be in -devel package
> 
> Fixed.

Unfortunately this fix has introduced a minor issue: the directory %{_libdir}/%{name} itself is now an orphan, since it isn't included neither in the main nor in the -devel package. Please add
%dir {_libdir}/%{name}
to the main pacakge.
Comment 11 John Guthrie 2009-07-06 23:58:50 EDT
(In reply to comment #10)
> > > * License: TODO
> > > - License in spec file does not match the actual license (COPYING looks like a
> > > variant of the MIT license)
> > > - however, the included spec file mentiones BSD
> > > - the enlightenment authors mentioned usually only BSD as the license of the
> > > related projects
> > > - I've asked fedora-legal for clarification and got a response that the
> > > following license field should be used:
> > > License: MIT with advertising
> > > https://www.redhat.com/archives/fedora-legal-list/2009-July/msg00003.html
> > > - license file packaged
> > 
> > When I looked at the license, I initially mis-identified it as being an BSD
> > license.  And then, like you saw as well, I saw other components of
> > enlightenment with BSD licenses.  So that led me to believe that I really had
> > put in the correct license.
> > 
> > Anyway, this is fixed.
> 
> Sorry, not 100% - the line must be exactly as I wrote:
> 
> License: MIT with advertising

Oops.  Missed that detail.  Fixed.

> > > * compilation: TODO
> > > - supports parallel build
> > > - RPM_OPT_FLAGS are correctly used
> > > - it would be better not to build the static libraries instead of deleting them
> > > later, please add a "--disable-static" and remove the deleting of the *.a files
> > 
> > Fixed.
> 
> Please remove the commented lines completely:
> # Removing .a files
> #find $RPM_BUILD_ROOT -name '*.a' -exec rm '{}' \;

Fixed.

> > > * main package should not contain development related parts: TODO
> > > /usr/lib/ewl/tests should be in -devel package
> > 
> > Fixed.
> 
> Unfortunately this fix has introduced a minor issue: the directory
> %{_libdir}/%{name} itself is now an orphan, since it isn't included neither in
> the main nor in the -devel package. Please add
> %dir {_libdir}/%{name}
> to the main pacakge.  

Missed that one.  Fixed.

Here are the new URLs:
http://www.guthrie.info/RPMS/f11/ewl.spec
http://www.guthrie.info/RPMS/f11/ewl-0.5.2.042-9.fc11.src.rpm

Thanks again for your help.
Comment 12 Christian Krause 2009-07-07 15:46:59 EDT
Hi John,

> Here are the new URLs:

I've reviewed the latest package and all issues brought up during the review were addressed and fixed.

Just one additional suggestion I've overseen before:
To include the libraries in the %files sections you can better write:

in %files:
%{_libdir}/*.so.* 

instead of
%{_libdir}/libewl.so.1
%{_libdir}/libewl.so.1.0.0

and in %files devel:
%{_libdir}/*.so

But since this is really quite subjective and there is no explicit rule about it, I don't think another review cycle is necessary.

Please import the package as it was reviewed into CVS and if you like you can do this minor enhancement later.

Regarding:
> http://www.guthrie.info/RPMS/f11/ewl.spec
> http://www.guthrie.info/RPMS/f11/ewl-0.5.2.042-9.fc11.src.rpm
the package is:

-> APPROVED

Best regards.
Christian
Comment 13 John Guthrie 2009-07-07 16:03:22 EDT
New Package CVS Request
=======================
Package Name: ewl
Short Description: Enlightenment Widget Library
Owners: guthrie
Branches: F-10 F-11
InitialCC:
Comment 14 Jason Tibbitts 2009-07-08 12:30:53 EDT
CVS done.
Comment 15 John Guthrie 2009-07-08 21:36:14 EDT
The package builds cleanly in all branches, so closing this ticket.

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