Re: [gentoo-portage-dev] [PATCH] per package environment: generalize the mechanism to be profile specific

[Michał Górny]

Thanks for the patch. I have a few comments though.

Dnia 2014-09-16, o godz. 14:37:04
Bertrand Simonnet <bsimonnet@××××××.com> napisał(a):

> From 4bf1ee98bb97136e3f6f2182e205aed7ca597640 Mon Sep 17 00:00:00 2001
> From: Bertrand SIMONNET <bsimonnet@××××××××.org>
> Date: Fri, 12 Sep 2014 05:07:20 -0700
> Subject: [PATCH] Make env/ bash scripts profile specific
>
> This generalize the /etc/portage/env mechanism to be stackable with profiles.
> ebuild.sh will walk the profiles and sources scripts in env/ following the same
> matching rules as for /etc/portage/env.
>
> Change-Id: I16bcd75790213d2204f83d4aa6e8b910f8829b6e

  ^^^^^^^^^ What is this? I don't know this tag.

> ---
>  bin/ebuild.sh                                      | 86 ++++++++++++++--------
>  bin/save-ebuild-env.sh                             |  3 +-
>  .../package/ebuild/_config/LocationsManager.py     |  6 ++
>  .../package/ebuild/_config/special_env_vars.py     |  2 +-
>  pym/portage/package/ebuild/config.py               |  2 +
>  pym/portage/package/ebuild/doebuild.py             |  2 +
>  pym/portage/repository/config.py                   |  2 +-
>  7 files changed, 68 insertions(+), 35 deletions(-)
>
> diff --git a/bin/ebuild.sh b/bin/ebuild.sh
> index be044e0..26b9190 100755
> --- a/bin/ebuild.sh
> +++ b/bin/ebuild.sh
> @@ -67,11 +67,11 @@ unset BASH_ENV
>  # earlier portage versions do not detect a version change in this case
>  # (9999 to 9999) and therefore they try execute an incompatible version of
>  # ebuild.sh during the upgrade.
> -export PORTAGE_BZIP2_COMMAND=${PORTAGE_BZIP2_COMMAND:-bzip2}
> +export PORTAGE_BZIP2_COMMAND=${PORTAGE_BZIP2_COMMAND:-bzip2}

While whitespace fixes are generally a good idea, please don't mix them
with feature patches. It makes reviewing harder, for a start.

[...]

> @@ -369,45 +369,67 @@ __source_all_bashrcs() {
>  		save_IFS
>  		IFS=$'\n'
>  		local path_array=($PROFILE_PATHS)
> +		local profile_attributes=($PORTAGE_PROFILE_ATTRIBUTES)
> +		local profile_path
>  		restore_IFS
> -		for x in "${path_array[@]}" ; do
> -			[ -f "$x/profile.bashrc" ] && __qa_source "$x/profile.bashrc"
> +		for i in "${!path_array[@]}" ; do
> +			profile_path="${path_array[$i]}"
> +			[ -f "${profile_path}/profile.bashrc" ] && \
> +				__qa_source "${profile_path}/profile.bashrc"
> +			[[ "${profile_attributes[$i]}" == *profile-env* ]] && \

I think you should use 'has' helper function here. The pattern match
would also succeed with 'subprofile-envofoobar' and we don't want
that :).

> +				__source_env_files "${profile_path}/env"

Could you split the patch into smaller, logical changes? If that's too
much of a hassle, then nevermind but I would prefer to have it like:

1) refactor the code to use __source_env_files and so on without
having anything directly related to new features yet,

2) add support for querying profile attributes,

3) add new profile-env support.

This way, we could distinguish better between what you refactored
and what you added.

>  		done
>  	fi
>
> -	if [ -r "${PORTAGE_BASHRC}" ] ; then
> -		if [ "$PORTAGE_DEBUG" != "1" ] || [ "${-/x/}" != "$-" ]; then
> -			source "${PORTAGE_BASHRC}"
> -		else
> -			set -x
> -			source "${PORTAGE_BASHRC}"
> -			set +x
> -		fi
> -	fi
> +	# The user's bashrc is the ONLY non-portage bit of code
> +	# that can change shopts without a QA violation.
> +	__try_source "${PORTAGE_BASHRC}" "no_qa"
                                         ^^^^^^^

This looks like a very confusing usage for a function. It would be
really preferable to use syntax similar to standard utilities, e.g.:

  __try_source --no-qa "${PORTAGE_BASHRC}"

>  	if [[ $EBUILD_PHASE != depend ]] ; then
> -		# The user's bashrc is the ONLY non-portage bit of code that can
> -		# change shopts without a QA violation.
> -		for x in "${PM_EBUILD_HOOK_DIR}"/${CATEGORY}/{${PN},${PN}:${SLOT%/*},${P},${PF}}; do
> -			if [ -r "${x}" ]; then
> -				# If $- contains x, then tracing has already been enabled
> -				# elsewhere for some reason. We preserve it's state so as
> -				# not to interfere.
> -				if [ "$PORTAGE_DEBUG" != "1" ] || [ "${-/x/}" != "$-" ]; then
> -					source "${x}"
> -				else
> -					set -x
> -					source "${x}"
> -					set +x
> -				fi
> -			fi
> -		done
> +		__source_env_files "${PM_EBUILD_HOOK_DIR}" "no_qa"
>  	fi
>
>  	[ ! -z "${OCC}" ] && export CC="${OCC}"
>  	[ ! -z "${OCXX}" ] && export CXX="${OCXX}"
>  }
>
> +# @FUNCTION: __source_env_files

Please describe the parameters as well (@USAGE).

> +# @DESCRIPTION:
> +# Source the files relevant to the current package from the given path.
> +# If $2 is not 'no_qa', all the scripts are sourced with __qa_source.
> +__source_env_files() {
> +	for x in "${1}"/${CATEGORY}/{${PN},${PN}:${SLOT%/*},${P},${PF}}; do
> +		__try_source "${x}" "${2}"
> +	done
> +}

Now I understand what the patch does. I don't think we want to actually
expand use of this verbose-style env files. I'd rather go for
package.env only since it is more in line with other package.* files.

> +
> +# @FUNCTION: __try_source
> +# @DESCRIPTION:
> +# If the path given as argument exists, source the file while preserving
> +# $-.
> +# If $2 is not 'no_qa', the file is sourced with __qa_source.
> +__try_source() {
> +	if [[ -r "$1" ]]; then
> +		# If $- contains x, then tracing has already been enabled
> +		# elsewhere for some reason. We preserve it's state so as
> +		# not to interfere.
> +		if [[ "$PORTAGE_DEBUG" != "1" ]] || [[ "${-/x/}" != "$-" ]]; then
> +			if [[ "${2}" == "no_qa" ]]; then
> +				source "${x}"
> +			else
> +				__qa_source "${x}"
> +			fi
> +		else
> +			set -x
> +			if [[ "${2}" == "no_qa" ]]; then
> +				source "${x}"
> +			else
> +				__qa_source "${x}"
> +			fi
> +			set +x
> +		fi
> +	fi
> +}

I'm pretty sure you could get this smaller if you put the long
conditional in a variable and use it twice just to control 'set -x' /
'set +x'. In pseudocode:

  [[ ${debug_on} ]] && set -x
  if [[ ... ]]; then
    source
  else
    qa_source
  fi
  [[ ${debug_on} ]] && set +x

> diff --git a/pym/portage/repository/config.py b/pym/portage/repository/config.py
> index 5e0d055..ef8054e 100644
> --- a/pym/portage/repository/config.py
> +++ b/pym/portage/repository/config.py
> @@ -40,7 +40,7 @@ if sys.hexversion >= 0x3000000:
>  _invalid_path_char_re = re.compile(r'[^a-zA-Z0-9._\-+:/]')
>
>  _valid_profile_formats = frozenset(
> -	['pms', 'portage-1', 'portage-2'])
> +	['pms', 'portage-1', 'portage-2', 'profile-env'])

I'm not sure if dedicated profile format is the best way to go. If I
understand your patch correctly, this means that 'profile-env' has only
features of PMS profile but not of 'portage-*' formats. This could mean
that some people would have to choose between features of one or
the other. Since both are compatible, i suggest 'portage-3' instead.

-- 
Best regards,
Michał Górny