[gentoo-dev] [PATCH] autotools.eclass: eclassdoc, cosmetic changes, drop old EAPIs

[gentoo-dev] [PATCH] autotools.eclass: eclassdoc, cosmetic changes, drop old EAPIs

[Sam James]

(Relatively) significant changes:
* inherit eutils for < EAPI 7 for eqawarn
* mark WANT_AUTO*, AUTOTOOLS_AUTO_DEPEND as @PRE_INHERIT
* convert phase test to EBUILD_PHASE_FUNC
* drop support for < EAPI 5

[Needed for the EBUILD_PHASE_FUNC change.
< EAPI 5 is no longer in ::gentoo (since December).]

eclassdoc fixes:
* explicitly blank and mark variables as @DEFAULT_UNSET
* add @DESCRIPTION for _at_uses_pkg
* mark AT_M4DIR as @DEFAULT_UNSET
* document AUTOTOOLS_DEPEND

Cosmetic changes:
* minor cosmetic changes to various elogs
* fix whitespace/phrasing in comment
* convert ewarn to eqawarn
* consistent 'case' style
* consistent variable references
* consistent references to bugs in comments
* consistent use of ${ECLASS}, not "autotools.eclass"
* use same ${WANT_AUTOCONF} comparison test throughout

Signed-off-by: Sam James <sam@gentoo.org>
---
 eclass/autotools.eclass | 101 ++++++++++++++++++++++++----------------
 1 file changed, 61 insertions(+), 40 deletions(-)

diff --git a/eclass/autotools.eclass b/eclass/autotools.eclass
index 4ae360aa24d..1c687d7cb5e 100644
--- a/eclass/autotools.eclass
+++ b/eclass/autotools.eclass
@@ -19,31 +19,38 @@ if [[ ${__AUTOTOOLS_AUTO_DEPEND+set} == "set" ]] ; then
 	# eclass at that point, but that adds overhead, and it's trivial
 	# to re-order inherit in eclasses/ebuilds instead.  #409611
 	if [[ ${__AUTOTOOLS_AUTO_DEPEND} != ${AUTOTOOLS_AUTO_DEPEND} ]] ; then
-		die "AUTOTOOLS_AUTO_DEPEND changed value between inherits; please inherit autotools.eclass first! ${__AUTOTOOLS_AUTO_DEPEND} -> ${AUTOTOOLS_AUTO_DEPEND}"
+		die "AUTOTOOLS_AUTO_DEPEND changed value between inherits; please inherit ${ECLASS} first! ${__AUTOTOOLS_AUTO_DEPEND} -> ${AUTOTOOLS_AUTO_DEPEND}"
 	fi
 fi
 
-if [[ -z ${_AUTOTOOLS_ECLASS} ]]; then
+if [[ -z ${_AUTOTOOLS_ECLASS} ]] ; then
 _AUTOTOOLS_ECLASS=1
 
 case ${EAPI:-0} in
-	0|1|2|3|4|5|6|7) ;;
+	5|6)
+		# Needed for eqawarn
+		inherit eutils
+		;;
+	7) ;;
 	*) die "${ECLASS}: EAPI ${EAPI} not supported" ;;
 esac
 
 inherit libtool
 
 # @ECLASS-VARIABLE: WANT_AUTOCONF
+# @PRE_INHERIT
 # @DESCRIPTION:
 # The major version of autoconf your package needs
 : ${WANT_AUTOCONF:=latest}
 
 # @ECLASS-VARIABLE: WANT_AUTOMAKE
+# @PRE_INHERIT
 # @DESCRIPTION:
 # The major version of automake your package needs
 : ${WANT_AUTOMAKE:=latest}
 
 # @ECLASS-VARIABLE: WANT_LIBTOOL
+# @PRE_INHERIT
 # @DESCRIPTION:
 # Do you want libtool?  Valid values here are "latest" and "none".
 : ${WANT_LIBTOOL:=latest}
@@ -61,9 +68,9 @@ inherit libtool
 # version.
 # If a newer slot is stable on any arch, and is NOT reflected in this list,
 # then circular dependencies may arise during emerge @system bootstraps.
-# 
-# See bug 312315 and 465732 for further information and context.
-# 
+#
+# See bug #312315 and bug #465732 for further information and context.
+#
 # Do NOT change this variable in your ebuilds!
 # If you want to force a newer minor version, you can specify the correct
 # WANT value by using a colon:  <PV>:<WANT_AUTOMAKE>
@@ -71,11 +78,11 @@ _LATEST_AUTOMAKE=( 1.16.2-r1:1.16 )
 
 _automake_atom="sys-devel/automake"
 _autoconf_atom="sys-devel/autoconf"
-if [[ -n ${WANT_AUTOMAKE} ]]; then
+if [[ -n ${WANT_AUTOMAKE} ]] ; then
 	case ${WANT_AUTOMAKE} in
 		# Even if the package doesn't use automake, we still need to depend
 		# on it because we run aclocal to process m4 macros.  This matches
-		# the autoreconf tool, so this requirement is correct.  #401605
+		# the autoreconf tool, so this requirement is correct, bug #401605.
 		none) ;;
 		latest)
 			# Use SLOT deps if we can.  For EAPI=0, we get pretty close.
@@ -111,12 +118,18 @@ if [[ -n ${WANT_LIBTOOL} ]] ; then
 	export WANT_LIBTOOL
 fi
 
+# @ECLASS-VARIABLE: AUTOTOOLS_DEPEND
+# @INTERNAL
+# @DESCRIPTION:
+# Contains the combination of requested automake/autoconf/libtool
+# versions in *DEPEND format.
 AUTOTOOLS_DEPEND="${_automake_atom}
 	${_autoconf_atom}
 	${_libtool_atom}"
 RDEPEND=""
 
 # @ECLASS-VARIABLE: AUTOTOOLS_AUTO_DEPEND
+# @PRE_INHERIT
 # @DESCRIPTION:
 # Set to 'no' to disable automatically adding to DEPEND.  This lets
 # ebuilds form conditional depends by using ${AUTOTOOLS_DEPEND} in
@@ -137,12 +150,14 @@ unset _automake_atom _autoconf_atom
 # @DESCRIPTION:
 # Additional options to pass to automake during
 # eautoreconf call.
+: ${AM_OPTS:=}
 
 # @ECLASS-VARIABLE: AT_NOEAUTOHEADER
 # @DEFAULT_UNSET
 # @DESCRIPTION:
 # Don't run eautoheader command if set to 'yes'; only used to work around
 # packages that don't want their headers being modified.
+: ${AT_NOEAUTOHEADER:=}
 
 # @ECLASS-VARIABLE: AT_NOEAUTOMAKE
 # @DEFAULT_UNSET
@@ -150,6 +165,7 @@ unset _automake_atom _autoconf_atom
 # Don't run eautomake command if set to 'yes'; only used to workaround
 # broken packages.  Generally you should, instead, fix the package to
 # not call AM_INIT_AUTOMAKE if it doesn't actually use automake.
+: ${AT_NOEAUTOMAKE:=}
 
 # @ECLASS-VARIABLE: AT_NOELIBTOOLIZE
 # @DEFAULT_UNSET
@@ -157,13 +173,16 @@ unset _automake_atom _autoconf_atom
 # Don't run elibtoolize command if set to 'yes',
 # useful when elibtoolize needs to be ran with
 # particular options
+: ${AT_NOELIBTOOLIZE:=}
 
 # @ECLASS-VARIABLE: AT_M4DIR
+# @DEFAULT_UNSET
 # @DESCRIPTION:
 # Additional director(y|ies) aclocal should search
 : ${AT_M4DIR:=}
 
 # @ECLASS-VARIABLE: AT_SYS_M4DIR
+# @DEFAULT_UNSET
 # @INTERNAL
 # @DESCRIPTION:
 # For system integrators, a list of additional aclocal search paths.
@@ -182,13 +201,13 @@ unset _automake_atom _autoconf_atom
 eautoreconf() {
 	local x g
 
-	# Subdirs often share a common build dir #529404.  If so, we can't safely
+	# Subdirs often share a common build dir, bug #529404.  If so, we can't safely
 	# run in parallel because many tools clobber the content in there.  Libtool
 	# and automake both `rm && cp` while aclocal reads the output.  We might be
 	# able to handle this if we split the steps and grab locks on the dirs the
 	# tools actually write to.  Then we'd run all the common tools that use
 	# those inputs.  Doing this in bash does not scale easily.
-	# If we do re-enable parallel support, make sure #426512 is handled.
+	# If we do re-enable parallel support, make sure bug #426512 is handled.
 	if [[ -z ${AT_NO_RECURSIVE} ]] ; then
 		# Take care of subdirs
 		for x in $(autotools_check_macro_val AC_CONFIG_SUBDIRS) ; do
@@ -239,7 +258,7 @@ eautoreconf() {
 	done
 	${rerun_aclocal} && eaclocal
 
-	if [[ ${WANT_AUTOCONF} = 2.1 ]] ; then
+	if [[ ${WANT_AUTOCONF} == "2.1" ]] ; then
 		eautoconf
 	else
 		eautoconf --force
@@ -259,6 +278,7 @@ eautoreconf() {
 # @FUNCTION: _at_uses_pkg
 # @USAGE: <macros>
 # @INTERNAL
+# @DESCRIPTION:
 # See if the specified macros are enabled.
 _at_uses_pkg() {
 	if [[ -n $(autotools_check_macro "$@") ]] ; then
@@ -294,8 +314,8 @@ eaclocal_amflags() {
 		[[ -e ${amflags_file} ]] || continue
 		# setup the env in case the pkg does something crazy
 		# in their ACLOCAL_AMFLAGS.  like run a shell script
-		# which turns around and runs autotools. #365401
-		# or split across multiple lines. #383525
+		# which turns around and runs autotools (bug #365401)
+		# or split across multiple lines (bug #383525)
 		autotools_env_setup
 		aclocal_opts=$(sed -n \
 			"/^ACLOCAL_AMFLAGS[[:space:]]*=/{ \
@@ -317,7 +337,7 @@ eaclocal_amflags() {
 # specified parametes. The name of the tool run is the same of the function
 # without e prefix.
 # They also force installing the support files for safety.
-# Respects AT_M4DIR for additional directories to search for macro's.
+# Respects AT_M4DIR for additional directories to search for macros.
 eaclocal() {
 	[[ ! -f aclocal.m4 || -n $(grep -e 'generated.*by aclocal' aclocal.m4) ]] && \
 		autotools_run_tool --at-m4flags aclocal "$@" $(eaclocal_amflags)
@@ -331,7 +351,7 @@ eaclocal() {
 _elibtoolize() {
 	local LIBTOOLIZE=${LIBTOOLIZE:-$(type -P glibtoolize > /dev/null && echo glibtoolize || echo libtoolize)}
 
-	if [[ $1 == "--auto-ltdl" ]] ; then
+	if [[ ${1} == "--auto-ltdl" ]] ; then
 		shift
 		_at_uses_libltdl && set -- "$@" --ltdl
 	fi
@@ -359,6 +379,7 @@ eautoconf() {
 		echo
 		die "No configure.{ac,in} present!"
 	fi
+
 	if [[ ${WANT_AUTOCONF} != "2.1" && -e configure.in ]] ; then
 		eqawarn "This package has a configure.in file which has long been deprecated.  Please"
 		eqawarn "update it to use configure.ac instead as newer versions of autotools will die"
@@ -366,7 +387,7 @@ eautoconf() {
 	fi
 
 	# Install config.guess and config.sub which are required by many macros
-	# in Autoconf >=2.70.
+	# in autoconf >=2.70.
 	local _gnuconfig
 	case ${EAPI:-0} in
 		0|1|2|3|4|5|6)
@@ -403,7 +424,7 @@ eautomake() {
 	if [[ -z ${makefile_name} ]] ; then
 		_at_uses_automake || return 0
 
-	elif [[ -z ${FROM_EAUTORECONF} && -f ${makefile_name%.am}.in ]]; then
+	elif [[ -z ${FROM_EAUTORECONF} && -f ${makefile_name%.am}.in ]] ; then
 		local used_automake
 		local installed_automake
 
@@ -411,7 +432,7 @@ eautomake() {
 		used_automake=$(head -n 1 < ${makefile_name%.am}.in | \
 			sed -e 's:.*by automake \(.*\) from .*:\1:')
 
-		if [[ ${installed_automake} != ${used_automake} ]]; then
+		if [[ ${installed_automake} != ${used_automake} ]] ; then
 			ewarn "Automake used for the package (${used_automake}) differs from" \
 				"the installed version (${installed_automake})."
 			ewarn "Forcing a full rebuild of the autotools to workaround."
@@ -424,10 +445,10 @@ eautomake() {
 		|| extra_opts+=( --foreign )
 
 	# Older versions of automake do not support --force-missing.  But we want
-	# to use this whenever possible to update random bundled files #133489.
+	# to use this whenever possible to update random bundled files, bug #133489.
 	case $(_automake_version) in
-	1.4|1.4[.-]*) ;;
-	*) extra_opts+=( --force-missing ) ;;
+		1.4|1.4[.-]*) ;;
+		*) extra_opts+=( --force-missing ) ;;
 	esac
 
 	autotools_run_tool automake --add-missing --copy "${extra_opts[@]}" "$@"
@@ -475,7 +496,7 @@ config_rpath_update() {
 autotools_env_setup() {
 	# We do the "latest" → version switch here because it solves
 	# possible order problems, see bug #270010 as an example.
-	if [[ ${WANT_AUTOMAKE} == "latest" ]]; then
+	if [[ ${WANT_AUTOMAKE} == "latest" ]] ; then
 		local pv
 		for pv in ${_LATEST_AUTOMAKE[@]/#*:} ; do
 			# Break on first hit to respect _LATEST_AUTOMAKE order.
@@ -505,30 +526,30 @@ autotools_env_setup() {
 autotools_run_tool() {
 	# Process our own internal flags first
 	local autofail=true m4flags=false missing_ok=false return_output=false
-	while [[ -n $1 ]] ; do
-		case $1 in
-		--at-no-fail) autofail=false;;
-		--at-m4flags) m4flags=true;;
-		--at-missing) missing_ok=true;;
-		--at-output)  return_output=true;;
-		# whatever is left goes to the actual tool
-		*) break;;
+	while [[ -n ${1} ]] ; do
+		case ${1} in
+			--at-no-fail) autofail=false ;;
+			--at-m4flags) m4flags=true ;;
+			--at-missing) missing_ok=true ;;
+			--at-output)  return_output=true ;;
+			# whatever is left goes to the actual tool
+			*) break ;;
 		esac
 		shift
 	done
 
-	if [[ ${EBUILD_PHASE} != "unpack" && ${EBUILD_PHASE} != "prepare" ]]; then
-		ewarn "QA Warning: running $1 in ${EBUILD_PHASE} phase"
+	if [[ ${EBUILD_PHASE_FUNC} != "src_unpack" && ${EBUILD_PHASE_FUNC} != "src_prepare" ]] ; then
+		eqawarn "Running '${1}' in ${EBUILD_PHASE_FUNC} phase"
 	fi
 
 	if ${missing_ok} && ! type -P ${1} >/dev/null ; then
-		einfo "Skipping '$*' due $1 not installed"
+		einfo "Skipping '$*' because '${1}' not installed"
 		return 0
 	fi
 
 	autotools_env_setup
 
-	# Allow people to pass in full paths. #549268
+	# Allow people to pass in full paths, bug #549268
 	local STDERR_TARGET="${T}/${1##*/}.out"
 	# most of the time, there will only be one run, but if there are
 	# more, make sure we get unique log filenames
@@ -551,19 +572,19 @@ autotools_run_tool() {
 		return
 	fi
 
-	printf "***** $1 *****\n***** PWD: ${PWD}\n***** $*\n\n" > "${STDERR_TARGET}"
+	printf "***** ${1} *****\n***** PWD: ${PWD}\n***** $*\n\n" > "${STDERR_TARGET}"
 
-	ebegin "Running $@"
+	ebegin "Running '$@'"
 	"$@" >> "${STDERR_TARGET}" 2>&1
 	if ! eend $? && ${autofail} ; then
 		echo
-		eerror "Failed Running $1 !"
+		eerror "Failed running '${1}'!"
 		eerror
-		eerror "Include in your bugreport the contents of:"
+		eerror "Include in your bug report the contents of:"
 		eerror
 		eerror "  ${STDERR_TARGET}"
 		echo
-		die "Failed Running $1 !"
+		die "Failed running '${1}'!"
 	fi
 }
 
@@ -636,7 +657,7 @@ _autotools_m4dir_include() {
 			# We handle it below
 			-${flag}) ;;
 			*)
-				[[ ! -d ${x} ]] && ewarn "autotools.eclass: '${x}' does not exist"
+				[[ ! -d ${x} ]] && ewarn "${ECLASS}: '${x}' does not exist"
 				include_opts+=" -${flag} ${x}"
 				;;
 		esac
-- 
2.31.1

Re: [gentoo-dev] [PATCH] autotools.eclass: eclassdoc, cosmetic changes, drop old EAPIs

[Matt Turner]

Looks good to me.

Re: [gentoo-dev] [PATCH] autotools.eclass: eclassdoc, cosmetic changes, drop old EAPIs

[Ulrich Mueller]

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

>>>>> On Sat, 27 Mar 2021, Sam James wrote:
 
> -if [[ -z ${_AUTOTOOLS_ECLASS} ]]; then
> +if [[ -z ${_AUTOTOOLS_ECLASS} ]] ; then

This just adds unnecessary noise to the git history. We don't have any
policy on whitespace before punctuation marks, but the examples in the
Bash manual don't have whitespace before semicolons. (Several more of
these changes in the reset of the commit.)

> -	# Subdirs often share a common build dir #529404.  If so, we can't safely
> +	# Subdirs often share a common build dir, bug #529404.  If so, we can't safely

Long line.
 
> -	if [[ ${EBUILD_PHASE} != "unpack" && ${EBUILD_PHASE} != "prepare" ]]; then
> -		ewarn "QA Warning: running $1 in ${EBUILD_PHASE} phase"
> +	if [[ ${EBUILD_PHASE_FUNC} != "src_unpack" && ${EBUILD_PHASE_FUNC} != "src_prepare" ]] ; then
> +		eqawarn "Running '${1}' in ${EBUILD_PHASE_FUNC} phase"

What is wrong with checking EBUILD_PHASE?

Ulrich

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

Re: [gentoo-dev] [PATCH] autotools.eclass: eclassdoc, cosmetic changes, drop old EAPIs

[David Seifert]

On Sun, 2021-03-28 at 10:13 +0200, Ulrich Mueller wrote:
> > > > > > On Sat, 27 Mar 2021, Sam James wrote:
>  
> > -if [[ -z ${_AUTOTOOLS_ECLASS} ]]; then
> > +if [[ -z ${_AUTOTOOLS_ECLASS} ]] ; then
> 
> This just adds unnecessary noise to the git history. We don't have any
> policy on whitespace before punctuation marks, but the examples in the
> Bash manual don't have whitespace before semicolons. (Several more of
> these changes in the reset of the commit.)

This is just bringing it in line with the rest of the eclass. You know,
consistency.

Re: [gentoo-dev] [PATCH] autotools.eclass: eclassdoc, cosmetic changes, drop old EAPIs

[Ulrich Mueller]

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

>>>>> On Sun, 28 Mar 2021, David Seifert wrote:

> This is just bringing it in line with the rest of the eclass. You know,
> consistency.

If that's the goal then the patch should update all occurences, though.
Especially those where usage is inconsistent within the same line.

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

Re: [gentoo-dev] [PATCH] autotools.eclass: eclassdoc, cosmetic changes, drop old EAPIs

[David Seifert]

On Sat, 2021-03-27 at 22:21 +0000, Sam James wrote:
> (Relatively) significant changes:
> * inherit eutils for < EAPI 7 for eqawarn
> * mark WANT_AUTO*, AUTOTOOLS_AUTO_DEPEND as @PRE_INHERIT
> * convert phase test to EBUILD_PHASE_FUNC
> * drop support for < EAPI 5

LGTM

Re: [gentoo-dev] [PATCH] autotools.eclass: eclassdoc, cosmetic changes, drop old EAPIs

[Michał Górny]

Hi,

Thank you for doing this.  LGTM modulo a few nits below.

On Sat, 2021-03-27 at 22:21 +0000, Sam James wrote:
> -	if [[ ${EBUILD_PHASE} != "unpack" && ${EBUILD_PHASE} != "prepare" ]]; then
> -		ewarn "QA Warning: running $1 in ${EBUILD_PHASE} phase"
> +	if [[ ${EBUILD_PHASE_FUNC} != "src_unpack" && ${EBUILD_PHASE_FUNC} != "src_prepare" ]] ; then

Maybe use 'has' here?

> +		eqawarn "Running '${1}' in ${EBUILD_PHASE_FUNC} phase"
>  	fi
>  
> 
> 
> 
> 
> 
> 
> 
>  	if ${missing_ok} && ! type -P ${1} >/dev/null ; then
> -		einfo "Skipping '$*' due $1 not installed"
> +		einfo "Skipping '$*' because '${1}' not installed"

...is not installed?  ;-)


-- 
Best regards,
Michał Górny

Re: [gentoo-dev] [PATCH] autotools.eclass: eclassdoc, cosmetic changes, drop old EAPIs

[Sam James]

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


> On 27 Mar 2021, at 22:21, Sam James <sam@gentoo.org> wrote:
> 

Given how uncontroversial most of it is, the positive reviews, and the fact that I pushed a load
of other eclassdoc changes (for other eclasses) but CI is still down, it makes
sense to throw in as much as we can to not waste the big cache regeneration that’s going to occur
anyway.

I’ve therefore pushed the uninteresting parts of this (all cosmetic for eclassdocs or whitespace,
nothing functional involving EAPIs or anything like that).

I’ll send out a v2 either later today or tomorrow with the parts I’ve cherry-picked removed.

v2 will look roughly like:
> (Relatively) significant changes:
> * inherit eutils for < EAPI 7 for eqawarn
> * drop support for < EAPI 5
> 
> [Needed for the EBUILD_PHASE_FUNC change.
> < EAPI 5 is no longer in ::gentoo (since December).]
> 
> eclassdoc fixes:
> * explicitly blank and mark variables as @DEFAULT_UNSET
> * add @DESCRIPTION for _at_uses_pkg
> * document AUTOTOOLS_DEPEND
> 
> Cosmetic changes:
> * convert ewarn to eqawarn
> * consistent 'case' style
> * consistent variable references
> * consistent use of ${ECLASS}, not "autotools.eclass"
> * use same ${WANT_AUTOCONF} comparison test throughout

If we’re sticking with the dropping of EAPIs earlier than 5, I think there’s some more cruft I can clear out too.

So, as you can see, not much interesting actually got cherry-picked. For transparency - although
you could read git yourself, I cherry-picked the following:

$ git shortlog 3f1cd0c161eeec3a5b0173da31f8949fa5eb38b5..HEAD
Sam James (6):
      autotools.eclass: mark AT_M4DIR as @DEFAULT_UNSET
      autotools.eclass: mark WANT_AUTO*, AUTOTOOLS_AUTO_DEPEND as @PRE_INHERIT
      autotools.eclass: mark AT_SYS_M4DIR variable as @DEFAULT_UNSET
      autotools.eclass: consistent references to bugs in comments
      autotools.eclass: fix whitespace/phrasing in comment
      autotools.eclass: minor cosmetic changes to various elogs


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 618 bytes --]

Re: [gentoo-dev] [PATCH] autotools.eclass: eclassdoc, cosmetic changes, drop old EAPIs

[Sam James]



> On 28 Mar 2021, at 13:46, Ulrich Mueller <ulm@gentoo.org> wrote:
> 
> 
>>>>>>> On Sun, 28 Mar 2021, David Seifert wrote:
>> This is just bringing it in line with the rest of the eclass. You know,
>> consistency.
> 
> If that's the goal then the patch should update all occurences, though.

Well, that was definitely the aim. To avoid any doubt, I don’t feel particularly passionate about which style we use so it was not the main point of this patch. Just copying existing style for consistency.

> Especially those where usage is inconsistent within the same line.

I’m mobile right now but maybe you could show me what you’re referring to? Thank you!

We have review for a reason - to improve - but being vague doesn’t help me do that ;)