| Summary: | einstalldocs does not install default documentation files with valueless DOCS | ||
|---|---|---|---|
| Product: | Portage Development | Reporter: | Ulrich Müller <ulm> |
| Component: | Core - Ebuild Support | Assignee: | Portage team <dev-portage> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | Keywords: | InVCS |
| Priority: | Normal | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Bug Depends on: | |||
| Bug Blocks: | 711148 | ||
| Attachments: | Minimum ebuild to reproduce the problem | ||
|
Description
Ulrich Müller
2020-02-18 17:21:39 UTC
'local variable' creates variable without setting its value.
Algorithm in PMS is imprecise about what it means in sentences "if the DOCS variable is a non-empty array", "if the DOCS variable is a non-empty scalar".
If you want distinction, when requiring BASH >=4.2, -v operator can be used:
From BASH's NEWS file:
"""
-------------------------------------------------------------------------------
This is a terse description of the new features added to bash-4.2 since
the release of bash-4.1. As always, the manual page (doc/bash.1) is
the place to look for complete descriptions.
1. New Features in Bash
...
f. test/[/[[ have a new -v variable unary operator, which returns success if
`variable' has been set.
"""
$ (f() { local VAR1 VAR2= ; local -a VAR3 VAR4=() ; local -A VAR5 VAR6=() ; declare -p VAR{1,2,3,4,5,6} ; for var in VAR{1,2,3,4,5,6} ; do [[ -v ${var} ]] ; echo $? ; done ; } ; f)
declare -- VAR1
declare -- VAR2=""
declare -a VAR3
declare -a VAR4=()
declare -A VAR5
declare -A VAR6=()
1
0
1
1
1
1
$
(In reply to Arfrever Frehtes Taifersar Arahesis from comment #1) > If you want distinction, when requiring BASH >=4.2, -v operator can be used: > [...] > $ (f() { local VAR1 VAR2= ; local -a VAR3 VAR4=() ; local -A VAR5 VAR6=() ; declare -p VAR{1,2,3,4,5,6} ; for var in VAR{1,2,3,4,5,6} ; do [[ -v ${var} ]] ; echo $? ; done ; } ; f) > declare -- VAR1 > declare -- VAR2="" > declare -a VAR3 > declare -a VAR4=() > declare -A VAR5 > declare -A VAR6=() > 1 > 0 > 1 > 1 > 1 > 1 > $ IMHO that's not what we want. VAR4 and VAR6 both have a value (namely, the empty array), so we want the test to succeed. Also, output of test -v differs between bash versions, I get "1 0 0 0 0 0" with bash-4.2_p53, but "1 0 1 1 1 1" with bash-5.0_p16 (even if I set BASH_COMPAT="4.2"). How about the following instead, for the first condition in einstalldocs? [[ $(declare -p DOCS 2>/dev/null) == *=* ]] In Portage some months ago this optimization (subshell avoidance) occurred: https://gitweb.gentoo.org/proj/portage.git/commit/?id=5473a2695aa3fb3a7cdee889fe8c861fcb274277 @a gives results which you would probably expect: $ (f() { local VAR1 VAR2= ; local -a VAR3 VAR4=() ; local -A VAR5 VAR6=() ; for var in VAR{1,2,3,4,5,6} ; do echo "${!var@a}" ; done ; } ; f) a A $ Using both -v and @a allows to distinguish variables without values and variables with values: $ (f() { local VAR1 VAR2= ; local -a VAR3 VAR4=() ; local -A VAR5 VAR6=() ; declare -p VAR{1,2,3,4,5,6} ; for var in VAR{1,2,3,4,5,6} ; do [[ -v ${var} || ${!var@a} =~ [aA] ]] ; echo $? ; done ; } ; f) declare -- VAR1 declare -- VAR2="" declare -a VAR3 declare -a VAR4=() declare -A VAR5 declare -A VAR6=() 1 0 1 0 1 0 $ Previously I have not noticed word "non-empty" in these sentences in PMS. But it seems to be mistake in PMS. Do you really expect that explicit DOCS=() or DOCS="" will be ignored, and README* ChangeLog AUTHORS NEWS TODO CHANGES THANKS BUGS FAQ CREDITS CHANGELOG will be still installed? There are many ebuilds which set explicit DOCS=() or DOCS="". (In reply to Arfrever Frehtes Taifersar Arahesis from comment #3) > Previously I have not noticed word "non-empty" in these sentences in PMS. > But it seems to be mistake in PMS. > Do you really expect that explicit DOCS=() or DOCS="" will be ignored, and > README* ChangeLog AUTHORS NEWS TODO CHANGES THANKS BUGS FAQ CREDITS > CHANGELOG will be still installed? This problem is absent in description of src_install() for EAPI={4,5}: https://projects.gentoo.org/pms/7/pms.html#x1-940009.1.9 Only einstalldocs() (for EAPI={6,7}) is affected. (In reply to Arfrever Frehtes Taifersar Arahesis from comment #3) > In Portage some months ago this optimization (subshell avoidance) occurred: > https://gitweb.gentoo.org/proj/portage.git/commit/ > ?id=5473a2695aa3fb3a7cdee889fe8c861fcb274277 > > @a gives results which you would probably expect: > > $ (f() { local VAR1 VAR2= ; local -a VAR3 VAR4=() ; local -A VAR5 VAR6=() ; > for var in VAR{1,2,3,4,5,6} ; do echo "${!var@a}" ; done ; } ; f) > > > > a > > A > $ Looks like @a behaviour differs between bash versions, too: $ declare -a foo; echo "${foo@a}" # bash-4.4_p23-r1 $ $ declare -a foo; echo "${foo@a}" # bash-5.0_p16 a $ Also, @a doesn't exist in bash-4.2. > Using both -v and @a allows to distinguish variables without values and > variables with values: > > $ (f() { local VAR1 VAR2= ; local -a VAR3 VAR4=() ; local -A VAR5 VAR6=() ; > declare -p VAR{1,2,3,4,5,6} ; for var in VAR{1,2,3,4,5,6} ; do [[ -v ${var} > || ${!var@a} =~ [aA] ]] ; echo $? ; done ; } ; f) > declare -- VAR1 > declare -- VAR2="" > declare -a VAR3 > declare -a VAR4=() > declare -A VAR5 > declare -A VAR6=() > 1 > 0 > 1 > 0 > 1 > 0 > $ That's easier achieved (and available in all bash versions) by matching the = sign in declare -p output, as I had suggested at the end of comment #2: [[ $(declare -p DOCS 2>/dev/null) == *=* ]] > Previously I have not noticed word "non-empty" in these sentences in PMS. > But it seems to be mistake in PMS. A "non-empty array" is an array with at least one element, and a "non-empty scalar" is a scalar variable with length >= 1. > Do you really expect that explicit DOCS=() or DOCS="" will be ignored, and > README* ChangeLog AUTHORS NEWS TODO CHANGES THANKS BUGS FAQ CREDITS > CHANGELOG will be still installed? Explicit DOCS=() or DOCS="" means the variable is set, but empty. So neither of the conditions in algorithm 12.4 will be true, i.e., nothing will be installed. > There are many ebuilds which set explicit DOCS=() or DOCS="". (In reply to Ulrich Müller from comment #5) > Looks like @a behaviour differs between bash versions, too: > > $ declare -a foo; echo "${foo@a}" # bash-4.4_p23-r1 > > $ > > $ declare -a foo; echo "${foo@a}" # bash-5.0_p16 > a > $ In fact, this makes me wonder if the "optimization" introduced in https://gitweb.gentoo.org/proj/portage.git/commit/?id=5473a2695aa3fb3a7cdee889fe8c861fcb274277 causes any breakage with one of these bash versions. Regarding https://gitweb.gentoo.org/proj/portage.git/commit/?id=5473a2695aa3fb3a7cdee889fe8c861fcb274277: +if [[ BASH_VERSINFO -gt 4 || (BASH_VERSINFO -eq 4 && BASH_VERSINFO[1] -ge 4) ]] ; then + ___is_indexed_array_var() { + [[ ${!1@a} == *a* ]] + } +else + ___is_indexed_array_var() { + [[ $(declare -p "$1" 2>/dev/null) == 'declare -a'* ]] + } +fi BASH >=4.4 version of ___is_indexed_array_var() does not work in the same way as BASH <4.4 version of this function due to bug in BASH: https://lists.gnu.org/archive/html/bug-bash/2020-02/msg00050.html https://lists.gnu.org/archive/html/bug-bash/2020-02/msg00054.html https://lists.gnu.org/archive/html/bug-bash/2020-02/msg00055.html ${!1@a} is expanded into empty string if array has no value, while 'declare -p' still shows 'declare -a ...'. In order to keep the same behavior of versions of this function, workaround in affected versions of BASH would be: ___is_indexed_array_var() { eval "[[ \${$1@a} == *a* ]]" } (In reply to Ulrich Müller from comment #5) > Looks like @a behaviour differs between bash versions, too: > > $ declare -a foo; echo "${foo@a}" # bash-4.4_p23-r1 > > $ > > $ declare -a foo; echo "${foo@a}" # bash-5.0_p16 > a > $ Due to this bug in BASH 4.4, the condition should be probably changed: -if [[ BASH_VERSINFO -gt 4 || (BASH_VERSINFO -eq 4 && BASH_VERSINFO[1] -ge 4) ]] ; then +if [[ BASH_VERSINFO -ge 5 ]] ; then (And some comment should be added.) I think that most ebuild writers would expect that all of the following commands would disable installation of documentation files: local DOCS local -a DOCS local DOCS= local -a DOCS=() But it seems that you would prefer that first two commands be ignored... I suggest that handling of DOCS in einstalldocs be the same as in src_install() in EAPI={4,5}.
The latter treats 'local DOCS' as disabling installation of documentation files from default list (regardless of "DOCS is ${DOCS-unset}${DOCS+set}" being expanded into "DOCS is unset").
(In reply to Arfrever Frehtes Taifersar Arahesis from comment #10) > I think that most ebuild writers would expect that all of the following > commands would disable installation of documentation files: > > local DOCS > local -a DOCS > local DOCS= > local -a DOCS=() > > But it seems that you would prefer that first two commands be ignored... What about this: local DOCS=( foo bar ) einstalldocs unset DOCS einstalldocs I believe that it is a valid use case, and that most ebuild writers would expect the second einstalldocs to install the default set of files. Besides, the spec says "if the DOCS variable is unset" which we cannot ignore. The bug has been closed via the following commit(s): https://gitweb.gentoo.org/proj/portage.git/commit/?id=42fe7eadad8c80836ace0de527e988383f63b7da commit 42fe7eadad8c80836ace0de527e988383f63b7da Author: Ulrich Müller <ulm@gentoo.org> AuthorDate: 2020-02-20 13:04:34 +0000 Commit: Ulrich Müller <ulm@gentoo.org> CommitDate: 2020-02-20 13:27:33 +0000 einstalldocs: Fix test for DOCS being unset. The current test does not exactly test for unset DOCS, because it also evaluates as true if the variable has attributes. Such attributes can be defined even for an unset variable. Therefore test the output of declare -p for presence of an = sign instead, which indicates that a value has been assigned to the variable (bug 710076 comment #2). PMS reference: Algorithm 12.4, line 7: https://projects.gentoo.org/pms/7/pms.html#x1-135011r183 See also bash upstream discussion: https://lists.gnu.org/archive/html/bug-bash/2020-02/msg00045.html Closes: https://bugs.gentoo.org/710076 Signed-off-by: Ulrich Müller <ulm@gentoo.org> bin/phase-helpers.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=196e51a0010cf17a3733fe6bc2516cf9e01a4a8a commit 196e51a0010cf17a3733fe6bc2516cf9e01a4a8a Author: Zac Medico <zmedico@gentoo.org> AuthorDate: 2020-03-01 06:51:49 +0000 Commit: Zac Medico <zmedico@gentoo.org> CommitDate: 2020-03-01 06:58:10 +0000 sys-apps/portage: Bump to version 2.3.90 #601252 DISTDIR NFS root_squash support #709746 new PORTAGE_LOG_FILTER_FILE variable specifies a command that filters build log output to a log file #710076 einstalldocs: Fix test for DOCS being unset Bug: https://bugs.gentoo.org/711148 Bug: https://bugs.gentoo.org/601252 Bug: https://bugs.gentoo.org/709746 Bug: https://bugs.gentoo.org/710076 Package-Manager: Portage-2.3.90, Repoman-2.3.20 Signed-off-by: Zac Medico <zmedico@gentoo.org> sys-apps/portage/Manifest | 1 + sys-apps/portage/portage-2.3.90.ebuild | 271 +++++++++++++++++++++++++++++++++ 2 files changed, 272 insertions(+) |