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

> 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


> more readable when compared to


I think the variable would be renamed in that case, i.e.

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


> 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

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


Attachment: signature.asc
Description: PGP signature

Reply via email to