Re: [gentoo-dev] [PATCH v3 1/1] greadme.eclass: new eclass

[Ulrich Mueller <ulm@gentoo.org>]

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

>>>>> On Thu, 13 Jun 2024, Florian Schmaus wrote:

> +# Copyright 1999-2024 Gentoo Authors
> +# Distributed under the terms of the GNU General Public License v2
> +
> +# @ECLASS: greadme.eclass
> +# @MAINTAINER:
> +# Florian Schmaus <flow@gentoo.org>
> +# @AUTHOR:
> +# Author: Florian Schmaus <flow@gentoo.org>
> +# @SUPPORTED_EAPIS: 8
> +# @BLURB: install a doc file, that will be conditionally shown via elog messages
> +# @DESCRIPTION:
> +# An eclass for installing a README.gentoo doc file with important
> +# information for the user.  The content of README.gentoo will shown be
> +# via elog messages either on fresh installations or if the contents of
> +# the file have changed.  Furthermore, the README.gentoo file will be
> +# installed under /usr/share/doc/${PF} for later consultation.
> +#
> +# This eclass was inspired by readme.gentoo-r1.eclass.  The main
> +# differences are as follows.  Firstly, it only displays the doc file
> +# contents if they have changed (unless GREADME_FORCE_SHOW is set).
> +# Secondly, it provides a convenient API to install the doc file via
> +# stdin.
> +#
> +# @CODE
> +# inherit greadme
> +#
> +# src_install() {
> +#   …

I'm not a big fan of using non-ASCII characters in places where they
aren't strictly necessary. Is there any particular advantage of "…"
over "..."? The latter is also less awkwardly spaced in teletype fonts.

> +#   greadme_stdin <<-EOF
> +#   This is the content of the created readme doc file.
> +#   EOF
> +#   …
> +#   if use foo; then
> +#     greadme_stdin --apend <<-EOF

Typo?

> +#     This is conditional readme content, based on USE=foo.
> +#     EOF
> +#   fi
> +# }
> +# @CODE
> +#
> +# If the ebuild overrides the default pkg_preinst or respectively
> +# pkg_postinst, then it must call greadme_pkg_preinst and
> +# greadme_pkg_postinst explicitly.
> +
> +if [[ -z ${_GREADME_ECLASS} ]]; then
> +_GREADME_ECLASS=1
> +
> +case ${EAPI} in
> +	8) ;;
> +	*) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
> +esac
> +
> +_GREADME_FILENAME="README.gentoo"
> +_GREADME_TMP_FILE="${T}/${_GREADME_FILENAME}"
> +_GREADME_DOC_DIR="usr/share/doc/${PF}"

It is somewhat unusual to call insinto or docompress with a relative
path. I'd use "/usr/share/doc/${PF}" here.

> +_GREADME_REL_PATH="${_GREADME_DOC_DIR}/${_GREADME_FILENAME}"

Why must this be a relative path? AFAICS it could be an absolute path in
everywhere it is used. (You even add an explicit slash in places like
${ED}/${_GREADME_REL_PATH}).

> +
> +# @ECLASS_VARIABLE: GREADME_FORCE
> +# @DEFAULT_UNSET
> +# @DESCRIPTION:
> +# If set, then uncondiionally show the contents of the readme file in
> +# pkg_postinst via elog.
> +
> +# @FUNCTION: greadme_stdin
> +# @USAGE: [--append]
> +# @DESCRIPTION:
> +# Create the readme doc via stdin.  You can use --append to append to an
> +# existing readme doc.
> +greadme_stdin() {
> +	debug-print-function ${FUNCNAME} "${@}"
> +
> +	local append=false
> +	while [[ -n ${1} ]] && [[ ${1} =~ --* ]]; do

$ [[ foo-bar =~ --* ]]; echo $?
0

This is not what is intended here, I guess?

> +		case ${1} in
> +			--append)
> +				append=true
> +				shift
> +				;;
> +		esac
> +	done
> +
> +	if $append; then

Please use the conventional coding style, i.e. ${append}.

Policy reference:
https://projects.gentoo.org/qa/policy-guide/ebuild-format.html#pg0101

> +		if [[ ! -f "${_GREADME_TMP_FILE}" ]]; then

Quotation marks inside [[ ]] are generally not necessary (also several
times below).

> +			die "Gentoo README does not exist when trying to append to it"
> +		fi
> +
> +		cat >> "${_GREADME_TMP_FILE}" || die
> +	else
> +		if [[ -f "${_GREADME_TMP_FILE}" ]]; then
> +			die "Gentoo README already exists while trying to create it"
> +		fi
> +
> +		cat > "${_GREADME_TMP_FILE}" || die
> +	fi
> +
> +	_greadme_install_doc
> +}
> +
> +# @FUNCTION: greadme_file
> +# @USAGE: <file>
> +# @DESCRIPTION:
> +# Installs the provided file as readme doc.
> +greadme_file() {
> +	debug-print-function ${FUNCNAME} "${@}"
> +
> +	local input_doc_file="${1}"
> +	if [[ -z "${input_doc_file}" ]]; then
> +		die "No file specified"
> +	fi
> +
> +	if [[ -f "${_GREADME_TMP_FILE}" ]]; then
> +		die "Gentoo README already exists"
> +	fi
> +
> +	cp "${input_doc_file}" "${_GREADME_TMP_FILE}" || die
> +
> +	_greadme_install_doc
> +}
> +
> +# @FUNCTION: _greadme_install_doc
> +# @INTERNAL
> +# @DESCRIPTION:
> +# Installs the readme file from the temp directory into the image.
> +_greadme_install_doc() {
> +	debug-print-function ${FUNCNAME} "${@}"
> +
> +	# Subshell to avoid pollution of calling environment.
> +	(
> +		insinto "${_GREADME_DOC_DIR}"
> +		doins "${_GREADME_TMP_FILE}"
> +	)

Why not a simple dodoc here?

> +
> +	# Exclude the readme file from compression, so that its contents can
> +	# be easily compared.
> +	docompress -x "${_GREADME_REL_PATH}"
> +}
> +
> +# @FUNCTION: greadme_pkg_preinst
> +# @DESCRIPTION:
> +# Performs checks like comparing the readme doc from the image with a
> +# potentially existing one in the live system.
> +greadme_pkg_preinst() {
> +	debug-print-function ${FUNCNAME} "${@}"
> +
> +	if [[ -z ${REPLACING_VERSIONS} ]]; then
> +		_GREADME_SHOW="fresh-install"
> +		return
> +	fi
> +
> +	if [[ -v GREADME_FORCE_SHOW ]]; then

This is technically valid, but I think it would be surprising when empty
GREADME_FORCE_SHOW triggered the behaviour. I suggest testing for
[[ -n ${GREADME_FORCE_SHOW} ]] instead; this is also how most other
eclasses perform such tests.

> +		_GREADME_SHOW="forced"
> +		return
> +	fi
> +
> +	local image_greadme_file="${ED}/${_GREADME_REL_PATH}"
> +	if [[ ! -f "${image_greadme_file}" ]]; then
> +		# No README file was created by the ebuild.
> +		_GREADME_SHOW=""
> +		return
> +	fi
> +
> +	check_live_doc_file() {
> +		local cur_pvr=$1
> +		local live_greadme_file="${EROOT}/usr/share/doc/${PN}-${cur_pvr}/${_GREADME_FILENAME}"
> +
> +		if [[ ! -f ${live_greadme_file} ]]; then
> +			_GREADME_SHOW="no-current-greadme"
> +			return
> +		fi
> +
> +		cmp -s "${live_greadme_file}" "${image_greadme_file}"
> +		local ret=$?
> +		case ${ret} in
> +			0)
> +				_GREADME_SHOW=""
> +				;;
> +			1)
> +				_GREADME_SHOW="content-differs"
> +				;;
> +			*)
> +				die "cmp failed with ${ret}"
> +				;;
> +		esac
> +	}
> +
> +	local replaced_version
> +	for replaced_version in ${REPLACING_VERSIONS}; do
> +		check_live_doc_file ${replaced_version}
> +
> +		# Once _GREADME_SHOW is non empty, we found a reason to show the
> +		# readme and we can abort the loop.
> +		if [[ -n ${_GREADME_SHOW} ]]; then
> +			break
> +		fi
> +	done
> +}
> +
> +# @FUNCTION: greadme_pkg_postinst
> +# @DESCRIPTION:
> +# Conditionally shows the contents of the readme doc via elog.
> +greadme_pkg_postinst() {
> +	debug-print-function ${FUNCNAME} "${@}"
> +
> +	if [[ ! -v _GREADME_SHOW ]]; then
> +		die "_GREADME_SHOW not set. Did you call greadme_pkg_preinst?"
> +	fi
> +
> +	if [[ -z "${_GREADME_SHOW}" ]]; then
> +		# If _GREADME_SHOW is empty, then there is no reason to show the contents.
> +		return
> +	fi
> +
> +	local line
> +	while read -r line; do elog "${line}"; done < "${EROOT}/${_GREADME_REL_PATH}"

It is not guaranteed that the file exists in ${EROOT} at this point.
See FEATURES="nodoc" in make.conf(5).

> +	elog ""
> +	elog "(Note: Above message is only printed the first time package is"
> +	elog "installed or if the the message changed. Please look at"
> +	elog "${EPREFIX}/${_GREADME_REL_PATH} for future reference)"
> +}
> +
> +fi
> +
> +EXPORT_FUNCTIONS pkg_preinst pkg_postinst

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