Bug 292121

Note: This is NOT a formal review (as I am not a reviewer).

[x] package meets naming guidelines
[x] specfile is encoded in ascii or utf-8
[x] specfile matches base package name
[!] specfile uses macros consistently
    - use %{_bindir} in %install
    - maybe use a macro for the value of GLOBALCONF, because this is
      used twice
[x] specfile is written cleanly
[!] specfile is written in AE
    contains non-english comments
[x] changelog is present and has correct format
[x] license matches actual license
[x] license is open source-compatible
[x] license text is included in package
[x] source tag has correct url
[x] source files match upstream
    md5sum: d2ef9fa28a87bf32b3fe0c47ab82fa97
[x] latest version is packaged
[x] summary is concise
[x] dist tag is present
[x] buildroot is correct
[x] buildroot is prepped
[x] %clean is present
[x] proper build requirements
[x] proper requirements
[!] uses %{?_smp_mflags}
[!] uses %{optflags}
    possibly requires patching the Makefile
[x] doesn't use %makeinstall
[x] package builds at least on one architecture
    tested on: fc7 x86_64
[!] packages installs and runs at least on one architecture
    boxes fails on fc7 x86_64 with:
    boxes: Alleged system-wide config file '/usr/share/boxes' is a directory
[!] rpmlint is quiet
    boxes.src:26: W: setup-not-quiet
    boxes.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 8)
[x] final provides/requires look sane
[-] ldconfig called in %post and %postun if required
[x] code, not content
[x] file permissions are appropriate
[!] debuginfo package looks usable
    the debuginfo package is empty, because -g is missing in CFLAGS,
    see above
[-] config files marked as %config(noreplace)
[x] owns all directories it creates
[-] static libraries in -devel subpackage
[-] header files in -devel subpackage
[-] development .so files in -devel subpackage
[-] pkgconfig files in -devel subpackage, requires pkgconfig
[x] no .la files
[x] doesn't need a -docs subpackage
[x] relevant docs are included
[x] doc files are not needed at runtime
[-] provides a .desktop file, build-requires desktop-file-utils
[-] uses %find_lang, build-requires gettext

Additional notes / suggestions:

- consider packaging doc/boxes.el (to ${datadir}/%{name})
- use install -p to preserve timestamps
- default permissions for directories are not set in %defattr
- use wildcard: %{_mandir}/man1/boxes.1*

Thomas,
thanks for taking a look at my package. Indeed, most of the things you pointed 
out were packaging mistakes and are fixed at:

http://hrozkovi.cz/boxes.spec
http://hrozkovi.cz/boxes-1.1-2.fc7.src.rpm

The only thing I could not reproduce was the error message " Alleged 
system-wide config file '/usr/share/boxes' is a directory" .. could you please 
retry? I've tested basic functionality of boxes on F7 i386 and rawhide x86_64 
and on both arches it runs fine...

One final note, the basic functionality test in my case was:
$ echo "hello" | boxes -d parchment
to make sure that boxes run and are able to display a non-default box

As boxes installs an elisp (.el) file, shouldn't it be packaged according to...

http://fedoraproject.org/wiki/PackagingDrafts/EmacsenAddOns?highlight=%28emacs%29

(In reply to comment #2)
> http://hrozkovi.cz/boxes-1.1-2.fc7.src.rpm

Most issues are fixed. In line 45, you should replace '%{_prefix}/bin' 
with '%{_bindir}'.

Some minor remarks:
- There's a full stop missing after the %description.
- No need to put a '/' before %{_prefix} or similar paths.
- Default permissions for directories are not set in %defattr (fourth arg).
- The patch file contains a whitespace change.
- The patch file is named 'fix_smp_flags' but also comments the strip command 
from the Makefile.

> The only thing I could not reproduce was the error message " Alleged 
> system-wide config file '/usr/share/boxes' is a directory" .. could 
> you please retry? I've tested basic functionality of boxes on F7 i386
> and rawhide x86_64 and on both arches it runs fine...

Using your latest specfile, the package runs fine on F7 i386 and F7 x86_64.

(The problem lies in the Makefile: The sed command to patch src/boxes.h is 
only executed if there's something newer than src/boxes.h. So, changing 
GLOBALCONF via the make commandline only doesn't have any effect unless that 
sed command is forced to be executed, e.g. by touching or patching the 
Makefile, as you do now.)

(In reply to comment #4)
> As boxes installs an elisp (.el) file, shouldn't it be packaged according 
to...
> 
> 
http://fedoraproject.org/wiki/PackagingDrafts/EmacsenAddOns?highlight=%28emacs%29
> 

You mean this section:
http://fedoraproject.org/wiki/Packaging/Emacs#head-9ca98ccf54aba75a0bbda73be9e4706d61155d08

Following this would indeed require creating two subpackages, named 
boxes-emacs and boxes-emacs-el.

New packages:

http://hrozkovi.cz/boxes.spec
http://hrozkovi.cz/boxes-1.1-3.fc7.src.rpm

I tried to address the issues pointed out in comment #5, hope everything is 
packaged according to the Guidelines now..
> Some minor remarks:
> - There's a full stop missing after the %description.
> - No need to put a '/' before %{_prefix} or similar paths.
> - Default permissions for directories are not set in %defattr (fourth arg).
> - The patch file contains a whitespace change.
The above should be fixed.
> - The patch file is named 'fix_smp_flags' but also comments the strip 
command from the Makefile.
I've renamed the patch and added a comment to the specfile describing what it 
really does. 

After some thinking, I decided that it would be wisest to drop the Emacs 
helper for now. I don't use Emacs myself, so I don't understand half of the 
terms used in the Emacs guidelines and frankly, I'm not all that much willing 
to learn how Emacs and its add-ons work, not to mention lisp. Maintaining 
something you don't use is bad. 

(In reply to comment #7)
> I tried to address the issues pointed out in comment #5, hope everything is 
> packaged according to the Guidelines now..

Looks good to me.

> After some thinking, I decided that it would be wisest to drop the Emacs 
> helper for now. I don't use Emacs myself, so I don't understand half of the 
> terms used in the Emacs guidelines and frankly, I'm not all that much 
willing 
> to learn how Emacs and its add-ons work, not to mention lisp. Maintaining 
> something you don't use is bad. 

Fair enough!

As far as I understand, the aforementioned packaging guidelines don't allow 
this, but: some packages install single .el files as part of the %doc's. This 
way, the user is freed at least from downloading the whole package himself in 
order to get the latest version of that emacs file.

At this point, an 'official' reviewer should have a look at the package, I 
suppose.

I;l try to review this. Technically everything sees fine, but I have a problem
with the license[s]:
- upstream's website and the README included in the tarball claim GPLv2
- almost all sources are GPLv2+
- some sources (2 or three) include the following construction:
 *	Copyright (c) 1986 by University of Toronto.
 *	Written by Henry Spencer.  Not derived from licensed software.
 *
 *	Permission is granted to anyone to use this software for any
 *	purpose on any computer system, and to redistribute it freely,
 *	subject to the following restrictions:
 *
 *	1. The author is not responsible for the consequences of use of
 *		this software, no matter how awful, even if they arise
 *		from defects in it.
 *
 *	2. The origin of this software must not be misrepresented, either
 *		by explicit claim or by omission.
 *
 *	3. Altered versions must be plainly marked as such, and must not
 *		be misrepresented as being the original software.
 *

 This looks like a variant of MIT to me, but I would like a confirmation and
suitability before proceeding

(In reply to comment #10)

>  *	Copyright (c) 1986 by University of Toronto.
>  *	Written by Henry Spencer.  Not derived from licensed software.
>  *
>  *	Permission is granted to anyone to use this software for any
>  *	purpose on any computer system, and to redistribute it freely,
>  *	subject to the following restrictions:
>  *
>  *	1. The author is not responsible for the consequences of use of
>  *		this software, no matter how awful, even if they arise
>  *		from defects in it.
>  *
>  *	2. The origin of this software must not be misrepresented, either
>  *		by explicit claim or by omission.
>  *
>  *	3. Altered versions must be plainly marked as such, and must not
>  *		be misrepresented as being the original software.
>  *
> 
>  This looks like a variant of MIT to me, but I would like a confirmation and
> suitability before proceeding

Indeed it looks like MIT/BSD, but is not exactly any of these 
license. It seems to be acceptable, even though there is an issue.
Indeed, the right to modify isn't explicitly granted. However
the point 3. implies that modifications are allowed, as long as 
they are marked as such. So I'd say that it is acceptable, but
upstream/author should be contacted.

(In reply to comment #10)

>  *	Copyright (c) 1986 by University of Toronto.
>  *	Written by Henry Spencer.  Not derived from licensed software.
>  *
>  *	Permission is granted to anyone to use this software for any
>  *	purpose on any computer system, and to redistribute it freely,
>  *	subject to the following restrictions:
>  *
>  *	1. The author is not responsible for the consequences of use of
>  *		this software, no matter how awful, even if they arise
>  *		from defects in it.
>  *
>  *	2. The origin of this software must not be misrepresented, either
>  *		by explicit claim or by omission.
>  *
>  *	3. Altered versions must be plainly marked as such, and must not
>  *		be misrepresented as being the original software.
>  *

IMO 1 is not a problem and 2 and 3 are the same as zlib
http://fedoraproject.org/wiki/Licensing
(GPL compatible)
http://www.gzip.org/zlib/zlib_license.html
So IMO this can be said as zlib.

It is not like the zlib, since the zlib license explictly allows for
alteration:

 Permission is granted to anyone to use this software for any purpose,
  including commercial applications, and to alter it and redistribute it
  freely, subject to the following restrictions:

 Pat, Mamoru, thank you for your advices.
 As Pat has said in comment #11, the 3rd sentence in the included license
implies that alteration is permitted so I assume there is no problem in treating
the license as being zlib.
 I am tempted to ask for the package to be released as GPLv2+ and have the
packager ask upstream for clarification + suggest him to use the zlib license
which is very close to what he has used. Would this approach be satisfactory in
order to include the package in Fedora ?

The files under the pseudo zlib license needs not to be
fully relicensed, since the license is compatible with the GPL
(and LGPL). If the binary is consituted by files under the
pseudo zlib license and the GPL it is under the GPL.

To tell the truth, I was not thinking about relicensing the files (because they
are compatible with GPL) but of the tag to be used in the spec. I am not sure
which one is correct: "GPLv2+ and zlib" or simply "GPLv2+" ?

Simply GPLv2+ if the files are linked together. See:

http://fedoraproject.org/wiki/Licensing/FAQ

Package Review for boxes-1.1-3
==============

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:x86_64
 [x] Rpmlint output: none
 [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.
 [!] License field in the package spec file matches the actual license.
     License type: %tag GPLv2; correct tag: GPLv2+
 [x] 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.
 [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: 1fd6e875e9b3c84f2a71f6df73d9eddb37d11c93
 [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 Packaging Guidelines.
 [-] 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} (or
$RPM_BUILD_ROOT).
 [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).
 [-] 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.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on:Devel/x86_64
 [x] Package should compile and build into binary rpms on all supported
architectures.
     Tested on:devel/x86_64 and i386
 [x] Package functions as described.
 [-] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.


=== Issues ===
1. the correct license tag is GPLv2+ instead of GPLv2

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

Please fix the license tag before uploading to CVS.

Thanks Thomas for pre-review and Patrice and Mamoru for license guidance.

Thanks for all the work done on this review!

New Package CVS Request
=======================
Package Name: boxes
Short Description: Draw any kind of box around some given text
Owners: jhrozek
Branches: F-7 F-8
InitialCC: none
Cvsextras Commits: yes

cvs done.

Built for rawhide - http://koji.fedoraproject.org/koji/buildinfo?buildID=30902

Package Change Request
======================
Package Name: boxes
New Branches: EL-4 EL-5
Owners: jhrozek

cvs done.