Bug 454207 - (Terminator) Review Request: Terminator - Multiple terminals in one window
Review Request: Terminator - Multiple terminals in one window
Status: CLOSED DUPLICATE of bug 462173
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Extras Quality Assurance
:
: 455811 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-06 12:19 EDT by Mathieu BONIFACE
Modified: 2009-05-22 12:52 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-09-15 12:41:56 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wolfy: fedora‑review+


Attachments (Terms of Use)
patch for terminatorterm.py (2.96 KB, patch)
2008-07-11 20:19 EDT, Mathieu BONIFACE
no flags Details | Diff

  None (edit)
Description Mathieu BONIFACE 2008-07-06 12:19:04 EDT
Spec URL: http://mat.boniface.googlepages.com/terminator.spec
SRPM URL: http://mat.boniface.googlepages.com/terminator-0.8.1-1.fc9.src.rpm

Description: Terminator, multiple terminals in one window. 
This is a little project to produce an 
efficient way of filling a large area of 
screen space with terminals.
The robot future of terminals

I need you review my english (i'm french)
Comment 1 Mathieu BONIFACE 2008-07-06 16:12:40 EDT
it's my first package, so i need a sponsor
Comment 2 manuel wolfshant 2008-07-06 16:16:30 EDT
Mathieu, could you please explain (even in French) what you meant by "The robot
future of terminals" ? It sounds pretty odd to me and I am not at all sure about
the meaning.
Comment 3 Mathieu BONIFACE 2008-07-06 16:44:07 EDT
Sorry, i've copy it from the official site

https://launchpad.net/terminator/

The spec and src.rpm links has been updated
Comment 4 Mathieu BONIFACE 2008-07-06 19:42:39 EDT
Spec URL: http://mat.boniface.googlepages.com/terminator.spec
SRPM URL: http://mat.boniface.googlepages.com/terminator-0.8.1-1.fc9.src.rpm

Links updated

--> added %oost and %postun scriptlets to update gtk icon cache
--> modified vendor in desktop-file-install command to "fedora"
Comment 5 manuel wolfshant 2008-07-07 01:31:04 EDT
Mathieu, in the future, each time you modify the spec, please increase the
release tag and add to the changelog a short description of the modifications.
This makes easier for reviewers (and even for yourself ) to track the changes
over time.


I am leaving for a 2 weeks vacation now, but if till I return your package has
not been officialy reviewed by someone else ( I am not assigning it to me on
purpose) I will see to it.
Comment 6 Mathieu BONIFACE 2008-07-07 17:06:58 EDT
Spec URL : http://mat.boniface.googlepages.com/terminator.spec
SRPM URL : http://mat.boniface.googlepages.com/terminator-0.8.1-2.fc9.src.rpm

@wolfshant : I understood, thank you.
Comment 8 Alan Dunn 2008-07-10 08:54:23 EDT
I can't do an official review (I'm in need of sponsorship myself!), but I have
looked at what will come up under a review:

X rpmlint is not silent:

terminator.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/terminatorlib/version.py 0644
terminator.noarch: E: wrong-script-interpreter
/usr/lib/python2.5/site-packages/terminatorlib/freebsd.py "/usr/local/bin/python"

(Perhaps freebsd.py shouldn't even appear in the Fedora RPM, you can direct the
spec file to not install that)

terminator.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/terminatorlib/freebsd.py 0644
terminator.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/terminatorlib/config.py 0644
terminator.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/terminatorlib/encoding.py 0644
terminator.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/terminatorlib/terminator.py 0644
terminator.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/terminatorlib/terminatorterm.py 0644

It seems that you create a terminator script already in /usr/bin, and that the
rest of the files /usr/lib could do without the #! notation (and this would fix
the problem).

- package name seems alright
- licensing information formatted correctly and matches project license
- md5 matches upstream
- spec file legible, in American English

However, I would change the description. From the way it was written I didn't
even realize it was for GNOME terminals (I originally thought it was like
screen.) You could use (from upstream):

Terminator is a program that allows users to set up flexible arrangements of
GNOME terminals. It is aimed at those who normally arrange lots of terminals
near each other, but don't want to use a frame based window manager.

- Builds on at least one supported platform (checked on i386)
- Locales appear to be handled properly (with %find_lang), though I'm not an
expert on this issue, so someone else's opinion would be better here
- Runs rm -rf %{buildroot} in %clean, %install
X Macro usage inconsistent:

You use $RPM_BUILD_ROOT in some places and %{buildroot} in others. You need to
use only one, I recommend the second one.
- Has a .desktop file installed with desktop-file-install

Shoulds:
- Builds in mock
- Actually runs

So, all in all, you have only a few relatively minor issues that I can see, but
as I'm fairly new to this game a second opinion, for example, that of the person
who will be doing the official review, would probably be good.

PS - "The robot future of terminals" is a pun based on the name of the project -
about Terminator the movie series :)
Comment 9 Pierre-YvesChibon 2008-07-10 10:26:19 EDT
(In reply to comment #8)
> X rpmlint is not silent:
> 
> terminator.noarch: E: non-executable-script
> /usr/lib/python2.5/site-packages/terminatorlib/version.py 0644
> terminator.noarch: E: wrong-script-interpreter
> /usr/lib/python2.5/site-packages/terminatorlib/freebsd.py "/usr/local/bin/python"
> 
> (Perhaps freebsd.py shouldn't even appear in the Fedora RPM, you can direct the
> spec file to not install that)
> 
> terminator.noarch: E: non-executable-script
> /usr/lib/python2.5/site-packages/terminatorlib/freebsd.py 0644
> terminator.noarch: E: non-executable-script
> /usr/lib/python2.5/site-packages/terminatorlib/config.py 0644
> terminator.noarch: E: non-executable-script
> /usr/lib/python2.5/site-packages/terminatorlib/encoding.py 0644
> terminator.noarch: E: non-executable-script
> /usr/lib/python2.5/site-packages/terminatorlib/terminator.py 0644
> terminator.noarch: E: non-executable-script
> /usr/lib/python2.5/site-packages/terminatorlib/terminatorterm.py 0644
> 
> It seems that you create a terminator script already in /usr/bin, and that the
> rest of the files /usr/lib could do without the #! notation (and this would fix
> the problem).
> 

I do not agree, these are python script, so they have to be executable, rpmlint
detect the shebang but the executable right this is why it is complainning, a
simple chmod +x solves the question.

There is also a wrong shebang that have to be changed via sed most probably...
The question of the presence of freebsd.py could indeed be asked. The best way
would be to test wether the program really needs it (vs crash without)

> However, I would change the description. From the way it was written I didn't
> even realize it was for GNOME terminals (I originally thought it was like
> screen.) You could use (from upstream):
> 
> Terminator is a program that allows users to set up flexible arrangements of
> GNOME terminals. It is aimed at those who normally arrange lots of terminals
> near each other, but don't want to use a frame based window manager.

hm does not work under KDE ??
Does it really requires this specification ?
Comment 10 Alan Dunn 2008-07-10 21:00:02 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > X rpmlint is not silent:
> > 
> > terminator.noarch: E: non-executable-script
> > /usr/lib/python2.5/site-packages/terminatorlib/version.py 0644
> > terminator.noarch: E: wrong-script-interpreter
> > /usr/lib/python2.5/site-packages/terminatorlib/freebsd.py
"/usr/local/bin/python"
> > 
> > (Perhaps freebsd.py shouldn't even appear in the Fedora RPM, you can direct the
> > spec file to not install that)
> > 
> > terminator.noarch: E: non-executable-script
> > /usr/lib/python2.5/site-packages/terminatorlib/freebsd.py 0644
> > terminator.noarch: E: non-executable-script
> > /usr/lib/python2.5/site-packages/terminatorlib/config.py 0644
> > terminator.noarch: E: non-executable-script
> > /usr/lib/python2.5/site-packages/terminatorlib/encoding.py 0644
> > terminator.noarch: E: non-executable-script
> > /usr/lib/python2.5/site-packages/terminatorlib/terminator.py 0644
> > terminator.noarch: E: non-executable-script
> > /usr/lib/python2.5/site-packages/terminatorlib/terminatorterm.py 0644
> > 
> > It seems that you create a terminator script already in /usr/bin, and that the
> > rest of the files /usr/lib could do without the #! notation (and this would fix
> > the problem).
> > 
> 
> I do not agree, these are python script, so they have to be executable, rpmlint
> detect the shebang but the executable right this is why it is complainning, a
> simple chmod +x solves the question.

I believe that's only the case if you are actually running the files like
executables at the command line. However, the purpose of these files is to be
imported by the terminator program, which is in /usr/bin. It seems to be general
practice to keep files in the python site-packages as 0644, and it seems that
for scripting languages the goal is to keep only modules (for example, for Perl,
.pm files) in the library (%{_libdir}) directory.

That's not to say that it hasn't been done the other way too... in fact, rpmlint
seems to complain about some of the source RPMs that one can download that are
already in the repositories... (so I would guess that folks are somewhat
permissive about this point or were at some point in the past.) However, the
vast majority appear to be with non-shebanged files in %{_libdir} - one of the
main violators seems to be python files in the python package itself.

The program runs with all those shebangs eliminated, and furthermore, with
freebsd.py and related files eliminated.

> There is also a wrong shebang that have to be changed via sed most probably...
> The question of the presence of freebsd.py could indeed be asked. The best way
> would be to test wether the program really needs it (vs crash without)
> 
> > However, I would change the description. From the way it was written I didn't
> > even realize it was for GNOME terminals (I originally thought it was like
> > screen.) You could use (from upstream):
> > 
> > Terminator is a program that allows users to set up flexible arrangements of
> > GNOME terminals. It is aimed at those who normally arrange lots of terminals
> > near each other, but don't want to use a frame based window manager.
> 
> hm does not work under KDE ??
> Does it really requires this specification ?

That is a good point, I copied that description from upstream, but I just wanted
to make the point that it requires X windows.
Comment 11 Mathieu BONIFACE 2008-07-11 20:19:58 EDT
Created attachment 311631 [details]
patch for terminatorterm.py
Comment 12 Mathieu BONIFACE 2008-07-11 20:21:31 EDT
Comment on attachment 311631 [details]
patch for terminatorterm.py

Spec URL : http://mat.boniface.googlepages.com/terminator.spec
SRPM URL : http://mat.boniface.googlepages.com/terminator-0.9-2.fc9.src.rpm

- freebsd.py has been removed

- terminatorterm.py has been patched :
   + fixed vte-warning at startup because of freebsd compatibility.
   + removed block that contain import freebsd.py even if this block can't be
interpreted.

- Removed shebang into %{python-sitelib}/* files.

- Changed description, I have voluntary remove « GNOME » because terminator
works on KDE.
Comment 13 Mamoru TASAKA 2008-07-17 22:40:32 EDT
*** Bug 455811 has been marked as a duplicate of this bug. ***
Comment 14 Mathieu BONIFACE 2008-07-20 16:53:38 EDT
Spec URL : http://mat.boniface.googlepages.com/terminator.spec
SRPM URL : http://mat.boniface.googlepages.com/terminator-0.9-3.fc9.src.rpm

 - added version in patch name.

rpmlint is silent on srpms and rpms.
mock is silent on fedora-9-i386 and fedora-9-x86_64.
Comment 15 manuel wolfshant 2008-07-23 11:17:20 EDT
- your source URL has a small error. you should use "%{version}" instead of the
0.8.1 which is over there now
- rpmlint is not silent at all, I'd say that your fixes (sed + rm freebsd.py)
are not correctly applied. See
http://koji.fedoraproject.org/koji/taskinfo?taskID=734181
for reference. In addition rpmlint also complains about the desktop file missing
a required "Encoding" key
Comment 16 Mamoru TASAKA 2008-07-23 11:46:36 EDT
wolfy, thank you for reviewing this. Actually I was hoping someone would
review this.

Just a comment for this
(In reply to comment #15)
> - rpmlint is not silent at all, I'd say that your fixes (sed + rm freebsd.py)
> are not correctly applied. See
> http://koji.fedoraproject.org/koji/taskinfo?taskID=734181
> for reference.

See these lines on build.log:
---------------------------------------------------------------
error: 
%patch without corresponding "Patch:" tag
---------------------------------------------------------------
It seems that rawhide rpm (4.5.90) never accepts the situation like
---------------------------------------------------------------
    10  Patch0:         terminator-fix_terminatorterm-0.9.patch

    31  %patch -p1 -b .fix_terminatorterm_0.9
---------------------------------------------------------------
You should use "Patch: <-> %patch" or "Patch0: <-> %patch0".

Comment 17 manuel wolfshant 2008-07-23 12:56:00 EDT
well then, fix the %patch line, your package will/should land in rawhide first:)

and do not forget about the desktop file:
terminator.noarch: E: invalid-desktopfile /usr/share/applications/terminator.desktop
Comment 18 Mathieu BONIFACE 2008-07-23 14:34:15 EDT
Spec URL : http://mat.boniface.googlepages.com/terminator.spec
SRPM URL : http://mat.boniface.googlepages.com/terminator-0.9-4.fc9.src.rpm

(In reply to comment #17)
> well then, fix the %patch line, your package will/should land in rawhide first:)

 - %patch fixed
   I've built it on rawhide and it seems that's ok
 
> and do not forget about the desktop file:
> terminator.noarch: E: invalid-desktopfile
/usr/share/applications/terminator.desktop

- rpmlint is silent for me (on F9) but desktop-file-validate says :

  [builder@localhost data]$ desktop-file-validate terminator.desktop 
  terminator.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated

  Also, terminator.desktop doesn't include special characters, so I think that's
useless to put the encoding key into.
  
Comment 19 manuel wolfshant 2008-07-23 15:36:25 EDT
You are right: I was checking using an older version of desktop-file-validate.
The newer version does not complain about the "Encoding" key, as
http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#deprecated-items
suggests it is deprecated.


I'll be back soon with the review.
Comment 20 manuel wolfshant 2008-07-23 15:57:25 EDT
Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on: devel/x86_64
 [x] Rpmlint output:
source RPM: empty
binary RPM:empty
 [x] Package is not relocatable.
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [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] License field in the package spec file matches the actual license.
     License type:GPLv2
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing th
e text of the license(s) for the package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
     SHA1SUM of package: fb61fe78a32b480ec00692cfc9272500e0872ec5 
terminator_0.9.tar.gz
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packag
ing Guidelines.
 [x] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot}.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [x] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [x] Description and summary sections in the package spec file contains
translations for supported Non-English languages, i
f available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: devel/x86_64 and i386
 [?] Package should compile and build into binary rpms on all supported
architectures.
     Tested on:
 [x] Package functions as described.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.


================
*** APPROVED ***
================


Mathieu, are you already sponsored ? If not, can you point me to any other
contributions of yours to fedora ?
Comment 21 Mathieu BONIFACE 2008-07-23 16:57:02 EDT
Wolphy, I'm not sponsored, also it's my first contribution at the Fedora project.
At present I make a package for galaxium-messenger
(http://code.google.com/p/galaxium/) so I think I could point you in few days.
Comment 22 manuel wolfshant 2008-07-23 18:05:22 EDT
Tres bien, alors. I am a potential sponsor, but I need to see some more
contributions from your part before sponsoring you. 
Remember that as an alternative to contributing new packages, you could also do
some preliminary reviews for other packages submitted to Fedora.
Comment 23 Mamoru TASAKA 2008-08-03 12:59:28 EDT
To Wolfy:
It seems that Mathieu has another review requests, bug 456774 and bug 456783 .

To Mathieu:
Please post to this bug entry that you have already submitted some other
review requests. This is needed for sponsoring process.
Comment 24 manuel wolfshant 2008-08-06 05:42:29 EDT
(In reply to comment #23)
Thanks, Mamoru. I've added myself to the CC: list of those bugs
Comment 25 Mathieu BONIFACE 2008-08-28 17:27:32 EDT
Spec URL : http://mat.boniface.googlepages.com/terminator.spec
SRPM URL : http://mat.boniface.googlepages.com/terminator-0.10-1.fc9.src.rpm

 - update to 0.10


rpmlint and mock are silent
Comment 26 Mamoru TASAKA 2008-09-13 09:00:24 EDT
*** Bug 462173 has been marked as a duplicate of this bug. ***
Comment 27 Mathieu BONIFACE 2008-09-13 09:21:04 EDT
(In reply to comment #26)
> *** Bug 462173 has been marked as a duplicate of this bug. ***

If Max Spevack wants to take this package, it can, I haven't enough time in this moment to contribute in order to be sponsorised.

see you soon :-)
Comment 28 Max Spevack 2008-09-15 12:40:57 EDT
Ok, I'm going to take Mathieu up on that offer and take the package.  I'm also going to re-open my review request, and mark this bug a duplicate of it.
Comment 29 Max Spevack 2008-09-15 12:41:56 EDT

*** This bug has been marked as a duplicate of bug 462173 ***

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