From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by finch.gentoo.org (Postfix) with ESMTPS id 6F08E15817D for ; Thu, 13 Jun 2024 09:31:29 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id CBB6CE2AD8; Thu, 13 Jun 2024 09:31:24 +0000 (UTC) Received: from smtp.gentoo.org (woodpecker.gentoo.org [IPv6:2001:470:ea4a:1:5054:ff:fec7:86e4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 7E6E0E2A5F for ; Thu, 13 Jun 2024 09:31:24 +0000 (UTC) From: Ulrich Mueller To: Florian Schmaus Cc: gentoo-dev@lists.gentoo.org, pacho@gentoo.org Subject: Re: [gentoo-dev] [PATCH v3 1/1] greadme.eclass: new eclass In-Reply-To: <20240613083928.165509-2-flow@gentoo.org> (Florian Schmaus's message of "Thu, 13 Jun 2024 10:39:25 +0200") References: <20240613083928.165509-1-flow@gentoo.org> <20240613083928.165509-2-flow@gentoo.org> User-Agent: Gnus/5.13 (Gnus v5.13) Date: Thu, 13 Jun 2024 11:31:19 +0200 Message-ID: Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-dev@lists.gentoo.org Reply-to: gentoo-dev@lists.gentoo.org X-Auto-Response-Suppress: DR, RN, NRN, OOF, AutoReply MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" X-Archives-Salt: f51b6917-92a0-41b4-ba15-5bbedeb30dd9 X-Archives-Hash: c8ab0eee2edebf11727b70779c9be131 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable >>>>> 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 > +# @AUTHOR: > +# Author: Florian Schmaus > +# @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() { > +# =E2=80=A6 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 "=E2=80=A6" 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 > +# =E2=80=A6 > +# if use foo; then > +# greadme_stdin --apend <<-EOF Typo? > +# This is conditional readme content, based on USE=3Dfoo. > +# 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=3D1 > + > +case ${EAPI} in > + 8) ;; > + *) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;; > +esac > + > +_GREADME_FILENAME=3D"README.gentoo" > +_GREADME_TMP_FILE=3D"${T}/${_GREADME_FILENAME}" > +_GREADME_DOC_DIR=3D"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=3D"${_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=3Dfalse > + while [[ -n ${1} ]] && [[ ${1} =3D~ --* ]]; do $ [[ foo-bar =3D~ --* ]]; echo $? 0 This is not what is intended here, I guess? > + case ${1} in > + --append) > + append=3Dtrue > + 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: > +# @DESCRIPTION: > +# Installs the provided file as readme doc. > +greadme_file() { > + debug-print-function ${FUNCNAME} "${@}" > + > + local input_doc_file=3D"${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=3D"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=3D"forced" > + return > + fi > + > + local image_greadme_file=3D"${ED}/${_GREADME_REL_PATH}" > + if [[ ! -f "${image_greadme_file}" ]]; then > + # No README file was created by the ebuild. > + _GREADME_SHOW=3D"" > + return > + fi > + > + check_live_doc_file() { > + local cur_pvr=3D$1 > + local live_greadme_file=3D"${EROOT}/usr/share/doc/${PN}-${cur_pvr}/${_= GREADME_FILENAME}" > + > + if [[ ! -f ${live_greadme_file} ]]; then > + _GREADME_SHOW=3D"no-current-greadme" > + return > + fi > + > + cmp -s "${live_greadme_file}" "${image_greadme_file}" > + local ret=3D$? > + case ${ret} in > + 0) > + _GREADME_SHOW=3D"" > + ;; > + 1) > + _GREADME_SHOW=3D"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 conte= nts. > + 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=3D"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 --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQFDBAEBCAAtFiEEtDnZ1O9xIP68rzDbUYgzUIhBXi4FAmZqvGcPHHVsbUBnZW50 b28ub3JnAAoJEFGIM1CIQV4ukPwH/2VM60tHMD3Ia3BUhUTVhXrgYb5FZ6s6L/l1 wZmLyJXV4DoaXMJ46WSjmDrNVjhaClgF+ozXOwE7vSJndJV84NYQVxvU3VMqD0wr ZLz7VllyE6EI4r8hl6qW+eA4DvOAlJxHwR5J9kf59zfsbG/kniiacYClem+wfHJy rEkbSREDrlxNo2pha2GglvzsFlRd6c2Lj+zUfC8UtgTGOfj9PL/JGiJ+FgnXvBKm bTLNy9cEXzn49qmDus+JhDmUM++noyxPdOQJmYVEuMyEgxs3Pega8xRlm59wSQ9y NfU26YC4NB22aQQabY0VCHA/fCZ9y6Hqn/O+SM1UhbZQM5NziXU= =txZP -----END PGP SIGNATURE----- --=-=-=--