Re: [gentoo-dev] New package neomutt

[Nicolas Bock <nicolasbock@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 7790 bytes --]

On Thu, Aug 10, 2017 at 09:40:30AM +0200, Michał Górny wrote:
>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.

I changed the ebuild such that it can be installed together with 
mutt without having to use symlinks or eselet.

>> # 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.

I split the ebuild into two so that this conditional is unnecessary.

>> 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.

Fixed, I reverted that change.

>> 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.

Fixed.

>>
>> 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.

The current version of ncurses works, so I removed the version on it. I
also sorted the dependencies.

>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

Fixed

>> 		"--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.

Up until now upstream was using `mutt` but there is work in progress to
rename it to `neomutt`. I think that there is a legitimate use case for
installing `mutt` and `neomutt` at the same time.

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

Fixed.

>> 	)
>>
>> 	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?

No, and as I don't have access to such a machine I can't test it.

>>
>> 	# 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.

This was a copy-and-paste leftover. I have removed it. I don't know what
it's supposed to mean either.

>> 	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.

Good point. I have removed the conditional. I also removed the default
configuration files.

>>
>> 	econf "${myconf[@]}"
>> }
>>
>> src_install() {
>> 	emake DESTDIR="${D}" install
>> 	if use mbox; then
>> 		insinto /etc/${PN}
>
>You can move the insinto above the 'if'.

I have removed this part.

>> 		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.

Fixed.

>>
>> 	# 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?

No, fixed.

>> 		# 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?

True, I changed the link to point to neomutt.

>> 			-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.

Fixed.

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

-- 
Nicolas Bock <nicolasbock@gentoo.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 981 bytes --]