Bug 891284 (terminology)

Correction:

SRPM URL: http://sundaram.fedorapeople.org/packages/terminology-0.2.0-1.fc18.src.rpm

I'd like to work with you on this if it's okay with you. Was looking at terminology and got sucked in to building the whole Enlightenment stack.

(and ecore)

terminology has released 0.3 for months.

Please update so we can review it.

Updated:

Spec URL: http://vicodan.fedorapeople.org/terminology.spec
SRPM URL: http://vicodan.fedorapeople.org/terminology-0.3.0-1.fc21.src.rpm

Description:
Fast and lightweight terminal emulator using EFL libraries.

Taking.

There are three small issues with the package (icon location, an unnecessary
explicit requirement, and preserving the timestamps of installed files) that
you can fix easily.

However, I am more concerned about the bundled fonts. Most of the problems I
found are all due to the bundled fonts, and will get automatically "fixed" if
you can unbundle.


Summary
=======

[!]: Icon installed in /usr/share/pixmaps

    => This directory is considered obsolete, application icons should be
       installed in /usr/share/icons/<theme>/<size>/apps/ and you must then
       run the icon cache update scriptlets.

       https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

[!]: Requires correct, justified where necessary.

    => Please drop the explicit Requires on elementary.

       It is unneeded, as rpmbuild automatically adds a requirement on
       libelementary.so.1%{_isa}, which is provided by the elementary package.

[!]: Packages should try to preserve timestamps of original installed files.

    => Please use the following to install the files:

        make install DESTDIR=%{buildroot} INSTALL="/usr/bin/install -p"

[!]: Package contains no bundled libraries without FPC exception.

    => No bundled library, but I'm reusing this item for the fact that the
       package bundles a few fonts:

        - Terminus is also available in the terminus-fonts package
        - The X fonts are also available in the xorg-x11-fonts-misc

        You should just remove the fonts from the package, and add the
        corresponding Requires: if they are needed.

        But that's just weird, why would an application require certain fonts?

        From a very quick look at the code (src/bin/options_font.c), it does
        seem like Terminology expects the fonts to be installed in a specific
        directory (given its name, I guess the elm_app_data_dir_get() function
        returns /usr/share/terminology/ ?)

        - nexus.pcf

        I can't figure where this one is coming from.

        All in all, can you ask for advice on the fonts mailing-list?

[!]: License field in the package spec file matches the actual license.

    => Given the bundled fonts, the current License field is wrong.

       Of course, this requirement gets waived if you unbundle the fonts.

[!]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.

    => As long as the fonts are bundled, then you should add the following
       files as %doc :

           data/fonts/TERMINUS.txt
           data/fonts/XFONT.txt

       Of course, this requirement gets waived if you unbundle the fonts.

[!]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
     Note: Arch-ed rpms have a total of 2826240 bytes in /usr/share 2826240
     terminology-0.3.0-1.fc21.x86_64.rpm

    => This is large enough to warrant a noarch subpackage IMHO.

       Note that most of the size comes from the bundled fonts.

       Of course, this requirement gets waived if you unbundle the fonts.


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed

MUST items
----------

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[!]: Package contains no bundled libraries without FPC exception.

    => No bundled library, but I'm reusing this item for the fact that the
       package bundles a few fonts:

        - Terminus is also available in the terminus-fonts package
        - The X fonts are also available in the xorg-x11-fonts-misc

        You should just remove the fonts from the package, and add the
        corresponding Requires: if they are needed.

        But that's just weird, why would an application require certain fonts?

        From a very quick look at the code (src/bin/options_font.c), it does
        seem like Terminology expects the fonts to be installed in a specific
        directory (given its name, I guess the elm_app_data_dir_get() function
        returns /usr/share/terminology/ ?)

        - nexus.pcf

        I can't figure where this one is coming from.

        All in all, can you ask for advice on the fonts mailing-list?

[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[!]: Package complies to the Packaging Guidelines

    => See details of issues.

[!]: License field in the package spec file matches the actual license.

    => Given the bundled fonts, the current License field is wrong, but once
       you unbundle the fonts it will be ok.

[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[!]: Requires correct, justified where necessary.

    => Please drop the explicit Requires on elementary.

       It is unneeded, as rpmbuild automatically adds a requirement on
       libelementary.so.1%{_isa}, which is provided by the elementary package.

[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[-]: update-desktop-database is invoked when required
[x]: Useful -debuginfo package or justification otherwise.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 20480 bytes in 2 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install if there is
     such a file.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[!]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.

    => As long as the fonts are bundled, then you should add the following
       files as %doc :

           data/fonts/TERMINUS.txt
           data/fonts/XFONT.txt

       Of course, this requirement gets waived if you unbundle the fonts.

[-]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).


SHOULD items
------------

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[!]: Packages should try to preserve timestamps of original installed files.

    => Please use the following to install the files:

        make install DESTDIR=%{buildroot} INSTALL="/usr/bin/install -p"

[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[-]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[-]: Spec use %global instead of %define.


EXTRA items
-----------

Generic:
[!]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
     Note: Arch-ed rpms have a total of 2826240 bytes in /usr/share 2826240
     terminology-0.3.0-1.fc21.x86_64.rpm

    => This is large enough to warrant a noarch subpackage IMHO. However, most
       of the size comes from the bundled fonts, so if you unbundle them then
       the size will probably become small again, so that a subpackage would
       not be needed.

       I won't block the review on this specific item in any case.

[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------

Checking: terminology-0.3.0-1.fc21.x86_64.rpm
terminology.x86_64: W: no-manual-page-for-binary typop
terminology.x86_64: W: no-manual-page-for-binary tybg
terminology.x86_64: W: no-manual-page-for-binary tyalpha
terminology.x86_64: W: no-manual-page-for-binary tycat
terminology.x86_64: W: no-manual-page-for-binary tyq
terminology.x86_64: W: no-manual-page-for-binary tyls
terminology.x86_64: W: no-manual-page-for-binary terminology
1 packages and 0 specfiles checked; 0 errors, 7 warnings.

    => Not blocking the review on this, but it would certainly be nice if you
       asked upstream to fix these. :-)

Rpmlint (installed packages)
----------------------------

# rpmlint terminology
terminology.x86_64: W: no-manual-page-for-binary typop
terminology.x86_64: W: no-manual-page-for-binary tybg
terminology.x86_64: W: no-manual-page-for-binary tyalpha
terminology.x86_64: W: no-manual-page-for-binary tycat
terminology.x86_64: W: no-manual-page-for-binary tyq
terminology.x86_64: W: no-manual-page-for-binary tyls
terminology.x86_64: W: no-manual-page-for-binary terminology
1 packages and 0 specfiles checked; 0 errors, 7 warnings.
# echo 'rpmlint-done:'


Requires
--------

terminology (rpmlib, GLIBC filtered):
    elementary
    libc.so.6()(64bit)
    libdbus-1.so.3()(64bit)
    libecore.so.1()(64bit)
    libecore_con.so.1()(64bit)
    libecore_evas.so.1()(64bit)
    libecore_file.so.1()(64bit)
    libecore_imf.so.1()(64bit)
    libecore_imf_evas.so.1()(64bit)
    libecore_input.so.1()(64bit)
    libecore_ipc.so.1()(64bit)
    libedbus.so.1()(64bit)
    libedje.so.1()(64bit)
    libeet.so.1()(64bit)
    libefreet.so.1()(64bit)
    libefreet_mime.so.1()(64bit)
    libefreet_trash.so.1()(64bit)
    libeina.so.1()(64bit)
    libelementary.so.1()(64bit)
    libemotion.so.1()(64bit)
    libethumb.so.1()(64bit)
    libethumb_client.so.1()(64bit)
    libevas.so.1()(64bit)
    libexif.so.12()(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    rtld(GNU_HASH)


Provides
--------

terminology:
    terminology
    terminology(x86-64)


Source checksums
----------------

http://download.enlightenment.org/releases/terminology-0.3.0.tar.bz2 :
  CHECKSUM(SHA256) this package     : 7ea94aec40fc0e210543b36236842f1dfe3c64abf4f3d7ae6f5dae2fb668476e
  CHECKSUM(SHA256) upstream package : 7ea94aec40fc0e210543b36236842f1dfe3c64abf4f3d7ae6f5dae2fb668476e


Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 891284

Updated:

Spec URL: http://vicodan.fedorapeople.org/terminology.spec
SRPM URL: http://vicodan.fedorapeople.org/terminology-0.3.0-2.fc21.src.rpm

Per our IRC conversation I have removed all the bundled fonts except Nexus which comes from Enlightenment upstream which falls under the BSD license.

Almost good to go.


--- 891284-terminology.5/srpm-unpacked/terminology.spec	2013-10-09 12:31:21.451827547 +0800
+++ 891284-terminology/srpm-unpacked/terminology.spec	2013-10-11 11:21:19.356240964 +0800
@@ -1,6 +1,6 @@
 Name:           terminology
 Version:        0.3.0
-Release:        1%{?dist}
+Release:        2%{?dist}
 License:        BSD
 Summary:        EFL based terminal emulator
 Url:            http://www.enlightenment.org
@@ -14,7 +14,8 @@
 BuildRequires:  ethumb-devel
 BuildRequires:  evas-devel
 BuildRequires:  libeina-devel
-Requires:       elementary

    => This fixes the point I raised about the unnecessary requirement.

+Requires:       terminus-fonts 
+Requires:       xorg-x11-fonts-misc
 

    => I'm honestly not sure this is needed. Having taken a quick look at the
       source code, the app seems to only pick up fonts installed in
       /usr/share/terminology/fonts... :-/

       What does upstream say?

       In any case, I won't block the review on this, you've unbundled the
       fonts, so at worst you're introducing some unnecessary requirements
       without making the fonts usable.

 %description
 Fast and lightweight terminal emulator using EFL libraries.
@@ -27,12 +28,41 @@
 make %{?_smp_mflags} V=1
 
 %install
-make install DESTDIR=%{buildroot}
+make install DESTDIR=%{buildroot} INSTALL="/usr/bin/install -p"

    => This ensures timestamps are preserved, fixing one of the points I
       raised.

+#Remove fonts that already exist in Fedora
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/10x20.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/4x6.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/5x7.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/5x8.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/6x10.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/6x12.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/6x13.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/6x9.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/7x13.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/7x14.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/8x13.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/9x15.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/9x18.pcf
+rm -rf %{buildroot}%{_datadir}/terminology/fonts/terminus-*
 
    => This unbundles the fonts, fixing a lot of problems I had raised.

-sed -i 's/.png[[:blank:]]*$//' %{buildroot}%{_datadir}/applications/terminology.desktop

    => This became unnecessary when upstream dropped the extension from the
       Icon= directive in the desktop file, good you're removing it.

-mv %{buildroot}%{_datadir}/icons %{buildroot}%{_datadir}/pixmaps

    => This was doubtful, and causing the point I raised about icon location,
       so I'm glad you're removing it.

       I still think this is not the right place for icons, though. As per the
       FreeDesktop.org spec, they should instead go in
       /usr/share/icons/hicolor/<size>/apps

       However, I'm worried E17 might not find it any more if you move it, as
       upstream clearly intends to install it in /usr/share/icons.

       So I won't block the review on this, but I'd be happier if you got
       upstream to move its icon to the right place, respecting the FDO spec.

 desktop-file-validate %{buildroot}/%{_datadir}/applications/terminology.desktop
 
+%post
+/bin/touch --no-create %{_datadir}/icons &>/dev/null || :
+/usr/bin/update-desktop-database &> /dev/null || :
+
+%postun
+/usr/bin/update-desktop-database &> /dev/null || :
+if [ $1 -eq 0 ] ; then
+    /bin/touch --no-create %{_datadir}/icons &>/dev/null
+    /usr/bin/gtk-update-icon-cache %{_datadir}/icons &>/dev/null || :
+fi
+
+%posttrans
+/usr/bin/gtk-update-icon-cache %{_datadir}/icons &>/dev/null || :

    => This is all completely unnecessary.

       First, the desktop-update-database call is only needed if the desktop
       file contains a MimeType= entry. Please remove it.

           http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database

       Second, the gtk-update-icon-cache just fails if you run it with
       /usr/share/icons:

           # /usr/bin/gtk-update-icon-cache /usr/share/icons
           gtk-update-icon-cache: No theme index file.

       Updating the cache is only needed if you install the icon in one of the
       themes (usually "hicolor"), which you are not doing here.

       Please remove this too.

 %files
 %doc README COPYING
 %{_bindir}/tyalpha
@@ -43,10 +73,15 @@
 %{_bindir}/tyq
 %{_bindir}/terminology
 %{_datadir}/applications/terminology.desktop
-%{_datadir}/pixmaps/terminology.png
+%{_datadir}/icons/terminology.png

    => Ack.

 %{_datadir}/terminology
 
+
 %changelog
+* Wed Oct 09 2913 Dan Mashal <dan.mashal> - 0.3.0-2
+- Remove bundled fonts that already exist in Fedora.
+- Add icon and desktop scriptlets
+
 * Tue Oct 08 2013 Dan Mashal <dan.mashal> - 0.3.0-1
 - Update to 0.3.0
 - Fix license

Updated:

Spec URL: http://vicodan.fedorapeople.org/terminology.spec
SRPM URL: http://vicodan.fedorapeople.org/terminology-0.3.0-3.fc19.src.rpm

> +Requires:       terminus-fonts 
> +Requires:       xorg-x11-fonts-misc
>  
> 
>     => I'm honestly not sure this is needed. Having taken a quick look at the
>        source code, the app seems to only pick up fonts installed in
>        /usr/share/terminology/fonts... :-/
> 
>        What does upstream say?
> 
>        In any case, I won't block the review on this, you've unbundled the
>        fonts, so at worst you're introducing some unnecessary requirements
>        without making the fonts usable.

No need to ask them, terminology does load these fonts when they are installed.  Requiring these fonts makes up for unbundling them.

> +%post
> +/bin/touch --no-create %{_datadir}/icons &>/dev/null || :
> +/usr/bin/update-desktop-database &> /dev/null || :
> +
> +%postun
> +/usr/bin/update-desktop-database &> /dev/null || :
> +if [ $1 -eq 0 ] ; then
> +    /bin/touch --no-create %{_datadir}/icons &>/dev/null
> +    /usr/bin/gtk-update-icon-cache %{_datadir}/icons &>/dev/null || :
> +fi
> +
> +%posttrans
> +/usr/bin/gtk-update-icon-cache %{_datadir}/icons &>/dev/null || :
> 
>     => This is all completely unnecessary.
> 
>        First, the desktop-update-database call is only needed if the desktop
>        file contains a MimeType= entry. Please remove it.
> 
>           
> http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database
> 
>        Second, the gtk-update-icon-cache just fails if you run it with
>        /usr/share/icons:
> 
>            # /usr/bin/gtk-update-icon-cache /usr/share/icons
>            gtk-update-icon-cache: No theme index file.
> 
>        Updating the cache is only needed if you install the icon in one of
> the
>        themes (usually "hicolor"), which you are not doing here.
> 
>        Please remove this too.

Removed.

(In reply to Dan Mashal from comment #11)
> > +Requires:       terminus-fonts 
> > +Requires:       xorg-x11-fonts-misc
> >  
> > 
> >     => I'm honestly not sure this is needed. Having taken a quick look at the
> >        source code, the app seems to only pick up fonts installed in
> >        /usr/share/terminology/fonts... :-/
> > 
> >        What does upstream say?
> > 
> >        In any case, I won't block the review on this, you've unbundled the
> >        fonts, so at worst you're introducing some unnecessary requirements
> >        without making the fonts usable.
> 
> No need to ask them, terminology does load these fonts when they are
> installed.  Requiring these fonts makes up for unbundling them.

Glad you checked. Ok then.

> > +%post
> > +/bin/touch --no-create %{_datadir}/icons &>/dev/null || :
> > +/usr/bin/update-desktop-database &> /dev/null || :
> > +
> > +%postun
> > +/usr/bin/update-desktop-database &> /dev/null || :
> > +if [ $1 -eq 0 ] ; then
> > +    /bin/touch --no-create %{_datadir}/icons &>/dev/null
> > +    /usr/bin/gtk-update-icon-cache %{_datadir}/icons &>/dev/null || :
> > +fi
> > +
> > +%posttrans
> > +/usr/bin/gtk-update-icon-cache %{_datadir}/icons &>/dev/null || :
> > 
> >     => This is all completely unnecessary.
> > 
> >        First, the desktop-update-database call is only needed if the desktop
> >        file contains a MimeType= entry. Please remove it.
> > 
> >           
> > http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database
> > 
> >        Second, the gtk-update-icon-cache just fails if you run it with
> >        /usr/share/icons:
> > 
> >            # /usr/bin/gtk-update-icon-cache /usr/share/icons
> >            gtk-update-icon-cache: No theme index file.
> > 
> >        Updating the cache is only needed if you install the icon in one of
> > the
> >        themes (usually "hicolor"), which you are not doing here.
> > 
> >        Please remove this too.
> 
> Removed.

Checked your latest spec file and this is indeed removed.

Package is approved.

Package Change Request
======================
Package Name: terminology
New Branches: f19 f20
Owners: vicodan sundaram spot

New package, please submit a New Package request.

New Package SCM Request
=======================
Package Name: terminology
Short Description: Fast and lightweight terminal emulator using EFL libraries.
Owners: vicodan sundaram spot
Branches: f19 f20

Git done (by process-git-requests).

terminology-0.3.0-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/terminology-0.3.0-3.fc19

terminology-0.3.0-3.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/terminology-0.3.0-3.fc20

terminology-0.3.0-3.fc20 has been pushed to the Fedora 20 testing repository.

terminology-0.3.0-3.fc19 has been pushed to the Fedora 19 stable repository.

terminology-0.3.0-3.fc20 has been pushed to the Fedora 20 stable repository.