From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by finch.gentoo.org (Postfix) with ESMTPS id B2BDB15817D for ; Thu, 13 Jun 2024 10:42:27 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 3F60C2BC01E; Thu, 13 Jun 2024 10:42:23 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [IPv6:2001:470:ea4a:1:5054:ff:fec7:86e4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id E9BC7E2B04 for ; Thu, 13 Jun 2024 10:42:22 +0000 (UTC) From: Ulrich Mueller To: Florian Schmaus Cc: gentoo-dev@lists.gentoo.org Subject: Re: [gentoo-dev] [PATCH v3 1/1] greadme.eclass: new eclass In-Reply-To: <2962cab0-a95d-48c0-b325-653364540326@gentoo.org> (Florian Schmaus's message of "Thu, 13 Jun 2024 12:18:51 +0200") References: <20240613083928.165509-1-flow@gentoo.org> <20240613083928.165509-2-flow@gentoo.org> <2962cab0-a95d-48c0-b325-653364540326@gentoo.org> Date: Thu, 13 Jun 2024 12:42:12 +0200 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-dev@lists.gentoo.org Reply-to: gentoo-dev@lists.gentoo.org X-Auto-Response-Suppress: DR, RN, NRN, OOF, AutoReply MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" X-Archives-Salt: a490982f-af09-4e8d-8952-8856352cc648 X-Archives-Hash: 9088d80e140e0a3cf7303670ad4d37c7 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable >>>>> On Thu, 13 Jun 2024, Florian Schmaus wrote: >>> +_GREADME_DOC_DIR=3D"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. >>=20 >>> +_GREADME_REL_PATH=3D"${_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-er= oot-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=3D"/usr/share/doc/${PF}" > instead of > _GREADME_DOC_DIR=3D"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=3D"forced" > ;; > no) > _GREADME_SHOW=3D"" > ;; > *) > 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_RE= L_PATH}" >> It is not guaranteed that the file exists in ${EROOT} at this point. >> See FEATURES=3D"nodoc" in make.conf(5). > Fair point. Fixed. Well, it still won't display anything with FEATURES=3Dnodoc. 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/ef307612a676c7810e823ff6= e38dac41bebd9dde). Ulrich --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQFDBAEBCAAtFiEEtDnZ1O9xIP68rzDbUYgzUIhBXi4FAmZqzQQPHHVsbUBnZW50 b28ub3JnAAoJEFGIM1CIQV4uhZ8IAJ4somw4Trn27wu1fMplu6z/OT8/yR619Q4p 3i/qMOGKY1tDSuIDQUPnx/xsL7H5QBlBQaduq5QG+bkIl1lbKeynmJ0vEdj4xqLG KFFuj0u2bXRYu7qcc/yTYhXmZAy8+R0MIiEVvhCT9JI8X50W5JS1CxFwPGkhta5H VWasunCSEWNFVvyqGm5yhlqrD4js6BcQf7d6Y/cxhaiBtGSnifu+fivtCg8f/SLT f1e/g/OuoJD8gPcbLmK9bwmSZNVa8NVNvWqlS2qrk8n6vmbYyFq0YC+p7dxZ64JG 210Ngu9aFoUxR82slwQmnAlckWCaw9otDSP5IITu/e/PB8EEcOM= =d/O/ -----END PGP SIGNATURE----- --=-=-=--