From: Florian Schmaus <flow@gentoo.org>
To: Ulrich Mueller <ulm@gentoo.org>
Cc: gentoo-dev@lists.gentoo.org
Subject: Re: [gentoo-dev] [PATCH v5] greadme.eclass: new eclass
Date: Tue, 23 Jul 2024 10:15:27 +0200 [thread overview]
Message-ID: <06bee5df-0362-4112-b61e-419df181a562@gentoo.org> (raw)
In-Reply-To: <umsmb6th1@gentoo.org>
[-- Attachment #1.1.1: Type: text/plain, Size: 9227 bytes --]
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
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 21567 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 618 bytes --]
next prev parent reply other threads:[~2024-07-23 8:15 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-16 9:25 [gentoo-dev] [PATCH v5] greadme.eclass: new eclass Florian Schmaus
2024-07-21 8:26 ` Ulrich Mueller
2024-07-23 8:15 ` Florian Schmaus [this message]
2024-07-23 10:55 ` Ulrich Mueller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=06bee5df-0362-4112-b61e-419df181a562@gentoo.org \
--to=flow@gentoo.org \
--cc=gentoo-dev@lists.gentoo.org \
--cc=ulm@gentoo.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox