Bug 470547 (emacs-semi)

Summary: Review Request: emacs-semi - MIME rendering library for Emacs
Product: [Fedora] Fedora Reporter: Vitaly Mayatskikh <vmayatsk>
Component: Package ReviewAssignee: Jonathan Underwood <jonathan.underwood>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fatkasuvayu, fedora-package-review, itamar, jonathan.underwood, leamas.alec, msuchy, notting, petersen, rgs
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-03-15 05:04:56 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449, 470545    

Description Vitaly Mayatskikh 2008-11-07 11:40:00 EST
Spec URL: http://people.redhat.com/vmayatsk/wl/semi.spec
SRPM URL: http://people.redhat.com/vmayatsk/wl/semi-1.14.6-1.fc10.src.rpm
RPM URL: http://people.redhat.com/vmayatsk/wl/semi-1.14.6-1.fc10.noarch.rpm

SEMI is a library to provide MIME feature for GNU Emacs.  MIME is a
proposed internet standard for including content and headers other
than (ASCII) plain text in messages.

Wanderlust mail client requires this library to render messages.

SEMI + Wanderlust are my first packages for Fedora.
Comment 1 Jason Tibbitts 2008-11-07 14:24:38 EST
Is there a dependency between this and Wanderlust?  If so, one of these tickets should block the other.
Comment 2 Vitaly Mayatskikh 2008-11-07 14:38:29 EST
Yes, Wanderlust requires SEMI.
Comment 3 Alec Leamas 2008-11-16 03:15:50 EST

Unfortunately, I'm not a reviewer... But according to the instructions, I need to show some interest in reviewing other requests in order to get  a sponsor. So I'll do that. Please feel free to do the same for me, my request is bug 471575 :-)

For me, rpmlint gives the following

semi.src: E: no-buildroot-tag
semi.src: W: mixed-use-of-spaces-and-tabs (spaces: line 15, tab: line 2)
semi.src: W: non-standard-group Unspecified
semi.src: W: invalid-license GPL

My mock build bails out, complaining about the missing Group: field.

I think all of these issues should be closed. 

Copyright & license. Most (all?) files have a nice GPLv2 copyright notice. However, the I really miss the top-level file COPYING - the
notices refer to this. I think it should be part of the package.

See more below

> #%define _default_patch_fuzz 2
> %define	_semiver	1.14.6
> %define	_flimver	1.14.8
> %define	_emacsver	22.2
> %define	_lispdir	%{_datadir}/emacs/site-lisp
> Summary: Library to provide MIME feature for GNU Emacs
> Name: semi
> Version: %{_semiver}
> Release: 1%{?dist}
> License: GPL
> #Group: Applications/Internet

As lint says, there need to be a valid group  and license tag. As for license, see http://fedoraproject.org/wiki/Licensing - I think it boils down to GPLv2. For Group:, take a look at http://koti.welho.com/vskytta/packagers-handbook/packagers-handbook.html#guidelines-group-tag

> URL: ftp://ftp.m17n.org/pub/mule/semi/semi-1.14-for-flim-1.14
> Source0:        ftp://ftp.m17n.org/pub/mule/semi/semi-1.14-for-flim-1.14/semi-%{version}.tar.bz2

Unfortunately, these are password protected.

> BuildRequires: emacs >= %{_emacsver}, flim >= %{_flimver}
> BuildArch: noarch
> Requires: emacs >= %{_emacsver}, flim >= %{_flimver}
> Patch1: semi-001-use-w3m-instead-of-w3.patch
> %description
> SEMI is a library to provide MIME feature for GNU Emacs.  MIME is a
> proposed internet standard for including content and headers other than
> (ASCII) plain text in messages

[nit-picking] This was written some time ago... Isn't it fair these days to say that MIME is the way to handle content on Internet?

> %prep
> %setup -q -n semi-%{version}
> %patch1 -p1
> # necessary to generate the auto-autoloads.el file:
> touch *.el
> %build
> %install
> rm -rf %buildroot
> %{__mkdir_p} %buildroot%{_lispdir}/semi
> cd $RPM_BUILD_DIR/semi-%{version}
> make LISPDIR=%buildroot%{_lispdir}
> make LISPDIR=%buildroot%{_lispdir} install
> make clean

Why make clean here? If all goes well, %clean will take care of it. If not, I think we want everything. Or am I missing something?

> %clean
> rm -rf %buildroot
> %files
> %defattr(-,root,root)
> %{_lispdir}/semi
> %changelog
> * Fri Nov  7 2008 Vitaly Mayatskikh <vmayatsk@redhat.com> [1.14.6-1]
> - first build


Comment 4 Alec Leamas 2008-11-16 03:35:58 EST
Reading the Review Guidelines once more, I realize that my proposal just to add the COPYING file  wasn't that good. What needs to be done is to try to get upstream to do this. But I don't know if it's feasible, and anyway I think you need advice by someone more experienced than me about this. I have already been wrong once :-)
Comment 5 Alec Leamas 2008-11-17 00:37:37 EST
#%define _default_patch_fuzz 2
> %define	_semiver	1.14.6
> %define	_flimver	1.14.8
> %define	_emacsver	22.2
> %define	_lispdir	%{_datadir}/emacs/site-lisp

You should not use _* as a macro name - these are by convention reserved for "system" macros. Use semiver, flimver, emacsver and lispdir instead. Sorry I  missed that.
Comment 6 Vitaly Mayatskikh 2008-11-21 08:44:48 EST
Thanks for your comments, Alec!
Comment 7 Jason Tibbitts 2009-07-14 14:32:38 EDT
Was an updated package ever released which addressed those comments?
Comment 8 Vitaly Mayatskikh 2009-07-14 15:13:39 EDT
Yes, half a year ago.

I've added one more patch to semi and updated packages.
Comment 9 Jason Tibbitts 2009-07-14 15:27:09 EDT
Where are the updated packages?  The spec URL is valid, but none of the package links exist.

Anyway, have you seen the emacs packaging guidelines?  From a quick inspection of the spec file, this package doesn't seem to follow them.  http://fedoraproject.org/wiki/Packaging:Emacs
Comment 11 Jens Petersen 2009-10-22 01:21:44 EDT
The spec file and package name should be the same.

I just note for the record that both semi and wl were
formerly in Fedora Core.
Comment 13 Jonathan Underwood 2010-04-05 20:22:26 EDT
Rebuild of packages in Comment #12 inside mock succeeds. rpmlint output on resulting rpms:

emacs-semi.noarch: W: incoherent-version-in-changelog [1.14.6-1] ['1.14.6-1.fc14', '1.14.6-1']

--> Needs fixing

emacs-semi.noarch: W: file-not-utf8 /usr/share/doc/emacs-semi-1.14.6/ChangeLog
emacs-semi.noarch: W: file-not-utf8 /usr/share/doc/emacs-semi-1.14.6/README.en
emacs-semi.noarch: W: file-not-utf8 /usr/share/doc/emacs-semi-1.14.6/VERSION

---> These need fixing using iconv in %prep

emacs-semi.noarch: W: empty-%post
emacs-semi.noarch: W: empty-%preun

---> Remove these sections

emacs-semi-el.noarch: W: spelling-error Summary(en_US) Elisp -> Lisp, Elise, Elisa
emacs-semi-el.noarch: W: spelling-error %description -l en_US elisp -> lisp, e lisp, Elise

---> False positives, safe to ignore

emacs-semi-el.noarch: W: no-documentation

---> False positive, ignore

3 packages and 0 specfiles checked; 0 errors, 9 warnings.
Comment 14 Jonathan Underwood 2010-04-05 20:29:14 EDT
Also, the spec file needs updating to comply with the latest Emacs add-on packaging guidelines. http://fedoraproject.org/wiki/Packaging:Emacs

Specifically, these changes need to be made to the spec file. 

1/ the pkgconfig stuff can be removed

2/the emacs specific macros need to be changed accordingly eg %{emacs_version} should now be %{_emacs_version} etc

3/ No need to buildrequire emacs-el

4/ Comments need adding to the spec file about the patches - have these been sent upstream? If so, supply a date, an email archive url or a bugzilla url.

5/ BuildRoot is no longer needed - remove.

6/ In install, remove the rm -rf $RPM_BUILD_ROOT

7/ Fix up the changelog entry to properly comply with the guidelines

Once these are done I'll finish the review.
Comment 16 Jason Tibbitts 2012-06-29 13:09:12 EDT
Can this just be closed out now?  It's been 26 months with no response.
Comment 17 Miroslav Suchý 2013-03-15 05:04:56 EDT
Stalled Review. Closing per #15, #16 and:
If you ever want to continue with this review, please reopen or
submit new review.