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).
I still think the prefix in the Emacs packaging guidelines should be changed from emacs-common to a single word like emacs.
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?
(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> - 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
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
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
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
(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... :-/
(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.
(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
cvs done.
Builds done in devel/rawhide. Pending pushes in F-7, F-8. Closing bug.