Bug 470547 - (emacs-semi) Review Request: emacs-semi - MIME rendering library for Emacs
Review Request: emacs-semi - MIME rendering library for Emacs
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jonathan Underwood
Fedora Extras Quality Assurance
Depends On:
Blocks: FE-DEADREVIEW emacs-wl
  Show dependency treegraph
Reported: 2008-11-07 11:40 EST by Vitaly Mayatskikh
Modified: 2013-03-15 05:04 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2013-03-15 05:04:56 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)

  None (edit)
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.

Note You need to log in before you can comment on or make changes to this bug.