Bug 379751

Summary: Review Request: emacs-common-ess - Emacs Speaks Statistics add-on package for Emacs
Product: [Fedora] Fedora Reporter: Alex Lancaster <alexl>
Component: Package ReviewAssignee: Jason Tibbitts <tibbs>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, notting, petersen, tibbs
Target Milestone: ---Flags: tibbs: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-11-19 21:15:55 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Alex Lancaster 2007-11-13 06:59:22 EST
Spec URL: http://alexlan.fedorapeople.org/reviews/emacs-common-ess.spec
SRPM URL: http://alexlan.fedorapeople.org/reviews/emacs-common-ess-5.3.6-1.fc7.src.rpm
Description: (ESS) is an add-on package for GNU Emacs and XEmacs.  
It provides Emacs-based front ends for popular statistics packages. ESS interfaces with S-PLUS, R, SAS, BUGS and other statistical analysis packages.

(Still a few minor rpmlint issues, but otherwise should be ready to review).
Comment 1 Jens Petersen 2007-11-14 02:08:24 EST
I still think the prefix in the Emacs packaging guidelines should be changed
from emacs-common to a single word like emacs.
Comment 2 Jason Tibbitts 2007-11-17 23:17:24 EST
The packages do end up with "a single word like emacs".  Specifically, "emacs".
 The data that's shared between the emacs and xemacs packages is called
emacs-common.  I don't see the naming of the source package to be a real issue
since users don't see it, but if the guidelines had be presented suggesting that
the SRPM be named emacs-foo regardless of which emacs variant the package
supports then I wouldn't have objected although I suppose that would look odd in
the case of an xemacs-only package.

Seems to build OK for me, although I saw complaints about not being to find any
version of R installed.  Will the built packages properly support R if it's not
in the buildroot?

Not that it matters, but why not use %{pkgname} in the Summary, too?

The install-info dependencies aren't quite right.  If you use something in
%post, you need Requires(post). 
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets should have a template
for installing texinfo files.

rpmlint says:

  emacs-common-ess.noarch: W: file-not-utf8 
   /usr/share/doc/emacs-common-ess-5.3.6/ChangeLog.lisp
  emacs-common-ess.noarch: W: file-not-utf8 
   /usr/share/doc/emacs-common-ess-5.3.6/doc/TODO
  emacs-common-ess.noarch: W: file-not-utf8 
   /usr/share/doc/emacs-common-ess-5.3.6/ChangeLog
These should be passed through iconv.

  emacs-ess.noarch: W: no-documentation
  emacs-ess-el.noarch: W: no-documentation
  xemacs-ess.noarch: W: no-documentation
  xemacs-ess-el.noarch: W: no-documentation
I don't think these are problematic.

  emacs-ess.noarch: E: non-executable-script 
   /usr/share/emacs/site-lisp/ess/etc/backbugs.sparc 0644
  emacs-ess.noarch: E: non-executable-script 
   /usr/share/emacs/site-lisp/ess/etc/ess-sas-sh-command 0644
  emacs-ess.noarch: E: non-executable-script 
   /usr/share/emacs/site-lisp/ess/etc/backbug5 0644
  emacs-ess.noarch: E: non-executable-script 
   /usr/share/emacs/site-lisp/ess/etc/backbug5.sparc 0644
  emacs-ess.noarch: E: script-without-shebang 
   /usr/share/emacs/site-lisp/ess/etc/BACKBUGS.BAT
I'm not really sure what the point of these files (and the rest of the ess/etc
directory) is.  Certainly the BAT files have no real place on Linux.  Are any of
them actually relevant?  What will call them?
Comment 3 Alex Lancaster 2007-11-18 06:15:02 EST
(In reply to comment #2)

> Seems to build OK for me, although I saw complaints about not being to find any
> version of R installed.  Will the built packages properly support R if it's not
> in the buildroot?

Builds OK in koji without R

http://koji.fedoraproject.org/koji/taskinfo?taskID=246902

I then also downloaded the builds and tested them locally and editing R worked
as did the interactive R mode.

> Not that it matters, but why not use %{pkgname} in the Summary, too?

Done.
 
> The install-info dependencies aren't quite right.  If you use something in
> %post, you need Requires(post). 
> http://fedoraproject.org/wiki/Packaging/ScriptletSnippets should have a template
> for installing texinfo files.

Fixed.

> rpmlint says:
> 
>   emacs-common-ess.noarch: W: file-not-utf8 
>    /usr/share/doc/emacs-common-ess-5.3.6/ChangeLog.lisp
>   emacs-common-ess.noarch: W: file-not-utf8 
>    /usr/share/doc/emacs-common-ess-5.3.6/doc/TODO
>   emacs-common-ess.noarch: W: file-not-utf8 
>    /usr/share/doc/emacs-common-ess-5.3.6/ChangeLog
> These should be passed through iconv.

Fixed

>   emacs-ess.noarch: W: no-documentation
>   emacs-ess-el.noarch: W: no-documentation
>   xemacs-ess.noarch: W: no-documentation
>   xemacs-ess-el.noarch: W: no-documentation
> I don't think these are problematic.
> 
>   emacs-ess.noarch: E: non-executable-script 
>    /usr/share/emacs/site-lisp/ess/etc/backbugs.sparc 0644
>   emacs-ess.noarch: E: non-executable-script 
>    /usr/share/emacs/site-lisp/ess/etc/ess-sas-sh-command 0644
>   emacs-ess.noarch: E: non-executable-script 
>    /usr/share/emacs/site-lisp/ess/etc/backbug5 0644
>   emacs-ess.noarch: E: non-executable-script 
>    /usr/share/emacs/site-lisp/ess/etc/backbug5.sparc 0644
>   emacs-ess.noarch: E: script-without-shebang 
>    /usr/share/emacs/site-lisp/ess/etc/BACKBUGS.BAT

These should be fixed now.

> I'm not really sure what the point of these files (and the rest of the ess/etc
> directory) is.  Certainly the BAT files have no real place on Linux.  Are any of
> them actually relevant?  What will call them?

Some of these files are needed at runtime (they are called out by the Lisp code)
such as backbugs.  I've removed all the unnecessary files such as the BAT files,
and others which shouldn't have been installed in the first place (now in %doc),
leaving just the files that the Lisp package appears to actually use.

So here are the updated files:

* Sun Nov 18 2007 <alexlan@fedoraproject.org> - 5.3.6-2
- Moved all non-code related files in etc/ to documentation directory
- Make sure all doc files are UTF-8
- Fix post, preun scriptlets and add Requires for installing info
  files

Spec URL: http://alexlan.fedorapeople.org/reviews/emacs-common-ess.spec
SRPM URL:
http://alexlan.fedorapeople.org/reviews/emacs-common-ess-5.3.6-2.fc7.src.rpm
Comment 4 Jason Tibbitts 2007-11-18 15:07:05 EST
rpmlint is down to just the four no-documentation complaints, which are OK. 
Everything looks good to me.

* source files match upstream:
   9531c53e534550924680862e10c04eadd9c049c66a1fc0b9df940c1f966c7a12  
   ess-5.3.6.tgz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* Emacs packaging guidelines look to be followed properly.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly
* rpmlint has acceptable complaints
* final provides and requires are sane:
  emacs-common-ess-5.3.6-2.fc9.noarch.rpm
   emacs-common-ess = 5.3.6-2.fc9
  =
   /bin/sh
   /sbin/install-info

  emacs-ess-5.3.6-2.fc9.noarch.rpm
   emacs-ess = 5.3.6-2.fc9
  =
   /bin/sh
   emacs(bin) >= 22.1
   emacs-common-ess = 5.3.6-2.fc9

  emacs-ess-el-5.3.6-2.fc9.noarch.rpm
   emacs-ess-el = 5.3.6-2.fc9
  =
   emacs-ess = 5.3.6-2.fc9

  xemacs-ess-5.3.6-2.fc9.noarch.rpm
   xemacs-ess = 5.3.6-2.fc9
  =
   /bin/sh
   emacs-common-ess = 5.3.6-2.fc9
   xemacs(bin) >= 21.5.28

  xemacs-ess-el-5.3.6-2.fc9.noarch.rpm
   xemacs-ess-el = 5.3.6-2.fc9
  =
   xemacs-ess = 5.3.6-2.fc9

* %check is not present; no test suite upstream.  Frankly I've no idea how to 
   test this so I'll trust the maintainer.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets are OK (texinfo index generation)
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.

APPROVED
Comment 5 Alex Lancaster 2007-11-18 17:02:17 EST
New Package CVS Request
=======================
Package Name: emacs-common-ess
Short Description: Emacs Speaks Statistics add-on package for Emacs
Owners: alexlan
Branches: F-7 F-8
Cvsextras Commits: yes
Comment 6 Alex Lancaster 2007-11-18 20:29:52 EST
Actually can you modify that to add Tom Moertel (tmoertel)?  New request is:

New Package CVS Request
=======================
Package Name: emacs-common-ess
Short Description: Emacs Speaks Statistics add-on package for Emacs
Owners: alexlan, tmoertel
Branches: F-7 F-8
Cvsextras Commits: yes
Comment 7 Jens Petersen 2007-11-19 00:12:03 EST
(In reply to comment #2)
> The packages do end up with "a single word like emacs". Specifically, "emacs".
> The data that's shared between the emacs and xemacs packages is called
> emacs-common.  I don't see the naming of the source package to be a real issue
> since users don't see it,

Well that is what appears in bugzilla, the buildsys, and other places,
so IMHO it is actually pretty important - specially as emacs-common-foo
packages start to proliferate.

> but if the guidelines had be presented suggesting that
> the SRPM be named emacs-foo regardless of which emacs variant the package
> supports then I wouldn't have objected although I suppose that would look odd
> in the case of an xemacs-only package.

I think xemacs only packages should be named xemacs-foo.

Anyway looks like we already have one more emacs-common-foo... :-/
Comment 8 Jens Petersen 2007-11-19 00:29:52 EST
(In reply to comment #6)
> Actually can you modify that to add Tom Moertel (tmoertel)?  New request is:

Just noticed by chance that tmoertel still needs a sponsor.
Comment 9 Alex Lancaster 2007-11-19 00:58:01 EST
(In reply to comment #8)
> (In reply to comment #6)
> > Actually can you modify that to add Tom Moertel (tmoertel)?  New request is:
> 
> Just noticed by chance that tmoertel still needs a sponsor.

OK, maybe revert my package request to the original and I can add him as
co-maintainer after he's sponsored:

New Package CVS Request
=======================
Package Name: emacs-common-ess
Short Description: Emacs Speaks Statistics add-on package for Emacs
Owners: alexlan
Branches: F-7 F-8
Cvsextras Commits: yes
Comment 10 Kevin Fenzi 2007-11-19 11:35:18 EST
cvs done.
Comment 11 Alex Lancaster 2007-11-19 21:15:55 EST
Builds done in devel/rawhide.   Pending pushes in F-7, F-8.  Closing bug.