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

[Ulrich Mueller <ulm@gentoo.org>]

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