Re: [gentoo-dev] [RFC] check-reqs.eclass.patch

[Michał Górny]

On Tue, 30 Aug 2011 09:11:40 +0200
Tomáš Chvátal <scarabeus@g.o> wrote:

> @@ -66,80 +52,234 @@
>
>  # @ECLASS-VARIABLE: CHECKREQS_MEMORY
>  # @DESCRIPTION:
> -# How much RAM is needed in MB?
> +# @DEAULT_UNSET
> +# How much RAM is needed?

Typo. Also, shouldn't defaults go before @DESCRIPTION: ?

>  # @ECLASS-VARIABLE:  CHECKREQS_DISK_BUILD
>  # @DESCRIPTION:
> -# How much diskspace is needed to build the package? In MB
> +# @DEAULT_UNSET
> +# How much diskspace is needed to build the package?

Ditto.

>  # @ECLASS-VARIABLE: CHECKREQS_DISK_USR
>  # @DESCRIPTION:
> -# How much space in /usr is needed to install the package? In MB
> +# @DEAULT_UNSET
> +# How much space in /usr is needed to install the package?

Ditto.

>  # @ECLASS-VARIABLE: CHECKREQS_DISK_VAR
>  # @DESCRIPTION:
> -# How much space is needed in /var? In MB
> +# @DEAULT_UNSET
> +# How much space is needed in /var?

Ditto.

> +CHECKREQS_EXPORTED_FUNCTIONS="pkg_setup"
> +case "${EAPI:-0}" in
> +	0|1|2|3) ;;
> +	4) CHECKREQS_EXPORTED_FUNCTIONS="${EXPORTED_FUNCTIONS}
> pkg_pretend" ;;

Not the same var.

> +	*) die "EAPI=${EAPI} is not supported" ;;
> +esac

CHECKREQS_EXPORTED_FUNCTIONS is not used anywhere.

>  # @FUNCTION: check_reqs
>  # @DESCRIPTION:
> -# Checks the requirements given in the specific variables. If not
> reached, -# either prints a warning or dies.
> +# Obsolete function executing all the checks and priting out results
>  check_reqs() {
> -	[[ -n "${1}" ]] && die "Usage: check_reqs"
> +	debug-print-function ${FUNCNAME} "$@"
> +
> +	echo
> +	ewarn "QA: Package calling old ${FUNCNAME} function."
> +	ewarn "QA: Please file a bug against the package."
> +	ewarn "QA: It should call check-reqs_pkg_pretend and
> check-reqs_pkg_setup"
> +	ewarn "QA: and possibly use EAPI=4 or later."
> +	echo
> +
> +	check-reqs_pkg_setup "$@"
> +}
>
> -	export CHECKREQS_NEED_SLEEP="" CHECKREQS_NEED_DIE=""
> -	if [[ "$CHECKREQS_ACTION" != "ignore" ]] ; then
> -		[[ -n "$CHECKREQS_MEMORY" ]] && check_build_memory
> -		[[ -n "$CHECKREQS_DISK_BUILD" ]] && check_build_disk
> \
> -			"${T}" "${CHECKREQS_DISK_BUILD}"
> -		[[ -n "$CHECKREQS_DISK_USR" ]] && check_build_disk \
> -			"${ROOT}/usr" "${CHECKREQS_DISK_USR}"
> -		[[ -n "$CHECKREQS_DISK_VAR" ]] && check_build_disk \
> -			"${ROOT}/var" "${CHECKREQS_DISK_VAR}"
> +# @FUNCTION: check-reqs_pkg_setup
> +# @DESCRIPTION:
> +# Exported function running the resources checks in pkg_setup phase.
> +# It should be run in both phases to ensure condition changes between
> +# pkg_pretend and pkg_setup won't affect the build.
> +check-reqs_pkg_setup() {
> +	debug-print-function ${FUNCNAME} "$@"
> +
> +	check-reqs_prepare
> +	check-reqs_run
> +	check-reqs_output
> +}
> +
> +# @FUNCTION: check-reqs_pkg_pretend
> +# @DESCRIPTION:
> +# Exported function running the resources checks in pkg_pretend
> phase. +check-reqs_pkg_pretend() {
> +	debug-print-function ${FUNCNAME} "$@"
> +
> +	check-reqs_pkg_setup "$@"
> +}
> +
> +# @FUNCTION: check-reqs_prepare
> +# @DESCRIPTION:
> +# Internal function that checks the variables that should be defined.
> +check-reqs_prepare() {
> +	debug-print-function ${FUNCNAME} "$@"
> +
> +	if [[ -z ${CHECKREQS_MEMORY} &&
> +			-z ${CHECKREQS_DISK_BUILD} &&
> +			-z ${CHECKREQS_DISK_USR} &&
> +			-z ${CHECKREQS_DISK_VAR} ]]; then
> +		eerror "Set some check-reqs eclass variables if you
> want to use it."
> +		eerror "If you are user and see this message fill a
> bug against the package."
> +		die "${FUNCNAME}: check-reqs eclass called but not
> actualy used!" fi
> +}
>
> -	if [[ -n "${CHECKREQS_NEED_SLEEP}" ]] ; then
> -		echo
> -		ewarn "Bad things may happen! You may abort the
> build by pressing ctrl+c in"
> -		ewarn "the next 15 seconds."
> -		ewarn " "
> -		einfo "To make this kind of warning a fatal error,
> add a line to /etc/make.conf"
> -		einfo "setting CHECKREQS_ACTION=\"error\". To skip
> build requirements checking,"
> -		einfo "set CHECKREQS_ACTION=\"ignore\"."
> -		epause 15
> +# @FUNCTION: check-reqs_run
> +# @DESCRIPTION:
> +# Internal function that runs the check based on variable settings.
> +check-reqs_run() {
> +	debug-print-function ${FUNCNAME} "$@"
> +
> +	# some people are *censored*
> +	unset CHECKREQS_FAILED
> +
> +	[[ -n ${CHECKREQS_MEMORY} ]] && \
> +		check-reqs_memory \
> +			${CHECKREQS_MEMORY}
> +
> +	[[ -n ${CHECKREQS_DISK_BUILD} ]] && \
> +		check-reqs_disk \
> +			"${T}" \
> +			"${CHECKREQS_DISK_BUILD}"

Why not WORKDIR?

> +
> +	[[ -n ${CHECKREQS_DISK_USR} ]] && \
> +		check-reqs_disk \
> +			"${EROOT}/usr" \
> +			"${CHECKREQS_DISK_USR}"
> +
> +	[[ -n ${CHECKREQS_DISK_VAR} ]] && \
> +		check-reqs_disk \
> +			"${EROOT}/var" \
> +			"${CHECKREQS_DISK_VAR}"
> +}

Also, it may be a good idea to add some kind of generic var for this.
Like:

CHECKREQS_DISK_INSTALLED=( /usr 1234M /var 2345M )

> +# @FUNCTION: check-reqs_get_mesg

Typo.

> +# @DESCRIPTION:
> +# Internal function that returns number in megabites.
> +# Converts from 1G=1024 or 1T=1048576
> +check-reqs_get_megs() {
> +	debug-print-function ${FUNCNAME} "$@"
> +
> +	[[ -z ${1} ]] && die "Usage: ${FUNCNAME} [size]"
> +
> +	local unit=${1//[[:digit:]]/}
> +	local size=${1//[[:alpha:]]/}

Seems hacky and poor. What if one uses '1G234'? :P

> +
> +	if [[ ${unit//[[:alpha:]]/} != "" ]]; then
> +		die "${FUNCNAME}: Failed to determine units, garbage
> in variables"

Would that catch anything beside symbols? I guess it would catch '.' as
well.

> +	else
> +		# temporary workaround for empty units
> +		unit="M"
> +	fi
> +
> +	case ${unit} in
> +		[gG]) echo $((1024 * ${size})) ;;
> +		[mM]) echo ${size} ;;
> +		[tT]) echo $((1024 * 1024 * ${size})) ;;
> +		*)
> +			die "${FUNCNAME}: Unknown unit size: ${unit}"

size unit.

> +		;;
> +	esac
> +}
> +
> +# @FUNCTION: check-reqs_get_numbers

Why the plural?

> +# @DESCRIPTION:
> +# Internal function that returns number without the unit.
> +# Converts from 1G=1 or 150T=150.
> +check-reqs_get_numbers() {
> +	debug-print-function ${FUNCNAME} "$@"
> +
> +	[[ -z ${1} ]] && die "Usage: ${FUNCNAME} [size]"
> +
> +	local size=${1//[[:alpha:]]/}
> +
> +	# warn about un-united variables
> +	if [[ ${size} == ${1//[[:alpha:]]/} ]]; then
> +		ewarn "QA: Package does not specify unit for the
> size check"
> +		ewarn "QA: Assuming Megabytes."
> +		ewarn "QA: File bug against the package. It should
> specify it." fi
>
> -	if [[ -n "${CHECKREQS_NEED_DIE}" ]] ; then
> -		eerror "Bailing out as specified by CHECKREQS_ACTION"
> -		die "Build requirements not met"
> +	if [[ ${size//[[:digit:]]/} == "" ]]; then
> +		echo ${size}
> +	else
> +		die "${FUNCNAME}: Failed to determine size, garbage
> in variables." fi
>  }

And at this point, I can't stand it anymore. Man, you've transformed
a bad eclass into almost-python.eclass. You should not post diffs when
they don't help with anything; and you should not rewrite eclasses like
that.

Start from scratch, write check-reqs-r1, make it SIMPLE.

-- 
Best regards,
Michał Górny