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 <f...@gentoo.org> +# @AUTHOR: +# Author: Florian Schmaus <f...@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 <<-EOFTypo?
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; thenPlease use the conventional coding style, i.e. ${append}.
Changed to [[ -n ${append} ]]
+ if [[ ! -f "${_GREADME_TMP_FILE}" ]]; thenQuotation 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 ]]; thenThis 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
OpenPGP_0x8CAC2A9678548E35.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
