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

[Florian Schmaus <flow@gentoo.org>]

[-- Attachment #1.1.1: Type: text/plain, Size: 9888 bytes --]

On 13/06/2024 11.31, Ulrich Mueller wrote:
>>>>>> 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.

Switched to "..."

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

Fixed, thanks!


>> +#     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}).

My idea was to denote relative paths by not including an initial slash 
(/). This allows to write "${ED}/${_GREADME_REL_PATH}" without a 
duplicate slash in the middle. I find

${ED}/${_GREADME_REL_PATH}"

more readable when compared to

${ED}${_GREADME_REL_PATH}"

which seems to be in-line with 
https://mgorny.pl/articles/the-ultimate-guide-to-eapi-7.html#d-ed-root-eroot-no-longer-have-a-trailing-slash

But maybe I misunderstand what you are suggesting. You want

_GREADME_DOC_DIR="/usr/share/doc/${PF}"

instead of

_GREADME_DOC_DIR="usr/share/doc/${PF}"

right? Then I'd also have to change to ${ED}${_GREADME_REL_PATH}", to 
avoid the double slash. :(


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

Good catch, switched to [[ ${1} == --* ]].


>> +		case ${1} in
>> +			--append)
>> +				append=true
>> +				shift
>> +				;;
>> +		esac
>> +	done
>> +
>> +	if $append; then
> 
> Please use the conventional coding style, i.e. ${append}.

Changed to [[ -n ${append} ]]


> 
>> +		if [[ ! -f "${_GREADME_TMP_FILE}" ]]; then
> 
> Quotation marks inside [[ ]] are generally not necessary (also several
> times below).

Dropped quotation marks inside [[ ]] everywhere.


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

This was inspired by readme.gentoo-r1.eclass, which does

	( # subshell to avoid pollution of calling environment
		docinto .
		dodoc "${T}"/README.gentoo
	) || die

I guess we could also switch to docinto & dodoc, but since we already 
need and have _GREADME_DOC_DIR, I went with that. Also the number of 
lines doesn't get any less.

However, if you feel strongly about it, then I am happy to switch to 
docinto & dodoc.


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

I did a last-minute change to that, which accidentally did not make it 
into the patch. This actually is now

# @ECLASS_VARIABLE: GREADME_SHOW
# @DEFAULT_UNSET
# @DESCRIPTION:
# If set to "yes" then unconditionally show the contents of the readme
# file in pkg_postinst via elog. If set to "no", then do not show the
# contents of the readme file, even if they have changed.
...
if [[ -v GREADME_SHOW ]]; then
	case ${GREADME_SHOW} in
		yes)
			_GREADME_SHOW="forced"
			;;
		no)
			_GREADME_SHOW=""
			;;
		*)
			die "Invalid argument of GREADME_SHOW"
			;;
	esac
	return
fi


where I would argue that the [[ -v ]] test is sensible.


>> +		_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).

Fair point. Fixed.

Thanks for your review. Adjusted accordingly 
(https://gitlab.com/flow/flow-s-ebuilds/-/commit/ef307612a676c7810e823ff6e38dac41bebd9dde).

- Flow


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 17797 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 618 bytes --]