[gentoo-dev] [PATCH v4] greadme.eclass: new eclass

[gentoo-dev] [PATCH v4] 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 | 240 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 240 insertions(+)
 create mode 100644 eclass/greadme.eclass

diff --git a/eclass/greadme.eclass b/eclass/greadme.eclass
new file mode 100644
index 000000000000..ddbcc8e9858d
--- /dev/null
+++ b/eclass/greadme.eclass
@@ -0,0 +1,240 @@
+# 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_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}"
+
+# @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.
+
+# @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
+
+	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
+		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.
+	(
+		docinto .
+		dodoc "${_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_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
+		# 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 greadme="${EROOT}${_GREADME_REL_PATH}"
+
+	if [[ ! -f ${greadme} ]]; then
+		ewarn "Unable to show ${_GREADME_FILENAME}, file not installed (FEATURES=nodoc enabled?)."
+		return
+	fi
+
+	local line
+	while read -r line; do elog "${line}"; done < "${greadme}"
+	elog ""
+	elog "(Note: Above message is only printed the first time package is"
+	elog "installed or if the message changes on update. 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 v4] greadme.eclass: new eclass

[Arthur Zamarin]

[-- Attachment #1.1: Type: text/plain, Size: 9550 bytes --]

On 16/06/2024 18.51, Florian Schmaus wrote:
> This new eclass includes various improvements over the existing
> readme.gentoo-r1.eclass.

So, some weird question from me - why is it called greadme? I can
understand why you don't want to modify existing eclass, but why not
call it "readme.gentoo-r2.eclass"? This should make it a little less
confusing (cause I imagine folks asking - which to use. With -r2 we all
know which one is better).

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

Is it common usage you think? I've never felt the need for that.

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

That actually sounds nice, but there is a caveat - if someone inherits
that eclass after other eclass phases, it might shadow those eclass
phase functions. There was a Check Request for pkgcheck to catch such
shadowed phase functions, but this still wasn't easy to implement, so we
still miss it. So I'm afraid this eclass being inherited last, causing
shadowed phases.

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

I'll be honest, I never felt this is really needed? From looking at the
current -r1 eclass, you could define DOC_CONTENTS just before invoking
readme.gentoo_create_doc, so you could for example modify as you want
the message and use `local DOC_CONTENTS="..."`.

I'm not saying directly I'm against, don't understand me incorrectly.
What I want to see a current way to use -r1 eclass, which is
bad/ugly/boilerplate, and the new way with your eclass which is nicer. A
nice thing I took from mgorny's eclass changes, is giving an example
diff for a package, showing the new usage, diff, and "win points".

> Signed-off-by: Florian Schmaus <flow@gentoo.org>
> ---
>  eclass/greadme.eclass | 240 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 240 insertions(+)
>  create mode 100644 eclass/greadme.eclass
> 
> diff --git a/eclass/greadme.eclass b/eclass/greadme.eclass
> new file mode 100644
> index 000000000000..ddbcc8e9858d
> --- /dev/null
> +++ b/eclass/greadme.eclass
> @@ -0,0 +1,240 @@
> +# 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_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}"
> +
> +# @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.
> +
> +# @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
> +
> +	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
> +		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.
> +	(
> +		docinto .
> +		dodoc "${_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_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
> +		# 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 greadme="${EROOT}${_GREADME_REL_PATH}"
> +
> +	if [[ ! -f ${greadme} ]]; then
> +		ewarn "Unable to show ${_GREADME_FILENAME}, file not installed (FEATURES=nodoc enabled?)."
> +		return
> +	fi
> +
> +	local line
> +	while read -r line; do elog "${line}"; done < "${greadme}"
> +	elog ""
> +	elog "(Note: Above message is only printed the first time package is"
> +	elog "installed or if the message changes on update. Please look at"
> +	elog "${EPREFIX}${_GREADME_REL_PATH} for future reference)"
> +}
> +
> +fi
> +
> +EXPORT_FUNCTIONS pkg_preinst pkg_postinst

-- 
Arthur Zamarin
arthurzam@gentoo.org
Gentoo Linux developer (Python, pkgcore stack, QA, Arch Teams, GURU)


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

[gentoo-dev] Re: [PATCH v4] greadme.eclass: new eclass

[Duncan]

Arthur Zamarin posted on Sun, 16 Jun 2024 21:15:25 +0300 as excerpted:

> On 16/06/2024 18.51, Florian Schmaus wrote:
>> This new eclass includes various improvements over the existing
>> readme.gentoo-r1.eclass.
> 
> So, some weird question from me - why is it called greadme? I can
> understand why you don't want to modify existing eclass, but why not
> call it "readme.gentoo-r2.eclass"? This should make it a little less
> confusing (cause I imagine folks asking - which to use. With -r2 we all
> know which one is better).

I had the same question but it was answered to my satisfaction in
[PATCH v3 0/1].  Quoting from that:

>>> [I]f anyone wants to have function names like
>>> 'readme.gentoo-r2_pkg_postinst', then we can go with that.

Convinced me!  greadme's /just/ fine, thankyou! =:^)

(Tho purely bikeshedding I'd prefer g2readme or gen2readme.  Which FWIW 
would match my gentoo bug shortcut g2b/g2bug...)

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass

[Florian Schmaus]

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

Hi Arthur,

thanks for your mail.

On 16/06/2024 20.15, Arthur Zamarin wrote:
> On 16/06/2024 18.51, Florian Schmaus wrote:
>> This new eclass includes various improvements over the existing
>> readme.gentoo-r1.eclass.
> 
> So, some weird question from me - why is it called greadme? I can
> understand why you don't want to modify existing eclass, but why not
> call it "readme.gentoo-r2.eclass"? This should make it a little less
> confusing (cause I imagine folks asking - which to use. With -r2 we all
> know which one is better).


It was already pointed out, but let me quote from my previous mail:

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

Personally, I think it is better to have a forward pointer from the old 
elcass to the new eclass via @DEPRECATED, compared to going with 
readme.gentoo-r2. But it is certainly not a hill I want to die on.


>> 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.
> 
> Is it common usage you think? I've never felt the need for that.

It can't be common usage, because it is not directly supported yet. :)

And yes, I feel the need for that. Short heredocs have the advantage 
over stuff in FILESDIR that one can not forget about cleaning up 
references files in FILESDIR.

Using heredocs also makes it easier to conditionally compose the 
contents of README.gentoo. See the eclass' @CODE example.

But yes, the same would be possible with by composing the content via a 
variable. In the end, it is probably subjective what you prefer.


>> Third, it exports phase functions, which helps to reduce the boilerplate
>> code in many cases.
> 
> That actually sounds nice, but there is a caveat - if someone inherits
> that eclass after other eclass phases, it might shadow those eclass
> phase functions. There was a Check Request for pkgcheck to catch such
> shadowed phase functions, but this still wasn't easy to implement, so we
> still miss it. So I'm afraid this eclass being inherited last, causing
> shadowed phases.

You are right, exported phase functions have their quirks in the 
multi-inherit case you describe.

But I am sorry, I am not sure how this is related to greadme exporting 
phase functions.

A dozen other eclasses also export phase functions. And this is, I 
presume, because even considering those quirks, exported phase functions 
are otherwise a sensible approach to reduce boilerplate.

In any case, if we want to decide that exporting phase functions is now 
an anti-pattern, then this would warrant a larger discussion and ideally 
some democratic vote.


>> 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).
> 
> I'll be honest, I never felt this is really needed? From looking at the
> current -r1 eclass, you could define DOC_CONTENTS just before invoking
> readme.gentoo_create_doc, so you could for example modify as you want
> the message and use `local DOC_CONTENTS="..."`.

readme.gentoo-r1.eclass requires DOC_CONTENTS to be part of the 
package's environment to show it later in readme.gentoo_print_elog(), 
which is typically invoked in pkg_postinst(). If DOC_CONTENTS is local 
to readme.gentoo_create_doc(), then it wont be able in pkg_postinst() 
and can potentially not be obtained from the README.gentoo file because 
that file may be compressed.

For greadme.eclass, the file is no longer compressed, therefore 
greadme.eclass does not need to carry a variable in the package's 
environment.


> I'm not saying directly I'm against, don't understand me incorrectly.
> What I want to see a current way to use -r1 eclass, which is
> bad/ugly/boilerplate, and the new way with your eclass which is nicer. A
> nice thing I took from mgorny's eclass changes, is giving an example
> diff for a package, showing the new usage, diff, and "win points".


While for me personally, being able to use heredoc is a major advantage 
in the eclass' API design. This, and the reduced boilerplate due to 
exported phase function are the major improvements API wise.

So one could argue that, while there are improvements in the API, they 
are not that dramatic.

The actual big "win point" of greadme.eclass is that the content of the 
README.gentoo will be shown on new installations or *if the content 
changed*.

That means, if README.gentoo changes, probably due to new, likely 
important, information being added, readme.gentoo-r1 will not show it 
again to the user, while greadme.eclass will.

For cases where the content changes only due to unimportant changes, 
like maybe spelling fixes, greadme.eclass provides a manual override of 
its default behavior.

- 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 v4] greadme.eclass: new eclass

[Ulrich Mueller]

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

>>>>> On Tue, 18 Jun 2024, Florian Schmaus wrote:

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

>> I'll be honest, I never felt this is really needed? From looking at
>> the current -r1 eclass, you could define DOC_CONTENTS just before
>> invoking readme.gentoo_create_doc, so you could for example modify as
>> you want the message and use `local DOC_CONTENTS="..."`.

> readme.gentoo-r1.eclass requires DOC_CONTENTS to be part of the
> package's environment to show it later in readme.gentoo_print_elog(),
> which is typically invoked in pkg_postinst(). If DOC_CONTENTS is local
> to readme.gentoo_create_doc(), then it wont be able in pkg_postinst()
> and can potentially not be obtained from the README.gentoo file
> because that file may be compressed.

> For greadme.eclass, the file is no longer compressed, therefore
> greadme.eclass does not need to carry a variable in the package's
> environment.

These are two different variables that must not be confused.
readme.gentoo-r1 has DOC_CONTENTS as an "input variable" that is
assigned by the ebuild. It creates the actual file from it (possibly
doing some automatic formatting).

The contents of the final file is then saved in another variable
README_GENTOO_DOC_VALUE, which is used in readme.gentoo_print_elog to
output the message.

BTW, I like readme.gentoo-r1's autoformatting, because the message may
contain variables (like paths containing EPREFIX) that can expand to
different lengths.

Ulrich

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

Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass

[Florian Schmaus]

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

On 18/06/2024 16.02, Ulrich Mueller wrote:
>>>>>> On Tue, 18 Jun 2024, Florian Schmaus wrote:
>>>> 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).
> 
>>> I'll be honest, I never felt this is really needed? From looking at
>>> the current -r1 eclass, you could define DOC_CONTENTS just before
>>> invoking readme.gentoo_create_doc, so you could for example modify as
>>> you want the message and use `local DOC_CONTENTS="..."`.
> 
>> readme.gentoo-r1.eclass requires DOC_CONTENTS to be part of the
>> package's environment to show it later in readme.gentoo_print_elog(),
>> which is typically invoked in pkg_postinst(). If DOC_CONTENTS is local
>> to readme.gentoo_create_doc(), then it wont be able in pkg_postinst()
>> and can potentially not be obtained from the README.gentoo file
>> because that file may be compressed.
> 
>> For greadme.eclass, the file is no longer compressed, therefore
>> greadme.eclass does not need to carry a variable in the package's
>> environment.
> 
> These are two different variables that must not be confused
 >[DOC_CONTENTS vs README_GENTOO_DOC_VALUE].

Thanks for pointing this out. I think I understand now what arthur is 
asking for:

src_install() {
     ...
     local DOC_CONTENTS="My README.Gentoo contents"
     readme.gentoo_create_doc
}

@arthur: is that right?

If so, then we could always add such an API to greadme.eclass too. 
However, it appears that it simply would duplicate what can already be 
done via greadme_stdin. Is there a compelling reason for such an API 
that I am missing?

In any case, I wouldn't be opposed to implement something like this if 
somebody asks for it.


> BTW, I like readme.gentoo-r1's autoformatting, because the message may
> contain variables (like paths containing EPREFIX) that can expand to
> different lengths.

Happy to add it.

Any preference regarding the auto-formatting tool? The 
readme.gentoo-r1.eclass uses fold, but fmt (both are in coreutils) would 
probably also be an option (and has a --uniform-spacing option ;)).


- 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 v4] greadme.eclass: new eclass

[Arthur Zamarin]

[-- Attachment #1.1: Type: text/plain, Size: 3397 bytes --]

On 18/06/2024 17.53, Florian Schmaus wrote:
> On 18/06/2024 16.02, Ulrich Mueller wrote:
>>>>>>> On Tue, 18 Jun 2024, Florian Schmaus wrote:
>>>>> 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).
>>
>>>> I'll be honest, I never felt this is really needed? From looking at
>>>> the current -r1 eclass, you could define DOC_CONTENTS just before
>>>> invoking readme.gentoo_create_doc, so you could for example modify as
>>>> you want the message and use `local DOC_CONTENTS="..."`.
>>
>>> readme.gentoo-r1.eclass requires DOC_CONTENTS to be part of the
>>> package's environment to show it later in readme.gentoo_print_elog(),
>>> which is typically invoked in pkg_postinst(). If DOC_CONTENTS is local
>>> to readme.gentoo_create_doc(), then it wont be able in pkg_postinst()
>>> and can potentially not be obtained from the README.gentoo file
>>> because that file may be compressed.
>>
>>> For greadme.eclass, the file is no longer compressed, therefore
>>> greadme.eclass does not need to carry a variable in the package's
>>> environment.
>>
>> These are two different variables that must not be confused
>>[DOC_CONTENTS vs README_GENTOO_DOC_VALUE].
> 
> Thanks for pointing this out. I think I understand now what arthur is
> asking for:
> 
> src_install() {
>     ...
>     local DOC_CONTENTS="My README.Gentoo contents"
>     readme.gentoo_create_doc
> }
> 
> @arthur: is that right?

yes, exactly. Please, I suggest going over the existing eclass, you
might get surprised how much is supported already.

> If so, then we could always add such an API to greadme.eclass too.
> However, it appears that it simply would duplicate what can already be
> done via greadme_stdin. Is there a compelling reason for such an API
> that I am missing?
> 
> In any case, I wouldn't be opposed to implement something like this if
> somebody asks for it.

I think you are looking at it from the wrong side. Thinking in this
"impl" possible now, I think *you* are duplicating work stuff which was
supported in readme.gentoo-r1. I don't see anything supportted by
greadme_stdin and unsupported with this `local DOC_CONTENTS` stuff.

What I'm trying to push you into, is understanding if you really need a
new eclass. With all of those things, I believe greadme eclass is just a
duplicate.

I think if there is a small thing you want to have in -r1 eclass, it is
already supported or easily added. The support for a $FILESDIR is
something I see more rare than direct DOC_CONTENTS (in global as common,
and as local as a possible way).

> 
>> BTW, I like readme.gentoo-r1's autoformatting, because the message may
>> contain variables (like paths containing EPREFIX) that can expand to
>> different lengths.
> 
> Happy to add it.
> 
> Any preference regarding the auto-formatting tool? The
> readme.gentoo-r1.eclass uses fold, but fmt (both are in coreutils) would
> probably also be an option (and has a --uniform-spacing option ;)).
> 
> 
> - Flow

-- 
Arthur Zamarin
arthurzam@gentoo.org
Gentoo Linux developer (Python, pkgcore stack, QA, Arch Teams, GURU)


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

Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass

[Ionen Wolkens]

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

On Tue, Jun 18, 2024 at 09:21:56PM +0300, Arthur Zamarin wrote:
> On 18/06/2024 17.53, Florian Schmaus wrote:
> > On 18/06/2024 16.02, Ulrich Mueller wrote:
> >>>>>>> On Tue, 18 Jun 2024, Florian Schmaus wrote:
> >>>>> 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).
> >>
> >>>> I'll be honest, I never felt this is really needed? From looking at
> >>>> the current -r1 eclass, you could define DOC_CONTENTS just before
> >>>> invoking readme.gentoo_create_doc, so you could for example modify as
> >>>> you want the message and use `local DOC_CONTENTS="..."`.
> >>
> >>> readme.gentoo-r1.eclass requires DOC_CONTENTS to be part of the
> >>> package's environment to show it later in readme.gentoo_print_elog(),
> >>> which is typically invoked in pkg_postinst(). If DOC_CONTENTS is local
> >>> to readme.gentoo_create_doc(), then it wont be able in pkg_postinst()
> >>> and can potentially not be obtained from the README.gentoo file
> >>> because that file may be compressed.
> >>
> >>> For greadme.eclass, the file is no longer compressed, therefore
> >>> greadme.eclass does not need to carry a variable in the package's
> >>> environment.
> >>
> >> These are two different variables that must not be confused
> >>[DOC_CONTENTS vs README_GENTOO_DOC_VALUE].
> > 
> > Thanks for pointing this out. I think I understand now what arthur is
> > asking for:
> > 
> > src_install() {
> >     ...
> >     local DOC_CONTENTS="My README.Gentoo contents"
> >     readme.gentoo_create_doc
> > }
> > 
> > @arthur: is that right?
> 
> yes, exactly. Please, I suggest going over the existing eclass, you
> might get surprised how much is supported already.
> 
> > If so, then we could always add such an API to greadme.eclass too.
> > However, it appears that it simply would duplicate what can already be
> > done via greadme_stdin. Is there a compelling reason for such an API
> > that I am missing?
> > 
> > In any case, I wouldn't be opposed to implement something like this if
> > somebody asks for it.
> 
> I think you are looking at it from the wrong side. Thinking in this
> "impl" possible now, I think *you* are duplicating work stuff which was
> supported in readme.gentoo-r1. I don't see anything supportted by
> greadme_stdin and unsupported with this `local DOC_CONTENTS` stuff.

fwiw I do think heredoc is "nicer", e.g. I could indent (thanks to the
- in <<-EOF, and no need for \ to keep alignment) nvidia's kind of ugly:

	local DISABLE_AUTOFORMATTING=yes
	local DOC_CONTENTS="\
Trusted users should be in the 'video' group to use NVIDIA devices.
You can add yourself by using: gpasswd -a my-user video\
$(usev modules "
<dynamic content based on USE>")
..."

Not that I think it's by any means necessary, and fwiw if I really
wanted this, don't even need a new function and could do:

	local DOC_CONTENTS
	read -r -d '' DOC_CONTENTS <<-EOF
		line1
		line2
	EOF


Not that it isn't ugly too.

> 
> What I'm trying to push you into, is understanding if you really need a
> new eclass. With all of those things, I believe greadme eclass is just a
> duplicate.
> 
> I think if there is a small thing you want to have in -r1 eclass, it is
> already supported or easily added. The support for a $FILESDIR is
> something I see more rare than direct DOC_CONTENTS (in global as common,
> and as local as a possible way).
> 
> > 
> >> BTW, I like readme.gentoo-r1's autoformatting, because the message may
> >> contain variables (like paths containing EPREFIX) that can expand to
> >> different lengths.
> > 
> > Happy to add it.
> > 
> > Any preference regarding the auto-formatting tool? The
> > readme.gentoo-r1.eclass uses fold, but fmt (both are in coreutils) would
> > probably also be an option (and has a --uniform-spacing option ;)).
> > 
> > 
> > - Flow
> 
> -- 
> Arthur Zamarin
> arthurzam@gentoo.org
> Gentoo Linux developer (Python, pkgcore stack, QA, Arch Teams, GURU)
> 




-- 
ionen

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

Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass

[Florian Schmaus]

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

On 18/06/2024 20.21, Arthur Zamarin wrote:
> On 18/06/2024 17.53, Florian Schmaus wrote:
>> Thanks for pointing this out. I think I understand now what arthur is
>> asking for:
>>
>> src_install() {
>>      ...
>>      local DOC_CONTENTS="My README.Gentoo contents"
>>      readme.gentoo_create_doc
>> }
>>
>> @arthur: is that right?
> 
> yes, exactly. Please, I suggest going over the existing eclass, you
> might get surprised how much is supported already.

Given that I have been working on this for some time, I became very 
familiar with readme.gentoo-r1.eclass' implementation.


>> If so, then we could always add such an API to greadme.eclass too.
>> However, it appears that it simply would duplicate what can already be
>> done via greadme_stdin. Is there a compelling reason for such an API
>> that I am missing?
>>
>> In any case, I wouldn't be opposed to implement something like this if
>> somebody asks for it.
> 
> I think you are looking at it from the wrong side. Thinking in this
> "impl" possible now, I think *you* are duplicating work stuff which was
> supported in readme.gentoo-r1. […]

Oh, greadme.eclass definitely duplicates functionality of 
readme.gentoo-r1.eclass…


> What I'm trying to push you into, is understanding if you really need a
> new eclass.

Yes, we have been there. Please look at the discussion in the thread 
"Improve readme.gentoo-r1.eclass" from two weeks ago, where it was 
attempted to do it without a new eclass.

The outcome was that here is no sensible way to implement all of the 
suggested behavior, that is now provided by greadme.eclass, into 
readme.gentoo-r1.eclass.

For this reason it became a new eclass. And again, I have no desire to 
bikeshed if the new eclass' name is greadme.eclass or 
readme.gentoo-r2.eclass. However, I do have a preference towards 
greadme.eclass, for the reasons explained before.

- 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 v4] greadme.eclass: new eclass

[Ulrich Mueller]

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

>>>>> On Tue, 18 Jun 2024, Florian Schmaus wrote:

> Any preference regarding the auto-formatting tool? The
> readme.gentoo-r1.eclass uses fold, but fmt (both are in coreutils)
> would probably also be an option (and has a --uniform-spacing option
> ;)).

readme.gentoo.eclass originally had fmt, but then switched to fold
following discussion in bug 460050 [1].

Ulrich

[1] https://bugs.gentoo.org/460050

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

Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass

[Florian Schmaus]

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

On 19/06/2024 10.32, Ulrich Mueller wrote:
>>>>>> On Tue, 18 Jun 2024, Florian Schmaus wrote:
> 
>> Any preference regarding the auto-formatting tool? The
>> readme.gentoo-r1.eclass uses fold, but fmt (both are in coreutils)
>> would probably also be an option (and has a --uniform-spacing option
>> ;)).
> 
> readme.gentoo.eclass originally had fmt, but then switched to fold
> following discussion in bug 460050 [1].

'fold' it is then. See

https://gitlab.com/flow/flow-s-ebuilds/-/commit/a9215c903a5434a65df1f2cae8409750d94eec74

Note that thanks to greadme.eclass *not* using a variable like 
DOC_CONTENTS, we can avoid some shell trickery that 
readme.gentoo-r1.eclass has to do [1].

- Flow

1: 
https://github.com/gentoo/gentoo/blob/f10decf0ec3bdf75618124534057df0a14b8c61b/eclass/readme.gentoo-r1.eclass#L63-L68

[-- 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 v4] greadme.eclass: new eclass

[Ulrich Mueller]

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

>>>>> On Sun, 16 Jun 2024, Florian Schmaus wrote:

> First, the content of README.gento will only be shown on new

Typo.

> +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 greadme="${EROOT}${_GREADME_REL_PATH}"
> +
> +	if [[ ! -f ${greadme} ]]; then
> +		ewarn "Unable to show ${_GREADME_FILENAME}, file not installed (FEATURES=nodoc enabled?)."

I still think that the file's contents should be saved in a variable,
which would avoid the problem. (Presumably, pkg_preinst wouldn't be
necessary then either, because the variable is still available in the
postinst phase, whereas ${D} isn't.)

In any case, showing this warning every time might be annoying.
How about showing it only the first time, i.e. if REPLACING_VERSIONS
is empty?

> +		return
> +	fi
> +
> +	local line
> +	while read -r line; do elog "${line}"; done < "${greadme}"
> +	elog ""
> +	elog "(Note: Above message is only printed the first time package is"
> +	elog "installed or if the message changes on update. Please look at"
> +	elog "${EPREFIX}${_GREADME_REL_PATH} for future reference)"
> +}

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