>>>>> 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
signature.asc
Description: PGP signature
