[gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Mike Frysinger]

To try and provide better stability across bash versions,
set the language compat level based on the current EAPI.
---
 bin/eapi.sh   |  8 ++++++++
 bin/ebuild.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/bin/eapi.sh b/bin/eapi.sh
index 528e6f2..b236344 100644
--- a/bin/eapi.sh
+++ b/bin/eapi.sh
@@ -191,3 +191,11 @@ ___eapi_enables_failglob_in_global_scope() {
 ___eapi_enables_globstar() {
 	[[ ${1-${EAPI-0}} =~ ^(4-python|5-progress)$ ]]
 }
+
+__eapi_bash_3_2() {
+	[[ ${1-${EAPI-0}} =~ ^(0|1|2|3|4|4-python|4-slot-abi|5|5-hdepend|5-progress)$ ]]
+}
+
+__eapi_bash_4_2() {
+	[[ ${1-${EAPI-0}} =~ ^(6)$ ]]
+}
diff --git a/bin/ebuild.sh b/bin/ebuild.sh
index 75a9d24..2d09fb8 100755
--- a/bin/ebuild.sh
+++ b/bin/ebuild.sh
@@ -6,8 +6,47 @@
 # Make sure it's before everything so we don't mess aliases that follow.
 unalias -a
 
+# Make sure this isn't exported to scripts we execute.
+unset BASH_COMPAT
+
 source "${PORTAGE_BIN_PATH}/isolated-functions.sh" || exit 1
 
+# Set up the bash version compatibility level.
+__check_bash_version() {
+	# Figure out which min version of bash we require.
+	local maj min
+	if __eapi_bash_3_2 ; then
+		maj=3 min=2
+	elif __eapi_bash_4_2 ; then
+		maj=4 min=2
+	else
+		return
+	fi
+
+	# Make sure the active bash is sane.
+	if [[ ${BASH_VERSINFO[0]} -lt ${maj} ]] ||
+	   [[ ${BASH_VERSINFO[0]} -eq ${maj} && ${BASH_VERSINFO[1]} -lt ${min} ]] ; then
+		die ">=bash-${maj}.${min} is required"
+	fi
+
+	# Set the compat level in case things change with newer ones.  We must not
+	# export this into the env otherwise we might break  other shell scripts we
+	# execute (e.g. ones in /usr/bin).
+	BASH_COMPAT="${maj}.${min}"
+
+	# The variable above is new to bash-4.3.  For older versions, we have to use
+	# a compat knob.  Further, the compat knob only exists with older versions
+	# (e.g. bash-4.3 has compat42 but not compat43).  This means we only need to
+	# turn the knob with older EAPIs, and only when running newer bash versions:
+	# there is no bash-3.3 (it went 3.2 to 4.0), and when requiring bash-4.2, the
+	# var works with bash-4.3+, and you don't need to set compat to 4.2 when you
+	# are already running 4.2.
+	if __eapi_bash_3_2 && [[ ${BASH_VERSINFO[0]} -gt 3 ]] ; then
+		shopt -s compat32
+	fi
+}
+__check_bash_version
+
 if [[ $EBUILD_PHASE != depend ]] ; then
 	source "${PORTAGE_BIN_PATH}/phase-functions.sh" || die
 	source "${PORTAGE_BIN_PATH}/save-ebuild-env.sh" || die
-- 
2.6.2

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

>>>>> On Tue, 10 Nov 2015, Mike Frysinger wrote:

> +	# Set the compat level in case things change with newer ones.  We must not
> +	# export this into the env otherwise we might break  other shell scripts we
> +	# execute (e.g. ones in /usr/bin).
> +	BASH_COMPAT="${maj}.${min}"
> +
> +	# The variable above is new to bash-4.3.  For older versions, we have to use
> +	# a compat knob.  Further, the compat knob only exists with older versions
> +	# (e.g. bash-4.3 has compat42 but not compat43).  This means we only need to
> +	# turn the knob with older EAPIs, and only when running newer bash versions:
> +	# there is no bash-3.3 (it went 3.2 to 4.0), and when requiring bash-4.2, the
> +	# var works with bash-4.3+, and you don't need to set compat to 4.2 when you
> +	# are already running 4.2.
> +	if __eapi_bash_3_2 && [[ ${BASH_VERSINFO[0]} -gt 3 ]] ; then
> +		shopt -s compat32
> +	fi

Wouldn't this profit from an additional test for <bash-4.3? If I
understood the upstream discussion correctly, they were thinking about
dropping the compat* options in some future version?

That is, the conditional should look like:

	if __eapi_bash_3_2 \
			&& [[ ${BASH_VERSINFO[0]} -eq 4 \
			&& ${BASH_VERSINFO[1]} -lt 3 ]] ; then
		shopt -s compat32
	fi

Ulrich

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 3269 bytes --]

On Tue, 10 Nov 2015 23:39:29 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

> To try and provide better stability across bash versions,
> set the language compat level based on the current EAPI.
> ---
>  bin/eapi.sh   |  8 ++++++++
>  bin/ebuild.sh | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/bin/eapi.sh b/bin/eapi.sh
> index 528e6f2..b236344 100644
> --- a/bin/eapi.sh
> +++ b/bin/eapi.sh
> @@ -191,3 +191,11 @@ ___eapi_enables_failglob_in_global_scope() {
>  ___eapi_enables_globstar() {
>  	[[ ${1-${EAPI-0}} =~ ^(4-python|5-progress)$ ]]
>  }
> +
> +__eapi_bash_3_2() {
> +	[[ ${1-${EAPI-0}} =~ ^(0|1|2|3|4|4-python|4-slot-abi|5|5-hdepend|5-progress)$ ]]
> +}
> +
> +__eapi_bash_4_2() {
> +	[[ ${1-${EAPI-0}} =~ ^(6)$ ]]
> +}
> diff --git a/bin/ebuild.sh b/bin/ebuild.sh
> index 75a9d24..2d09fb8 100755
> --- a/bin/ebuild.sh
> +++ b/bin/ebuild.sh
> @@ -6,8 +6,47 @@
>  # Make sure it's before everything so we don't mess aliases that follow.
>  unalias -a
>  
> +# Make sure this isn't exported to scripts we execute.
> +unset BASH_COMPAT
> +
>  source "${PORTAGE_BIN_PATH}/isolated-functions.sh" || exit 1
>  
> +# Set up the bash version compatibility level.
> +__check_bash_version() {
> +	# Figure out which min version of bash we require.
> +	local maj min
> +	if __eapi_bash_3_2 ; then
> +		maj=3 min=2
> +	elif __eapi_bash_4_2 ; then
> +		maj=4 min=2
> +	else
> +		return
> +	fi
> +
> +	# Make sure the active bash is sane.
> +	if [[ ${BASH_VERSINFO[0]} -lt ${maj} ]] ||
> +	   [[ ${BASH_VERSINFO[0]} -eq ${maj} && ${BASH_VERSINFO[1]} -lt ${min} ]] ; then
> +		die ">=bash-${maj}.${min} is required"
> +	fi
> +
> +	# Set the compat level in case things change with newer ones.  We must not
> +	# export this into the env otherwise we might break  other shell scripts we
> +	# execute (e.g. ones in /usr/bin).
> +	BASH_COMPAT="${maj}.${min}"
> +
> +	# The variable above is new to bash-4.3.  For older versions, we have to use
> +	# a compat knob.  Further, the compat knob only exists with older versions
> +	# (e.g. bash-4.3 has compat42 but not compat43).  This means we only need to
> +	# turn the knob with older EAPIs, and only when running newer bash versions:
> +	# there is no bash-3.3 (it went 3.2 to 4.0), and when requiring bash-4.2, the
> +	# var works with bash-4.3+, and you don't need to set compat to 4.2 when you
> +	# are already running 4.2.
> +	if __eapi_bash_3_2 && [[ ${BASH_VERSINFO[0]} -gt 3 ]] ; then
> +		shopt -s compat32
> +	fi
> +}
> +__check_bash_version
> +
>  if [[ $EBUILD_PHASE != depend ]] ; then
>  	source "${PORTAGE_BIN_PATH}/phase-functions.sh" || die
>  	source "${PORTAGE_BIN_PATH}/save-ebuild-env.sh" || die

I'm not convinced we ought to do this for EAPI < 6. It is a breaking
change after all, and as such changes the behavior of EAPI < 6 ebuilds.

There are some ebuilds/eclasses that have bash version checks,
and execute bash-4 code when bash-4 is available. As far as I
understand, this will effectively prohibit bash-4 code even though
BASH_VERSINFO will still indicate bash-4 is being used.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

>>>>> On Wed, 11 Nov 2015, Michał Górny wrote:

> I'm not convinced we ought to do this for EAPI < 6. It is a breaking
> change after all, and as such changes the behavior of EAPI < 6
> ebuilds.

Which according to PMS should assume bash version 3.2. Also, AFAICS
the only feature where the compat settings could have any impact is
this setting from compat42:

   If set, bash does not process the replacement string in the pattern
   substitution word expansion using quote removal.

All other compat changes affect either only POSIX mode, or interactive
mode, or string comparison (where the compat32 setting disables locale
specific behaviour and uses strcmp instead, so effectively the compat
setting should be saner).

> There are some ebuilds/eclasses that have bash version checks,
> and execute bash-4 code when bash-4 is available. As far as I
> understand, this will effectively prohibit bash-4 code even though
> BASH_VERSINFO will still indicate bash-4 is being used. 

The compat settings don't disable any new features. They only restore
previous behaviour where there was an incompatible change.

Ulrich

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

On 11 Nov 2015 07:33, Ulrich Mueller wrote:
> >>>>> On Tue, 10 Nov 2015, Mike Frysinger wrote:
> > +	# Set the compat level in case things change with newer ones.  We must not
> > +	# export this into the env otherwise we might break  other shell scripts we
> > +	# execute (e.g. ones in /usr/bin).
> > +	BASH_COMPAT="${maj}.${min}"
> > +
> > +	# The variable above is new to bash-4.3.  For older versions, we have to use
> > +	# a compat knob.  Further, the compat knob only exists with older versions
> > +	# (e.g. bash-4.3 has compat42 but not compat43).  This means we only need to
> > +	# turn the knob with older EAPIs, and only when running newer bash versions:
> > +	# there is no bash-3.3 (it went 3.2 to 4.0), and when requiring bash-4.2, the
> > +	# var works with bash-4.3+, and you don't need to set compat to 4.2 when you
> > +	# are already running 4.2.
> > +	if __eapi_bash_3_2 && [[ ${BASH_VERSINFO[0]} -gt 3 ]] ; then
> > +		shopt -s compat32
> > +	fi
> 
> Wouldn't this profit from an additional test for <bash-4.3? If I
> understood the upstream discussion correctly, they were thinking about
> dropping the compat* options in some future version?

my take away was that they weren't going to be adding new compat levels.
i don't think they were planning on dropping existing ones.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On 11 Nov 2015 10:32, Michał Górny wrote:
> I'm not convinced we ought to do this for EAPI < 6. It is a breaking
> change after all, and as such changes the behavior of EAPI < 6 ebuilds.

that is a false statement.  anything not working with bash-3.2 is already
broken according to the PMS.

> There are some ebuilds/eclasses that have bash version checks,
> and execute bash-4 code when bash-4 is available. As far as I
> understand, this will effectively prohibit bash-4 code even though
> BASH_VERSINFO will still indicate bash-4 is being used.

no, that's not what it does.  it changes behavior for code that itself
has changed behavior between versions.  it does not disable any newer
functionality.  it would have been nice to have a knob that would turn
off newer functionality as well, but upstream didn't seem keen on it.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Zac Medico]

On 11/10/2015 08:39 PM, Mike Frysinger wrote:
> To try and provide better stability across bash versions,
> set the language compat level based on the current EAPI.
> ---
>  bin/eapi.sh   |  8 ++++++++
>  bin/ebuild.sh | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/bin/eapi.sh b/bin/eapi.sh
> index 528e6f2..b236344 100644
> --- a/bin/eapi.sh
> +++ b/bin/eapi.sh
> @@ -191,3 +191,11 @@ ___eapi_enables_failglob_in_global_scope() {
>  ___eapi_enables_globstar() {
>  	[[ ${1-${EAPI-0}} =~ ^(4-python|5-progress)$ ]]
>  }
> +
> +__eapi_bash_3_2() {
> +	[[ ${1-${EAPI-0}} =~ ^(0|1|2|3|4|4-python|4-slot-abi|5|5-hdepend|5-progress)$ ]]
> +}
> +
> +__eapi_bash_4_2() {
> +	[[ ${1-${EAPI-0}} =~ ^(6)$ ]]
> +}
> diff --git a/bin/ebuild.sh b/bin/ebuild.sh
> index 75a9d24..2d09fb8 100755
> --- a/bin/ebuild.sh
> +++ b/bin/ebuild.sh
> @@ -6,8 +6,47 @@
>  # Make sure it's before everything so we don't mess aliases that follow.
>  unalias -a
>  
> +# Make sure this isn't exported to scripts we execute.
> +unset BASH_COMPAT
> +
>  source "${PORTAGE_BIN_PATH}/isolated-functions.sh" || exit 1
>  
> +# Set up the bash version compatibility level.
> +__check_bash_version() {

Please unset all new internal function inside bin/save-ebuild-env.sh.
Note that it already uses this line to unset functions beginning with
___eapi:

   unset -f $(compgen -A function ___eapi_)

However, your __eapi functions will not be matched because they only
begin with 2 underscores.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

On 11 Nov 2015 11:42, Zac Medico wrote:
> On 11/10/2015 08:39 PM, Mike Frysinger wrote:
> > +# Set up the bash version compatibility level.
> > +__check_bash_version() {
> 
> Please unset all new internal function inside bin/save-ebuild-env.sh.
> Note that it already uses this line to unset functions beginning with
> ___eapi:
> 
>    unset -f $(compgen -A function ___eapi_)

why don't we create a new namespace for portage funcs ?  something like __e* ?

> However, your __eapi functions will not be matched because they only
> begin with 2 underscores.

that wasn't intentional.  i'll change it to 3.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[gentoo-portage-dev] [PATCH v2] ebuild: set up bash compat levels

[Mike Frysinger]

To try and provide better stability across bash versions,
set the language compat level based on the current EAPI.
This does not ban newer features, it tells bash to use
the older bash behavior when the behavior changes across
versions.
---
 bin/eapi.sh            |  8 ++++++++
 bin/ebuild.sh          | 42 ++++++++++++++++++++++++++++++++++++++++++
 bin/save-ebuild-env.sh |  2 +-
 3 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/bin/eapi.sh b/bin/eapi.sh
index 528e6f2..cd3e1a4 100644
--- a/bin/eapi.sh
+++ b/bin/eapi.sh
@@ -191,3 +191,11 @@ ___eapi_enables_failglob_in_global_scope() {
 ___eapi_enables_globstar() {
 	[[ ${1-${EAPI-0}} =~ ^(4-python|5-progress)$ ]]
 }
+
+___eapi_bash_3_2() {
+	[[ ${1-${EAPI-0}} =~ ^(0|1|2|3|4|4-python|4-slot-abi|5|5-hdepend|5-progress)$ ]]
+}
+
+___eapi_bash_4_2() {
+	[[ ${1-${EAPI-0}} =~ ^(6)$ ]]
+}
diff --git a/bin/ebuild.sh b/bin/ebuild.sh
index 75a9d24..78a93f0 100755
--- a/bin/ebuild.sh
+++ b/bin/ebuild.sh
@@ -6,8 +6,50 @@
 # Make sure it's before everything so we don't mess aliases that follow.
 unalias -a
 
+# Make sure this isn't exported to scripts we execute.
+unset BASH_COMPAT
+
 source "${PORTAGE_BIN_PATH}/isolated-functions.sh" || exit 1
 
+# Set up the bash version compatibility level.  This does not disable
+# features when running with a newer version, but makes it so that when
+# bash changes behavior in an incompatible way, the older behavior is
+# used instead.
+__check_bash_version() {
+	# Figure out which min version of bash we require.
+	local maj min
+	if ___eapi_bash_3_2 ; then
+		maj=3 min=2
+	elif ___eapi_bash_4_2 ; then
+		maj=4 min=2
+	else
+		return
+	fi
+
+	# Make sure the active bash is sane.
+	if [[ ${BASH_VERSINFO[0]} -lt ${maj} ]] ||
+	   [[ ${BASH_VERSINFO[0]} -eq ${maj} && ${BASH_VERSINFO[1]} -lt ${min} ]] ; then
+		die ">=bash-${maj}.${min} is required"
+	fi
+
+	# Set the compat level in case things change with newer ones.  We must not
+	# export this into the env otherwise we might break  other shell scripts we
+	# execute (e.g. ones in /usr/bin).
+	BASH_COMPAT="${maj}.${min}"
+
+	# The variable above is new to bash-4.3.  For older versions, we have to use
+	# a compat knob.  Further, the compat knob only exists with older versions
+	# (e.g. bash-4.3 has compat42 but not compat43).  This means we only need to
+	# turn the knob with older EAPIs, and only when running newer bash versions:
+	# there is no bash-3.3 (it went 3.2 to 4.0), and when requiring bash-4.2, the
+	# var works with bash-4.3+, and you don't need to set compat to 4.2 when you
+	# are already running 4.2.
+	if ___eapi_bash_3_2 && [[ ${BASH_VERSINFO[0]} -gt 3 ]] ; then
+		shopt -s compat32
+	fi
+}
+__check_bash_version
+
 if [[ $EBUILD_PHASE != depend ]] ; then
 	source "${PORTAGE_BIN_PATH}/phase-functions.sh" || die
 	source "${PORTAGE_BIN_PATH}/save-ebuild-env.sh" || die
diff --git a/bin/save-ebuild-env.sh b/bin/save-ebuild-env.sh
index 477ed28..1120297 100644
--- a/bin/save-ebuild-env.sh
+++ b/bin/save-ebuild-env.sh
@@ -75,7 +75,7 @@ __save_ebuild_env() {
 		__ebuild_main __ebuild_phase __ebuild_phase_with_hooks \
 		__ebuild_arg_to_phase __ebuild_phase_funcs default \
 		__unpack_tar __unset_colors \
-		__source_env_files __try_source \
+		__source_env_files __try_source __check_bash_version \
 		__eqaquote __eqatag \
 		${QA_INTERCEPTORS}
 
-- 
2.6.2

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Zac Medico]

On 11/11/2015 12:55 PM, Mike Frysinger wrote:
> On 11 Nov 2015 11:42, Zac Medico wrote:
>> On 11/10/2015 08:39 PM, Mike Frysinger wrote:
>>> +# Set up the bash version compatibility level.
>>> +__check_bash_version() {
>>
>> Please unset all new internal function inside bin/save-ebuild-env.sh.
>> Note that it already uses this line to unset functions beginning with
>> ___eapi:
>>
>>    unset -f $(compgen -A function ___eapi_)
> 
> why don't we create a new namespace for portage funcs ?  something like __e* ?

That works for me. According to PMS, we're free to do anything we want
as long as it begins with at least 2 underscores.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On 11 Nov 2015 13:04, Zac Medico wrote:
> On 11/11/2015 12:55 PM, Mike Frysinger wrote:
> > On 11 Nov 2015 11:42, Zac Medico wrote:
> >> On 11/10/2015 08:39 PM, Mike Frysinger wrote:
> >>> +# Set up the bash version compatibility level.
> >>> +__check_bash_version() {
> >>
> >> Please unset all new internal function inside bin/save-ebuild-env.sh.
> >> Note that it already uses this line to unset functions beginning with
> >> ___eapi:
> >>
> >>    unset -f $(compgen -A function ___eapi_)
> > 
> > why don't we create a new namespace for portage funcs ?  something like __e* ?
> 
> That works for me. According to PMS, we're free to do anything we want
> as long as it begins with at least 2 underscores.

interesting.  why don't we just unmap all things that begin with 2 underscores
then ?  or maybe 3 ?
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Zac Medico]

On 11/11/2015 01:11 PM, Mike Frysinger wrote:
> On 11 Nov 2015 13:04, Zac Medico wrote:
>> On 11/11/2015 12:55 PM, Mike Frysinger wrote:
>>> On 11 Nov 2015 11:42, Zac Medico wrote:
>>>> On 11/10/2015 08:39 PM, Mike Frysinger wrote:
>>>>> +# Set up the bash version compatibility level.
>>>>> +__check_bash_version() {
>>>>
>>>> Please unset all new internal function inside bin/save-ebuild-env.sh.
>>>> Note that it already uses this line to unset functions beginning with
>>>> ___eapi:
>>>>
>>>>    unset -f $(compgen -A function ___eapi_)
>>>
>>> why don't we create a new namespace for portage funcs ?  something like __e* ?
>>
>> That works for me. According to PMS, we're free to do anything we want
>> as long as it begins with at least 2 underscores.
> 
> interesting.  why don't we just unmap all things that begin with 2 underscores
> then ?  or maybe 3 ?
> -mike
> 

Well, that's sort of a "greedy" approach when you consider that you
might wipe out a function defined in an eclass or ebuild. Check this to
see what might be filtered:

  bzgrep ^__ /var/db/pkg/*/*/environment.bz2

A nice compromise is to choose a namespace like __portage or something
like that.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Zac Medico]

On 11/11/2015 10:33 PM, Zac Medico wrote:
> On 11/11/2015 01:11 PM, Mike Frysinger wrote:
>> On 11 Nov 2015 13:04, Zac Medico wrote:
>>> On 11/11/2015 12:55 PM, Mike Frysinger wrote:
>>>> On 11 Nov 2015 11:42, Zac Medico wrote:
>>>>> On 11/10/2015 08:39 PM, Mike Frysinger wrote:
>>>>>> +# Set up the bash version compatibility level.
>>>>>> +__check_bash_version() {
>>>>>
>>>>> Please unset all new internal function inside bin/save-ebuild-env.sh.
>>>>> Note that it already uses this line to unset functions beginning with
>>>>> ___eapi:
>>>>>
>>>>>    unset -f $(compgen -A function ___eapi_)
>>>>
>>>> why don't we create a new namespace for portage funcs ?  something like __e* ?
>>>
>>> That works for me. According to PMS, we're free to do anything we want
>>> as long as it begins with at least 2 underscores.
>>
>> interesting.  why don't we just unmap all things that begin with 2 underscores
>> then ?  or maybe 3 ?
>> -mike
>>
> 
> Well, that's sort of a "greedy" approach when you consider that you
> might wipe out a function defined in an eclass or ebuild. Check this to
> see what might be filtered:
> 
>   bzgrep ^__ /var/db/pkg/*/*/environment.bz2
> 
> A nice compromise is to choose a namespace like __portage or something
> like that.
> 

Also note that some internals have been intentionally preserved in
environment.bz2. For example, __eapi6_src_install exposes the default
src_install implementation, which someone might examine for debugging
purposes.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH v2] ebuild: set up bash compat levels

[Zac Medico]

On 11/11/2015 01:00 PM, Mike Frysinger wrote:
> To try and provide better stability across bash versions,
> set the language compat level based on the current EAPI.
> This does not ban newer features, it tells bash to use
> the older bash behavior when the behavior changes across
> versions.
> ---
>  bin/eapi.sh            |  8 ++++++++
>  bin/ebuild.sh          | 42 ++++++++++++++++++++++++++++++++++++++++++
>  bin/save-ebuild-env.sh |  2 +-
>  3 files changed, 51 insertions(+), 1 deletion(-)

Looks good now.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 2397 bytes --]

On 11 Nov 2015 22:40, Zac Medico wrote:
> On 11/11/2015 10:33 PM, Zac Medico wrote:
> > On 11/11/2015 01:11 PM, Mike Frysinger wrote:
> >> On 11 Nov 2015 13:04, Zac Medico wrote:
> >>> On 11/11/2015 12:55 PM, Mike Frysinger wrote:
> >>>> On 11 Nov 2015 11:42, Zac Medico wrote:
> >>>>> On 11/10/2015 08:39 PM, Mike Frysinger wrote:
> >>>>>> +# Set up the bash version compatibility level.
> >>>>>> +__check_bash_version() {
> >>>>>
> >>>>> Please unset all new internal function inside bin/save-ebuild-env.sh.
> >>>>> Note that it already uses this line to unset functions beginning with
> >>>>> ___eapi:
> >>>>>
> >>>>>    unset -f $(compgen -A function ___eapi_)
> >>>>
> >>>> why don't we create a new namespace for portage funcs ?  something like __e* ?
> >>>
> >>> That works for me. According to PMS, we're free to do anything we want
> >>> as long as it begins with at least 2 underscores.
> >>
> >> interesting.  why don't we just unmap all things that begin with 2 underscores
> >> then ?  or maybe 3 ?
> > 
> > Well, that's sort of a "greedy" approach when you consider that you
> > might wipe out a function defined in an eclass or ebuild.

i'm fully aware we might clobber eclasses/ebuilds, but as you said, PMS already
says ebuilds shouldn't be using that space.

> > Check this to
> > see what might be filtered:
> > 
> >   bzgrep ^__ /var/db/pkg/*/*/environment.bz2

i find nothing of value in there.

from portage:
__bashpid ()
__eapi6_src_install ()
__eapi6_src_prepare ()
__start_distcc ()

from ebuilds/eclasses that have already stopped using __:
__do_sed_fix ()
___ECLASS_RECUR_MULTILIB=yes
___ECLASS_RECUR_TOOLCHAIN_FUNCS=yes
__versionator_shopt_toggle ()
__versionator__test_version_compare ()
__versionator__test_version_is_at_least ()

grepping the tree, i see like two packages and one eclass still using __.
both of which are trivial to convert.

> > A nice compromise is to choose a namespace like __portage or something
> > like that.

we should nuke ___* at least.  i don't see any installed package using that.

> Also note that some internals have been intentionally preserved in
> environment.bz2. For example, __eapi6_src_install exposes the default
> src_install implementation, which someone might examine for debugging
> purposes.

is that actually useful ?  i can't see how it would be.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Zac Medico]

On 11/12/2015 04:06 PM, Mike Frysinger wrote:
> On 11 Nov 2015 22:40, Zac Medico wrote:
>> On 11/11/2015 10:33 PM, Zac Medico wrote:
>>> On 11/11/2015 01:11 PM, Mike Frysinger wrote:
>>>> On 11 Nov 2015 13:04, Zac Medico wrote:
>>>>> On 11/11/2015 12:55 PM, Mike Frysinger wrote:
>>>>>> On 11 Nov 2015 11:42, Zac Medico wrote:
>>>>>>> On 11/10/2015 08:39 PM, Mike Frysinger wrote:
>>>>>>>> +# Set up the bash version compatibility level.
>>>>>>>> +__check_bash_version() {
>>>>>>>
>>>>>>> Please unset all new internal function inside bin/save-ebuild-env.sh.
>>>>>>> Note that it already uses this line to unset functions beginning with
>>>>>>> ___eapi:
>>>>>>>
>>>>>>>    unset -f $(compgen -A function ___eapi_)
>>>>>>
>>>>>> why don't we create a new namespace for portage funcs ?  something like __e* ?
>>>>>
>>>>> That works for me. According to PMS, we're free to do anything we want
>>>>> as long as it begins with at least 2 underscores.
>>>>
>>>> interesting.  why don't we just unmap all things that begin with 2 underscores
>>>> then ?  or maybe 3 ?
>>>
>>> Well, that's sort of a "greedy" approach when you consider that you
>>> might wipe out a function defined in an eclass or ebuild.
> 
> i'm fully aware we might clobber eclasses/ebuilds, but as you said, PMS already
> says ebuilds shouldn't be using that space.
> 
>>> Check this to
>>> see what might be filtered:
>>>
>>>   bzgrep ^__ /var/db/pkg/*/*/environment.bz2
> 
> i find nothing of value in there.
> 
> from portage:
> __bashpid ()
> __eapi6_src_install ()
> __eapi6_src_prepare ()
> __start_distcc ()

It's a bug that __bashpid and __start_distcc are not filtered.
One of the uses of ___save_ebuild_env is to remove anything that might
conflict with portage internals (__preprocess_ebuild_env uses it this way).

> from ebuilds/eclasses that have already stopped using __:
> __do_sed_fix ()
> ___ECLASS_RECUR_MULTILIB=yes
> ___ECLASS_RECUR_TOOLCHAIN_FUNCS=yes
> __versionator_shopt_toggle ()
> __versionator__test_version_compare ()
> __versionator__test_version_is_at_least ()
> 
> grepping the tree, i see like two packages and one eclass still using __.
> both of which are trivial to convert.

Sure, but do we really want to confuse people who might be ignorant of
this rule? Having functions disappear from the environment without
warning is very likely to cause confusion...

>>> A nice compromise is to choose a namespace like __portage or something
>>> like that.
> 
> we should nuke ___* at least.  i don't see any installed package using that.

I guess that's safe enough to mostly avoid confusion. Also, there's the
element of backward-compatibility for any __* functions in
/var/db/pkg/*/*/environment.bz2 of users' installed systems that might
be needed during pkg_prerm and pkg_postrm.

>> Also note that some internals have been intentionally preserved in
>> environment.bz2. For example, __eapi6_src_install exposes the default
>> src_install implementation, which someone might examine for debugging
>> purposes.
> 
> is that actually useful ?  i can't see how it would be.
> -mike
> 

Shrug, probably not (unless there's a bug in a particular
implementation, and someone wants to go back and check which
implementation was used for a particular installed package).
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 2001 bytes --]

On 12 Nov 2015 16:58, Zac Medico wrote:
> On 11/12/2015 04:06 PM, Mike Frysinger wrote:
> > from ebuilds/eclasses that have already stopped using __:
> > __do_sed_fix ()
> > ___ECLASS_RECUR_MULTILIB=yes
> > ___ECLASS_RECUR_TOOLCHAIN_FUNCS=yes
> > __versionator_shopt_toggle ()
> > __versionator__test_version_compare ()
> > __versionator__test_version_is_at_least ()
> > 
> > grepping the tree, i see like two packages and one eclass still using __.
> > both of which are trivial to convert.
> 
> Sure, but do we really want to confuse people who might be ignorant of
> this rule? Having functions disappear from the environment without
> warning is very likely to cause confusion...

that already happens to a degree if you happen to use a name that portage
uses itself.  we can add a repoman check, and if we think it comes up enough,
have portage itself warn when it blows away things it didn't register.

> Also, there's the
> element of backward-compatibility for any __* functions in
> /var/db/pkg/*/*/environment.bz2 of users' installed systems that might
> be needed during pkg_prerm and pkg_postrm.

the ones i highlighted are not needed for those purposes.  you're right it
could be a problem, i think the likelihood is low considering how infrequently
these two are used, and how much we push people to use other phases instead.

> >> Also note that some internals have been intentionally preserved in
> >> environment.bz2. For example, __eapi6_src_install exposes the default
> >> src_install implementation, which someone might examine for debugging
> >> purposes.
> > 
> > is that actually useful ?  i can't see how it would be.
> 
> Shrug, probably not (unless there's a bug in a particular
> implementation, and someone wants to go back and check which
> implementation was used for a particular installed package).

if we want to tag that kind of metadata in a build, we should just explicitly
include something like PORTAGE_VERSION.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Tim Harder]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

On 2015-11-11 14:42, Zac Medico wrote:
> Please unset all new internal function inside bin/save-ebuild-env.sh.
> Note that it already uses this line to unset functions beginning with
> ___eapi:

>    unset -f $(compgen -A function ___eapi_)

> However, your __eapi functions will not be matched because they only
> begin with 2 underscores.

Just to note another approach, pkgcore generates global and per-eapi
function lists at install time (or uses the generation scripts when
running from a checkout) so manually tracking lists of functions isn't
required.

The main reason I set that up was because I sometimes forgot to add
functions to a similar list that was previously used. :)

I'm not overly familiar with portage's bash code so I'm not sure if
that's feasible with its current structure.

Tim

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Zac Medico]

On 11/12/2015 06:07 PM, Tim Harder wrote:
> On 2015-11-11 14:42, Zac Medico wrote:
>> Please unset all new internal function inside bin/save-ebuild-env.sh.
>> Note that it already uses this line to unset functions beginning with
>> ___eapi:
> 
>>    unset -f $(compgen -A function ___eapi_)
> 
>> However, your __eapi functions will not be matched because they only
>> begin with 2 underscores.
> 
> Just to note another approach, pkgcore generates global and per-eapi
> function lists at install time (or uses the generation scripts when
> running from a checkout) so manually tracking lists of functions isn't
> required.
> 
> The main reason I set that up was because I sometimes forgot to add
> functions to a similar list that was previously used. :)
> 
> I'm not overly familiar with portage's bash code so I'm not sure if
> that's feasible with its current structure.
> 
> Tim
> 

That seems like a pretty reasonable approach, at least up until PMS
reserved the __ prefix for package managers. I'm pretty sure that Mike
is intend on exploiting this reserved prefix now. :)
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Tim Harder]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On 2015-11-12 21:25, Zac Medico wrote:
> > Just to note another approach, pkgcore generates global and per-eapi
> > function lists at install time (or uses the generation scripts when
> > running from a checkout) so manually tracking lists of functions isn't
> > required.

> That seems like a pretty reasonable approach, at least up until PMS
> reserved the __ prefix for package managers. I'm pretty sure that Mike
> is intend on exploiting this reserved prefix now. :)

I don't think it's always possible to use a prefix for functions that
shouldn't be saved to the env that are internally implemented in the PM
but externally accessible in ebuilds, e.g. insinto, docinto, inherit,
etc.

Tim

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Zac Medico]

On 11/12/2015 06:33 PM, Tim Harder wrote:
> On 2015-11-12 21:25, Zac Medico wrote:
>>> Just to note another approach, pkgcore generates global and per-eapi
>>> function lists at install time (or uses the generation scripts when
>>> running from a checkout) so manually tracking lists of functions isn't
>>> required.
> 
>> That seems like a pretty reasonable approach, at least up until PMS
>> reserved the __ prefix for package managers. I'm pretty sure that Mike
>> is intend on exploiting this reserved prefix now. :)
> 
> I don't think it's always possible to use a prefix for functions that
> shouldn't be saved to the env that are internally implemented in the PM
> but externally accessible in ebuilds, e.g. insinto, docinto, inherit,
> etc.
> 
> Tim
> 

That's true, I guess we should look into generating the list automatically.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On 12 Nov 2015 21:07, Tim Harder wrote:
> On 2015-11-11 14:42, Zac Medico wrote:
> > Please unset all new internal function inside bin/save-ebuild-env.sh.
> > Note that it already uses this line to unset functions beginning with
> > ___eapi:
> >
> >    unset -f $(compgen -A function ___eapi_)
> >
> > However, your __eapi functions will not be matched because they only
> > begin with 2 underscores.
> 
> Just to note another approach, pkgcore generates global and per-eapi
> function lists at install time (or uses the generation scripts when
> running from a checkout) so manually tracking lists of functions isn't
> required.
> 
> The main reason I set that up was because I sometimes forgot to add
> functions to a similar list that was previously used. :)
> 
> I'm not overly familiar with portage's bash code so I'm not sure if
> that's feasible with its current structure.

i was thinking of building lists too, but i'm not entirely sure how to
tease that out of portage.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

>>>>> On Thu, 12 Nov 2015, Zac Medico wrote:

> On 11/12/2015 04:06 PM, Mike Frysinger wrote:
>> from ebuilds/eclasses that have already stopped using __:
>> __do_sed_fix ()
>> ___ECLASS_RECUR_MULTILIB=yes
>> ___ECLASS_RECUR_TOOLCHAIN_FUNCS=yes
>> __versionator_shopt_toggle ()
>> __versionator__test_version_compare ()
>> __versionator__test_version_is_at_least ()
>> 
>> grepping the tree, i see like two packages and one eclass still using __.
>> both of which are trivial to convert.

> Sure, but do we really want to confuse people who might be ignorant of
> this rule? Having functions disappear from the environment without
> warning is very likely to cause confusion...

Maybe repoman should make people aware of this rule then?

Ulrich

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Zac Medico]

On 11/13/2015 12:12 AM, Ulrich Mueller wrote:
>>>>>> On Thu, 12 Nov 2015, Zac Medico wrote:
> 
>> On 11/12/2015 04:06 PM, Mike Frysinger wrote:
>>> from ebuilds/eclasses that have already stopped using __:
>>> __do_sed_fix ()
>>> ___ECLASS_RECUR_MULTILIB=yes
>>> ___ECLASS_RECUR_TOOLCHAIN_FUNCS=yes
>>> __versionator_shopt_toggle ()
>>> __versionator__test_version_compare ()
>>> __versionator__test_version_is_at_least ()
>>>
>>> grepping the tree, i see like two packages and one eclass still using __.
>>> both of which are trivial to convert.
> 
>> Sure, but do we really want to confuse people who might be ignorant of
>> this rule? Having functions disappear from the environment without
>> warning is very likely to cause confusion...
> 
> Maybe repoman should make people aware of this rule then?
> 
> Ulrich
> 

Yeah, that would be great. I'm a little more concerned about non-gentoo
people that don't necessarily run repoman, though. At my workplace, we
don't use repoman because it has too many checks that we don't care
about. We really need to make it more configurable for third-party users.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Michał Górny]

[-- Attachment #1: Type: text/plain, Size: 1720 bytes --]

On Wed, 11 Nov 2015 12:52:05 +0100
Ulrich Mueller <ulm@gentoo.org> wrote:

> >>>>> On Wed, 11 Nov 2015, Michał Górny wrote:  
> 
> > I'm not convinced we ought to do this for EAPI < 6. It is a breaking
> > change after all, and as such changes the behavior of EAPI < 6
> > ebuilds.  
> 
> Which according to PMS should assume bash version 3.2. Also, AFAICS
> the only feature where the compat settings could have any impact is
> this setting from compat42:
> 
>    If set, bash does not process the replacement string in the pattern
>    substitution word expansion using quote removal.
> 
> All other compat changes affect either only POSIX mode, or interactive
> mode, or string comparison (where the compat32 setting disables locale
> specific behaviour and uses strcmp instead, so effectively the compat
> setting should be saner).

Ok, I recalled the issue I was particularly concerned about.

  PV="1_beta2"
  MY_PV="${PV/_/~}"

This triggers different behavior between bash<=4.2 and bash>=4.3, with
the latter requiring:

  PV="1_beta2"
  MY_PV="${PV/_/\~}"

(which in turns breaks older). Now, if someone put a conditional on
BASH_VERSINFO that worked around this discrepancy, introducing
BASH_COMPAT suddenly breaks it by forcing the old behavior when
BASH_VERSINFO indicates the new behavior is expected.

Not saying this is very likely to happen. However, we are clearly
violating the idea behind PMS by allowing it to happen retroactively.

However, enabling BASH_COMPAT in EAPI 6 is a good thing. For example,
because people in EAPI 6 no longer need to be concerned about this
discrepancy.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

Re: [gentoo-portage-dev] [PATCH] ebuild: set up bash compat levels

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

>>>>> On Fri, 13 Nov 2015, Michał Górny wrote:

> Ok, I recalled the issue I was particularly concerned about.

>   PV="1_beta2"
>   MY_PV="${PV/_/~}"

> This triggers different behavior between bash<=4.2 and bash>=4.3,
> with the latter requiring:

>   PV="1_beta2"
>   MY_PV="${PV/_/\~}"

> (which in turns breaks older). Now, if someone put a conditional on
> BASH_VERSINFO that worked around this discrepancy, introducing
> BASH_COMPAT suddenly breaks it by forcing the old behavior when
> BASH_VERSINFO indicates the new behavior is expected.

> Not saying this is very likely to happen.

Highly unlikely, IMHO. There are several workarounds that are simpler
than making the code conditional on the bash version. For example:

   tilde="~"; MY_PV="${PV/_/${tilde}}"

or:

   MY_PV="${PV/_/$'\x7e'}"

> However, we are clearly violating the idea behind PMS by allowing it
> to happen retroactively.

Not sure. PMS is (and always was) quite clear about which bash version
belongs to which EAPI, so ebuilds have no business of circumventing
this by doing their own version checks.

> However, enabling BASH_COMPAT in EAPI 6 is a good thing. For
> example, because people in EAPI 6 no longer need to be concerned
> about this discrepancy.

Enabling it for EAPI 6 only will lead to the paradoxical situation
that bash will be compatible to 4.2 in EAPI 6, as it should be.
Whereas earlier EAPIs (which should use 3.2 really) will get the
behaviour of bash 4.3 or later instead.

Ulrich

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]