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

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

[Florian Schmaus]

Following up on the discussion of the last patchset, this
- moves the functionally into a new eclass, as adjusting the existing
  eclass to export new phase functions is not viable.
- excludes the README.gentoo from decompression, as all other
  presented approaches add complexity and cause additional disk space
  consumption. While on the other hand, README.gentoo files are
  typically very small because they should be suitable as pkg_postinst
  output, so ther is often not much gained by compressing them.
- adds a GREADME_SHOW show option, to manually override the behavior
  (as requested by ionen).

Note that I choose greadme.eclass as the name for the new eclass for
aesthetic reasons. I find readme.gentoo-r2 a mouthful, and it leads to
an ugly combination of dot, hyphen, and underscores. However, if
anyone wants to have function names like
'readme.gentoo-r2_pkg_postinst', then we can go with that.

Florian Schmaus (1):
  greadme.eclass: new eclass

 eclass/greadme.eclass | 223 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 223 insertions(+)
 create mode 100644 eclass/greadme.eclass

-- 
2.44.2

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

[Florian Schmaus]

This new eclass includes various improvements over the existing
readme.gentoo-r1.eclass.

First, the content of README.gento will only be shown on new
installations or if it has changed between updates.

Second, it allows the composition of readme via bash's heredoc.

Third, it exports phase functions, which helps to reduce the boilerplate
code in many cases.

Finally, unlike readme.gentoo-r1.elcass, this eclass does not need to
store the content of the readme in an environment variable. Not having
to store the content in an environment variable reduces the pollution of
the environment (sadly, this only refers to the process environment).

Signed-off-by: Florian Schmaus <flow@gentoo.org>
---
 eclass/greadme.eclass | 223 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 223 insertions(+)
 create mode 100644 eclass/greadme.eclass

diff --git a/eclass/greadme.eclass b/eclass/greadme.eclass
new file mode 100644
index 000000000000..f26e939df206
--- /dev/null
+++ b/eclass/greadme.eclass
@@ -0,0 +1,223 @@
+# 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() {
+#   …
+#   greadme_stdin <<-EOF
+#   This is the content of the created readme doc file.
+#   EOF
+#   …
+#   if use foo; then
+#     greadme_stdin --apend <<-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_DOC_DIR="usr/share/doc/${PF}"
+_GREADME_REL_PATH="${_GREADME_DOC_DIR}/${_GREADME_FILENAME}"
+
+# @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
+		case ${1} in
+			--append)
+				append=true
+				shift
+				;;
+		esac
+	done
+
+	if $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
+		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}"
+	)
+
+	# 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
+		_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}"
+	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
-- 
2.44.2

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

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 7800 bytes --]

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

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

Typo?

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

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

> +		case ${1} in
> +			--append)
> +				append=true
> +				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: <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?

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

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 507 bytes --]

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

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

>>>>> On Thu, 13 Jun 2024, Ulrich Mueller wrote:

>> +	if $append; then

> Please use the conventional coding style, i.e. ${append}.

Or rather, assign the variable to empty or non-empty and test for
[[ -n ${append} ]] instead of executing it as a true or false command.

(I'm pretty sure that there was a previous discussion on this, but I
fail to find it.)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 507 bytes --]

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

[Florian Schmaus]

[-- 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 --]

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

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 3704 bytes --]

>>>>> On Thu, 13 Jun 2024, Florian Schmaus wrote:

>>> +_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}"

I think the variable would be renamed in that case, i.e.
${ED}${_GREADME_PATH} or ${ED}${_GREADME_ABS_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

This has examples like ${D}${EPREFIX}/usr/share/foo: All path variables
start with a slash, _don't_ end with a slash, and no slash between them
when concatenating.

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

Correct.

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

I see no problem with this.

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

I don't have any strong argument, dodoc just feels more approriate when
installing documentation.

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

I'd still argue that empty should behave the same way as unset, but at
least with the explicit die there's no bad surprise for an empty value.

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

Well, it still won't display anything with FEATURES=nodoc. IIUC
readme.gentoo-r1.eclass works around the problem by saving the file
contents in an environment variable. (However, I see the problem that
even then you couldn't compare old vs new file contents.)

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

Ulrich

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 507 bytes --]

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

[Florian Schmaus]

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

On 13/06/2024 12.42, Ulrich Mueller wrote:
>>>>>> On Thu, 13 Jun 2024, Florian Schmaus wrote:
>>>> +_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}"
> 
> I think the variable would be renamed in that case, i.e.
> ${ED}${_GREADME_PATH} or ${ED}${_GREADME_ABS_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
> 
> This has examples like ${D}${EPREFIX}/usr/share/foo: All path variables
> start with a slash, _don't_ end with a slash, and no slash between them
> when concatenating.
> 
>> 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?
> 
> Correct.
> 
>> Then I'd also have to change to ${ED}${_GREADME_REL_PATH}", to avoid
>> the double slash. :(
> 
> I see no problem with this.

Will change accordingly then.


>>>> +	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.
> 
> Well, it still won't display anything with FEATURES=nodoc. IIUC
> readme.gentoo-r1.eclass works around the problem by saving the file
> contents in an environment variable. (However, I see the problem that
> even then you couldn't compare old vs new file contents.)

I'd assume this is readme.gentoo-r1.eclass working around the readme 
file being compressed, and by doing so, coincidentally being able to 
display the readme even if FEATURES=nodoc.

It appears we have now two options:
A) Just like readme.gentoo-r1, store the content in an environment
    variable , to be able to show the content (unconditionally) in case
    of FEATURES=nodoc
B) Live with the fact that we will not be able to show the content on
    FEATURES=nodoc (but document this accordingly).

I have no strong opinion, therefore I am happy about hearing peoples 
thoughts on this.

- 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 --]

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

[Ionen Wolkens]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On Thu, Jun 13, 2024 at 10:39:24AM +0200, Florian Schmaus wrote:
> Following up on the discussion of the last patchset, this
> - moves the functionally into a new eclass, as adjusting the existing
>   eclass to export new phase functions is not viable.
> - excludes the README.gentoo from decompression, as all other
>   presented approaches add complexity and cause additional disk space
>   consumption. While on the other hand, README.gentoo files are
>   typically very small because they should be suitable as pkg_postinst
>   output, so ther is often not much gained by compressing them.
> - adds a GREADME_SHOW show option, to manually override the behavior
>   (as requested by ionen).

I don't recall requesting anything, or was it something i said on
IRC that I forgot about? On the ML, just talked a bit about an
implementation possibility that wouldn't use the live files (plus
its perk of better control like not showing it on minor style/space
changes).

Also I assume GREADME_SHOW is actually GREADME_FORCE?

Not that I really looked closely at this yet, just replying because
I was mentioned.
-- 
ionen

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Florian Schmaus]

On 13/06/2024 13.59, Ionen Wolkens wrote:
> On Thu, Jun 13, 2024 at 10:39:24AM +0200, Florian Schmaus wrote:
>> Following up on the discussion of the last patchset, this
>> - moves the functionally into a new eclass, as adjusting the existing
>>    eclass to export new phase functions is not viable.
>> - excludes the README.gentoo from decompression, as all other
>>    presented approaches add complexity and cause additional disk space
>>    consumption. While on the other hand, README.gentoo files are
>>    typically very small because they should be suitable as pkg_postinst
>>    output, so ther is often not much gained by compressing them.
>> - adds a GREADME_SHOW show option, to manually override the behavior
>>    (as requested by ionen).
> 
> I don't recall requesting anything, or was it something i said on
> IRC that I forgot about?

No, that seems to be a misunderstanding then. You wrote about the 
maintainer being able to choose when the readme is worth showing again 
and that a comparison would also "display it for minor style or typo
fixes.".

I thought that this was asking for a manual override. I am sorry, seems 
that I got that wrong.


> Also I assume GREADME_SHOW is actually GREADME_FORCE?

It's actually the other way around. Latest HEAD of greadme.eclass has

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

- Flow