Bug 830581

I'll go ahead and take this review.

ftp://ftp.nohats.ca/jove/jove-4.16.0.73-2.fc16.src.rpm

Just so fedora-review picks up the new version properly:

Spec URL: ftp://ftp.nohats.ca/jove/jove.spec
SRPM URL: ftp://ftp.nohats.ca/jove/jove-4.16.0.73-2.fc16.src.rpm

I'd like to get some clarification on the license for this package.  It looks free, and it's listed as BSD in the spec, but it's unusual and I want to make sure we're classifying it correctly.

The following license is at the top of the README and all of the source files:

##########################################################################
# This program is Copyright (C) 1986-2002 by Jonathan Payne.  JOVE is    #
# provided by Jonathan and Jovehacks without charge and without          #
# warranty.  You may copy, modify, and/or distribute JOVE, provided that #
# this notice is included in all the source files and documentation.     #
##########################################################################

It's not BSD, but it is okay. Use License: Copyright only

Lifting FE-Legal.

Thanks Tom!

What follows is the formal review of the package, generated in part by fedora-review.

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

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== C/C++ ====
[x]: MUST Header files in -devel subpackage, if present.
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Package is not relocatable.


==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
     Note: Spot has confirmed we should be using the "Copyright only" license tag
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files section. This is OK if packaging
     for EPEL5. Otherwise not needed
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[-]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST 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.
     Note: License is included at the top of the README file
[!]: MUST License field in the package spec file matches the actual license.
     Note: The license should be changed from BSD to Copyright only
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint output is silent.

rpmlint jove-4.16.0.73-2.fc18.src.rpm

jove.src: W: spelling-error %description -l en_US emacs -> Emacs, macs, maces
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint jove-4.16.0.73-2.fc18.i686.rpm

jove.i686: W: spelling-error %description -l en_US emacs -> Emacs, macs, maces
1 packages and 0 specfiles checked; 0 errors, 1 warnings.


rpmlint jove-debuginfo-4.16.0.73-2.fc18.i686.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Note: These spelling errors are OK.

[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
/home/rich/tmp/jove4.16.0.73.tgz :
  MD5SUM this package     : d5e826bf05fc1fbe1b7f10571156c735
  MD5SUM upstream package : d5e826bf05fc1fbe1b7f10571156c735

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[ ]: SHOULD 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]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[!]: SHOULD Spec use %global instead of %define.
     Note: %define configflags SYSDEFS="-DSYSVR4 -D_XOPEN_SOURCE=500
     -DIPROC_TERM='\\"TERM=vanilla\\"'" NROFF="nroff -Tascii" TROFF=groff
     TROFFPOST=""

Issues:

* License should be changed in spec
* Should use %global instead of %define for configflags
* Don't need BuildRoot or defattr if you're not planning on building for el5

Once the license is changed I can approve this package.

Spec URL: ftp://ftp.nohats.ca/jove/jove.spec
SRPM URL: ftp://ftp.nohats.ca/jove/jove-4.16.0.73-3.fc16.src.rpm

All issues mentioned fixed

Great, then this package is APPROVED

New Package SCM Request
=======================
Package Name: jove
Short Description: Jonathan's Own Version of Emacs
Owners: pwouters
Branches: f17 el6
InitialCC: pwouters

Git done (by process-git-requests).

On CentOS 5.8:
    $ rpmbuild --rebuild jove-4.16.0.73-3.fc16.src.rpm
    Installing jove-4.16.0.73-3.fc16.src.rpm
    warning: user paul does not exist - using root
    warning: group paul does not exist - using root
    error: unpacking of archive failed on file
    /repo5/hugh/rpmbuilds.kernel-2.6.18-274.7.1.el5/SPECS/jove.spec;5006c962: cpio: MD5 sum mismatch
    error: jove-4.16.0.73-3.fc16.src.rpm cannot be installed

But it works fine on Fedora 16.  So I guess that rpm now has a different hash but CentOS 5.8 hasn't caught up.

I think that all of Paul's changes are in the jove.spec file.

I (the upstream maintainer) would like to align your spec file with the one in the jove source.  That doesn't need to happen now.

I like at least some of the comments that have been deleted: they help for older or non Red Hat systems.  For example. I don't know what Suse uses for a jove.spec file, or even if they still include it, but they did at one time.

Why did you rename README to README.doc?

I think that the empty file $RPM_BUILD_ROOT/%{_datadir}/jove/keychart. is intentional.

You removed a bunch of %doc thingees.  I think that the result is that these files are missing from the RPM:

-rw-r--r--. 1 root root    716 Mar 15  1994 example.rc
-rw-r--r--. 1 root root 268054 Jan  3  2012 jove.man
-rw-r--r--. 1 root root 524611 Jan  3  2012 jove.man.ps
-rw-r--r--. 1 root root   5558 Dec 18  1994 jove.qref
-rw-r--r--. 1 root root   2992 Jan 21  1995 jove.rc

They are needed.

correct, rhel5 and fedora up to f12 or so uses md5, After that, it uses sha1.

I renamed doc/README to doc/README.doc because there is also a README file we include in the root dir of the package, and I cannot include both README's under the same file name

I re-instated the empty keychart file

I did not include jove.man/jove.man.ps because we have 'man jove'

The jove.rc is part of the package and is the default rc file, so I did not package it again as a document element. For the same reason I left out example.rc

[paul@thinkpad jove]$ rpm -ql jove |grep rc
/usr/share/jove/XTermresource
/usr/share/jove/jove.rc
/usr/share/jove/jove.rc.3022
/usr/share/jove/jove.rc.sun
/usr/share/jove/jove.rc.sun-cmd
/usr/share/jove/jove.rc.vt100
/usr/share/jove/jove.rc.wyse
/usr/share/jove/jove.rc.xterm
/usr/share/jove/jove.rc.z29

How are these "needed"? Nothing inside %doc should be needed as per package guidelines. What is needed is shipped in /usr/share/jove/

jove(1) talks about invoking JOVE.

jove.man (plain text) and jove.man.ps (postscript) are a complete manual for JOVE.  For example, all commands are described.

jove.qref is a "quick reference guide" for jove commands and variables.

You are right that /usr/share/jove/jove.rc is the same as /usr/share/doc/jove/jove.rc and can be considered redundant.  example.rc is simpler but nothing depends on it so it could be left out without harm.

/usr/share/jove/jove.rc* are not within %doc.  
The near the end of jove.rc is the line:
  1 source jove.rc.$TERM
This causes one of the other jove.rc files to be run, depending on the setting of the environment variable TERM.  That's how those files are used.

jove-4.16.0.73-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/jove-4.16.0.73-4.fc17

jove-4.16.0.73-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/jove-4.16.0.73-4.el6

jove-4.16.0.73-4.el6 has been pushed to the Fedora EPEL 6 testing repository.

Thanks, Paul.

I don't have a Fedora 17 system to test this on so I've installed it on a CentOS 6.3 system.  All is well after fairly light testing.

jove-4.16.0.73-4.fc17 has been pushed to the Fedora 17 stable repository.

jove-4.16.0.73-4.el6 has been pushed to the Fedora EPEL 6 stable repository.