Re: [gentoo-dev] New package neomutt

[Michał Górny]

On czw, 2017-08-10 at 06:58 +0200, Nicolas Bock wrote:
> On Mon, Jul 31, 2017 at 09:11:19AM +0200, Nicolas Bock wrote:
> > Hi,
> >
> > I would like to add neomutt to the tree. This new package is meant as
> > an alternative and not a replacement of the existing mutt package.
>
> Thanks for all of the great suggestions and feedback!
>
> This is round two. I have update the ebuild with all your
> suggestions. I have also added support for eselecting between mutt
> and neomutt. Before the eselect ebuild can land though, we need to
> rename the mutt binary so that the managed link can be called
> mutt.

What for? How many people are exactly in the dire need of having both
installed simultaneously and switching between them? If you really can't
learn to type the new command, add IUSE=symlink blocking original mutt
and be done with it. Don't add more unowned files to /usr by another
poorly written eselect module.

> # Copyright 1999-2017 Gentoo Foundation
> # Distributed under the terms of the GNU General Public License v2
>
> EAPI=6
>
> inherit autotools eutils flag-o-matic
>
> if [[ ${PV} == 99999999 ]] ; then
> 	# live ebuild
> 	inherit git-r3
> 	EGIT_REPO_URI="https://github.com/neomutt/neomutt.git"
> 	EGIT_CHECKOUT_DIR="${WORKDIR}/neomutt-${P}"
> 	KEYWORDS=""

This is going to confuse the hell out of ekeyword.

> else
> 	SRC_URI="https://github.com/${PN}/${PN}/archive/${P}.tar.gz"
> 	KEYWORDS="~amd64 ~x86"
> fi
>
> DESCRIPTION="Teaching an Old Dog New Tricks"

This doesn't tell anybody who doesn't know mutt what this is.

> HOMEPAGE="https://www.neomutt.org/"
>
> LICENSE="GPL-2"
> SLOT="0"
> IUSE="berkdb crypt debug doc gdbm gnutls gpg idn kerberos libressl mbox
> 	nls notmuch qdbm sasl selinux slang smime ssl tokyocabinet kyotocabinet
> 	lmdb"

Sort lexically.

>
> CDEPEND="
> 	app-eselect/eselect-mutt
> 	app-misc/mime-types
> 	nls? ( virtual/libintl )
> 	tokyocabinet?  ( dev-db/tokyocabinet )
> 	qdbm?  ( dev-db/qdbm )
> 	gdbm?  ( sys-libs/gdbm )
> 	berkdb? ( >=sys-libs/db-4:= )
> 	kyotocabinet? ( dev-db/kyotocabinet )
> 	lmdb? ( dev-db/lmdb )
> 	gnutls?  ( >=net-libs/gnutls-1.0.17 )
> 	!gnutls? (
> 		ssl? (
> 			!libressl? ( >=dev-libs/openssl-0.9.6:0 )
> 			libressl? ( dev-libs/libressl )
> 		)
> 	)

This is not a correct use of 'ssl' flag:

 global:ssl: Add support for Secure Socket Layer connections

It's supposed to go top-level, above any implementation flags.

> 	sasl?    ( >=dev-libs/cyrus-sasl-2 )
> 	kerberos? ( virtual/krb5 )
> 	idn?     ( net-dns/libidn )
> 	gpg?     ( >=app-crypt/gpgme-0.9.0 )
> 	smime?   (
> 		!libressl? ( >=dev-libs/openssl-0.9.6:0 )
> 		libressl? ( dev-libs/libressl )

What is the point of preferring gnutls when USE=smime pulls openssl
anyway?

> 	)
> 	notmuch? ( net-mail/notmuch )
> 	slang? ( sys-libs/slang )
> 	!slang? ( >=sys-libs/ncurses-5.2:0 )

Why not = slotop? ncurses definitely changed ABI in the past. It's
something you are supposed to use when needed, not when repoman
complains about it and you didn't accidentally workaround the check.

Sorting this would also help reviews.

> "
> DEPEND="${CDEPEND}
> 	net-mail/mailbase
> 	doc? (
> 		dev-libs/libxml2
> 		dev-libs/libxslt
> 		app-text/docbook-xsl-stylesheets
> 		|| ( www-client/lynx www-client/w3m www-client/elinks )
> 	)"
> RDEPEND="${CDEPEND}
> 	selinux? ( sec-policy/selinux-mutt )
> "
>
> S="${WORKDIR}/${PN}-${P}"
>
> src_prepare() {
> 	eapply "${FILESDIR}/0001-Rename-mutt-to-neomutt.patch"
> 	eapply_user
> 	AT_M4DIR="m4" eautoreconf
> }
>
> src_configure() {
> 	local myconf=(
> 		"$(use_enable crypt pgp)"
> 		"$(use_enable debug)"
> 		"$(use_enable doc)"
> 		"$(use_enable gpg gpgme)"
> 		"$(use_enable nls)"
> 		"$(use_enable smime)"
> 		"$(use_enable notmuch)"
> 		"$(use_with idn)"
> 		"$(use_with kerberos gss)"
> 		"$(use_with sasl)"
> 		"$(use_with tokyocabinet)"
> 		"$(use_with kyotocabinet)"
> 		"$(use_with qdbm)"
> 		"$(use_with gdbm)"
> 		"$(use_with berkdb bdb)"
> 		"$(use_with lmdb)"
> 		"--with-$(use slang && echo slang || echo curses)=${EPREFIX}/usr"

usex

> 		"--sysconfdir=${EPREFIX}/etc/${PN}"

I'd really prefer if you didn't abuse PN to construct paths, and make me
wonder if upstream really wants 'neomutt' or 'mutt' here.

> 		"--with-docdir=${EPREFIX}/usr/share/doc/${PN}-${PVR}"

PF?

> 	)
>
> 	if [[ ${CHOST} == *-solaris* ]] ; then
> 		# arrows in index view do not show when using wchar_t
> 		myconf+=( "--without-wc-funcs" )
> 	fi

Are you sure that this still applies?

>
> 	# there's no need for gnutls, ssl or sasl without socket support

What is this comment supposed to mean? Looks like copy-paste without
even reading it.

> 	if use gnutls; then
> 		myconf+=( "--with-gnutls" )
> 	elif use ssl; then
> 		myconf+=( "--with-ssl" )
> 	fi
>
> 	if use mbox; then
> 		myconf+=( "--with-mailpath=${EPREFIX}/var/spool/mail" )
> 	else
> 		myconf+=( "--with-homespool=Maildir" )
> 	fi

Would configuring both paths do any harm? Maybe it'd make easier for
user to switch without having to rebuild it.

>
> 	econf "${myconf[@]}"
> }
>
> src_install() {
> 	emake DESTDIR="${D}" install
> 	if use mbox; then
> 		insinto /etc/${PN}

You can move the insinto above the 'if'.

> 		newins "${FILESDIR}"/Muttrc.mbox Muttrc
> 	else
> 		insinto /etc/${PN}
> 		doins "${FILESDIR}"/Muttrc
> 	fi
>
> 	# A newer file is provided by app-misc/mime-types. So we link it.
> 	rm "${ED}"/etc/${PN}/mime.types || die
> 	dosym /etc/mime.types /etc/${PN}/mime.types

Don't use absolute symlinks.

>
> 	# A man-page is always handy, so fake one
> 	if use !doc; then
> 		emake -C doc DESTDIR="${D}" muttrc.man

Is DESTDIR really necessary here?

> 		# make the fake slightly better, bug #413405
> 		sed -e 's#@docdir@/manual.txt#http://www.mutt.org/doc/manual/#' \
> 			-e 's#in @docdir@,#at http://www.mutt.org/,#' \

You sure you want to link to the original mutt.org?

> 			-e "s#@sysconfdir@#${EPREFIX}/etc/${PN}#" \
> 			-e "s#@bindir@#${EPREFIX}/usr/bin#" \
> 			doc/mutt.man > neomutt.1 || die
> 		cp doc/muttrc.man neomuttrc.5 || die
> 		doman neomutt.1 neomuttrc.5
> 	else
> 		# nuke manpages that should be provided by an MTA, bug #177605
> 		rm "${ED}"/usr/share/man/man5/{mbox,mmdf}.5 \
> 			|| ewarn "failed to remove files, please file a bug"

die.

> 	fi
>
> 	dodoc COPYRIGHT ChangeLog* OPS* README*
> }
>

-- 
Best regards,
Michał Górny