Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass
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 OpenPGP_0x8CAC2A9678548E35.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass
> 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 signature.asc Description: PGP signature
Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass
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 OpenPGP_0x8CAC2A9678548E35.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass
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 " ") ..." 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 > arthur...@gentoo.org > Gentoo Linux developer (Python, pkgcore stack, QA, Arch Teams, GURU) > -- ionen signature.asc Description: PGP signature
Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass
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 arthur...@gentoo.org Gentoo Linux developer (Python, pkgcore stack, QA, Arch Teams, GURU) OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass
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 OpenPGP_0x8CAC2A9678548E35.asc Description: OpenPGP public key OpenPGP_signature.asc Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass
> 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 signature.asc Description: PGP signature
Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass
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 ch
Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass
> 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)" > +} signature.asc Description: PGP signature
Re: [gentoo-dev] [PATCH v4] greadme.eclass: new eclass
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 > --- > 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 ..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 > +# @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_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) > +