On 21/07/2024 10.26, Ulrich Mueller wrote:
>>>>>> On Tue, 16 Jul 2024, Florian Schmaus wrote:
> 
>> Notes:
>>      - also show readme contents if FEATURES=nodoc
> 
> Good. :)

Thanks for your review.


>> --- /dev/null
>> +++ b/eclass/greadme.eclass
>> @@ -0,0 +1,257 @@
>> +# 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>
> 
> "Author:" is redundant when there's only a single author.

Applied locally.


>> +# @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_SHOW is set).
>> +# Secondly, it provides a convenient API to install the doc file via
>> +# stdin.
>> +#
>> +# @CODE
>> +# inherit greadme
>> +#
>> +# src_install() {
>> +#   ...
>> +#   greadme_stdin <<-EOF
>> +#   This is the content of the created readme doc file.
>> +#   EOF
>> +#   ...
>> +#   if use foo; then
>> +#     greadme_stdin --append <<-EOF
>> +#     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_REL_PATH="/usr/share/doc/${PF}/${_GREADME_FILENAME}"
> 
> These are internal variables and they're never set to anything else.
> I also think that it is unlikely that "README.gentoo" would ever be
> changed to anything else.
> 
> IMHO hardcoding it would improve readability. ("README.gentoo" makes it
> immediately clear what is happening, while with ${_GREADME_FILENAME} one
> must look up what the variable is.)

Applied locally.


>> +# @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.
>> +
>> +# @ECLASS_VARIABLE: GREADME_AUTOFORMATTING_DISABLED
> 
> It is an input variable, so make it a (grammatical) command, e.g.
> GREADME_DISABLE_AUTOFORMAT?

Applied locally.


>> +# @DEFAULT_UNSET
>> +# @DESCRIPTION:
>> +# If non-empty, the readme file will not be automatically formatted.
>> +
>> +# @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
>> +	while [[ -n ${1} ]] && [[ ${1} == --* ]]; do
>> +		case ${1} in
>> +			--append)
>> +				append=1
>> +				shift
>> +				;;
>> +		esac
>> +	done
> 
> Simply check for either no option, or one option that must be exactly
> "--append". Everything else should be an error, i.e. not silent ignoring
> of parameters. Something like this:
> 
> 	[[ ${1} = --append ]] && { append=1; shift; }
> 	[[ $# -eq 0 ]] || die "${FUNCNAME[0]}: Bad parameters: $*"

Applied locally.


>> +
>> +	if [[ -n ${append} ]]; then
>> +		if [[ ! -f ${_GREADME_TMP_FILE} ]]; then
>> +			die "Gentoo README does not exist when trying to append to it"
>> +		fi
>> +
>> +		cat >> "${_GREADME_TMP_FILE}" || die
>> +	else
>> +		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
>> +
>> +	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} "${@}"
>> +
>> +	local greadme="${_GREADME_TMP_FILE}"
>> +	if [[ ! "${GREADME_AUTOFORMATTING_DISABLED}" ]]; then
> 
> Quotation marks are not necessary here.

Applied locally.



>> +		greadme="${_GREADME_TMP_FILE}".formatted
>> +
>> +		# Use fold, followed by a sed to strip trailing whitespace.
>> +		# https://bugs.gentoo.org/460050#c7
>> +		fold -s -w 70 "${_GREADME_TMP_FILE}" |
>> +			sed 's/[[:space:]]*$//' > "${greadme}"
>> +		assert "failed to autoformat ${_GREADME_FILENAME}"
>> +	fi
>> +
>> +	# Subshell to avoid pollution of calling environment.
>> +	(
>> +		docinto .
>> +		newdoc "${greadme}" "${_GREADME_FILENAME}"
>> +	)
>> +
>> +	# Exclude the readme file from compression, so that its contents can
>> +	# be easily compared.
>> +	docompress -x "${_GREADME_REL_PATH}"
>> +
>> +	# Save the contents of the of the readme. Unfortunately we have to
>> +	# do this in src_* phase, because FEATURES=nodoc is applied right
>> +	# after src_install and not after pkg_preinst.
>> +	_GREADME_CONTENT=$(< "${greadme}")
>> +}
>> +
>> +# @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_SHOW ]]; then
>> +		case ${GREADME_SHOW} in
>> +			yes)
>> +				_GREADME_SHOW="forced"
>> +				;;
>> +			no)
>> +				_GREADME_SHOW=""
>> +				;;
>> +			*)
>> +				die "Invalid argument of GREADME_SHOW: ${GREADME_SHOW}"
>> +				;;
>> +		esac
>> +		return
>> +	fi
>> +
>> +	local image_greadme_file="${ED}${_GREADME_REL_PATH}"
>> +	if [[ ! -f ${image_greadme_file} ]]; then
>> +		if [[ -v _GREADME_CONTENT ]]; then
>> +			# There is no greadme in the image but the ebuild created
>> +			# one. This is likely because FEATURES=nodoc is active.
>> +			# Unconditionally show greadme's contents.
>> +			_GREADME_SHOW="nodoc-active"
>> +		else
>> +			# No README file was created by the ebuild.
>> +			_GREADME_SHOW=""
>> +		fi
>> +
>> +		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
> 
> You could check for $? directly here, without the need for the
> intermediate "ret" variable.

I usually try to save $? as soon as possible, so that the saved value is 
used in later processing and not clobbered by something that sets $? 
later on. This does not seem to possible here. Therefore: applied 
proposed changed locally.


>> +			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.
> 
> Having more than one element in REPLACING_VERSIONS is certainly a corner
> case, but I still wonder about the logic. Shouldn't it be the other way
> around, i.e. if there is _any_ empty ${_GREADME_SHOW} then there is an
> identical file installed that has been shown before, i.e. the file
> should _not_ be shown again?

I would argue the other way around: if there is a non-identical file 
installed, then it should be shown.

But as you said, it's a corner case. I wouldn't worry about it until we 
hit it and gained experience what the best behavior in this situation 
is. Then we can change the code accordingly.

Thanks again for your review.

- Flow