[gentoo-dev] (resent) [PATCH] eclass: Support dev-util/samurai

[gentoo-dev] (resent) [PATCH] eclass: Support dev-util/samurai

[Lars Wendler]

From: orbea <orbea@riseup.net>

Signed-off-by: Lars Wendler <polynomial-c@gentoo.org>
---
 eclass/cmake.eclass       | 22 ++++++++--------------
 eclass/meson.eclass       |  4 ++--
 eclass/ninja-utils.eclass | 37 +++++++++++++++++++++++++++++++++++--
 3 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/eclass/cmake.eclass b/eclass/cmake.eclass
index 4bd09459ea6..935ee1d8c81 100644
--- a/eclass/cmake.eclass
+++ b/eclass/cmake.eclass
@@ -52,9 +52,9 @@ _CMAKE_ECLASS=1
 # @DEFAULT_UNSET
 # @DESCRIPTION:
 # Specify a makefile generator to be used by cmake.
-# At this point only "emake" and "ninja" are supported.
-# The default is set to "ninja".
-: ${CMAKE_MAKEFILE_GENERATOR:=ninja}
+# At this point only "emake", "eninja" and "ninja" are supported.
+# The default is set to "eninja".
+: ${CMAKE_MAKEFILE_GENERATOR:=eninja}
 
 # @ECLASS-VARIABLE: CMAKE_REMOVE_MODULES_LIST
 # @DESCRIPTION:
@@ -117,8 +117,8 @@ case ${CMAKE_MAKEFILE_GENERATOR} in
 	emake)
 		BDEPEND="sys-devel/make"
 		;;
-	ninja)
-		BDEPEND="dev-util/ninja"
+	eninja|ninja)
+		BDEPEND="|| ( dev-util/ninja dev-util/samurai )"
 		;;
 	*)
 		eerror "Unknown value for \${CMAKE_MAKEFILE_GENERATOR}"
@@ -335,13 +335,6 @@ cmake_src_prepare() {
 		die "FATAL: Unable to find CMakeLists.txt"
 	fi
 
-	# if ninja is enabled but not installed, the build could fail
-	# this could happen if ninja is manually enabled (eg. make.conf) but not installed
-	if [[ ${CMAKE_MAKEFILE_GENERATOR} == ninja ]] && ! has_version -b dev-util/ninja; then
-		eerror "CMAKE_MAKEFILE_GENERATOR is set to ninja, but ninja is not installed."
-		die "Please install dev-util/ninja or unset CMAKE_MAKEFILE_GENERATOR."
-	fi
-
 	local modules_list
 	if [[ $(declare -p CMAKE_REMOVE_MODULES_LIST) == "declare -a"* ]]; then
 		modules_list=( "${CMAKE_REMOVE_MODULES_LIST[@]}" )
@@ -534,7 +527,7 @@ cmake_src_configure() {
 
 	local generator_name
 	case ${CMAKE_MAKEFILE_GENERATOR} in
-		ninja) generator_name="Ninja" ;;
+		eninja|ninja) generator_name="Ninja" ;;
 		emake) generator_name="Unix Makefiles" ;;
 	esac
 
@@ -592,8 +585,9 @@ cmake_build() {
 				*) emake VERBOSE=1 "$@" ;;
 			esac
 			;;
-		ninja)
+		eninja|ninja)
 			[[ -e build.ninja ]] || die "build.ninja not found. Error during configure stage."
+			CMAKE_MAKEFILE_GENERATOR=eninja
 			eninja "$@"
 			;;
 	esac
diff --git a/eclass/meson.eclass b/eclass/meson.eclass
index d0ce5adb355..ea02402aa83 100644
--- a/eclass/meson.eclass
+++ b/eclass/meson.eclass
@@ -1,4 +1,4 @@
-# Copyright 2017-2020 Gentoo Authors
+# Copyright 2017-2021 Gentoo Authors
 # Distributed under the terms of the GNU General Public License v2
 
 # @ECLASS: meson.eclass
@@ -55,7 +55,7 @@ if [[ -z ${_MESON_ECLASS} ]]; then
 _MESON_ECLASS=1
 
 MESON_DEPEND=">=dev-util/meson-0.54.0
-	>=dev-util/ninja-1.8.2
+	|| ( >=dev-util/ninja-1.8.2 dev-util/samurai )
 	dev-util/meson-format-array
 "
 
diff --git a/eclass/ninja-utils.eclass b/eclass/ninja-utils.eclass
index ca8d67191dc..5008564dabf 100644
--- a/eclass/ninja-utils.eclass
+++ b/eclass/ninja-utils.eclass
@@ -1,4 +1,4 @@
-# Copyright 1999-2018 Gentoo Foundation
+# Copyright 1999-2021 Gentoo Authors
 # Distributed under the terms of the GNU General Public License v2
 
 # @ECLASS: ninja-utils.eclass
@@ -27,6 +27,15 @@ case ${EAPI:-0} in
 	*) die "EAPI=${EAPI} is not yet supported" ;;
 esac
 
+# @ECLASS-VARIABLE: NINJA
+# @PRE_INHERIT
+# @DEFAULT_UNSET
+# @DESCRIPTION:
+# Specify a compatible ninja implementation to be used by eninja.
+# At this point only "ninja" and "samu" are supported.
+# The default is set to "ninja".
+: ${NINJA:=ninja}
+
 # @ECLASS-VARIABLE: NINJAOPTS
 # @DEFAULT_UNSET
 # @DESCRIPTION:
@@ -36,6 +45,30 @@ esac
 
 inherit multiprocessing
 
+_ninja_to_use() {
+	case "${NINJA}" in
+		ninja)
+			local ninja=dev-util/${NINJA}
+		;;
+		samu)
+			local ninja=dev-util/samurai
+		;;
+		*)
+			eerror "Unknown value for \${NINJA}"
+			die "Value ${NINJA} is not supported"
+		;;
+	esac
+
+	# if ninja or samurai are enabled but not installed, the build could fail
+	# this could happen if they are manually enabled (eg. make.conf) but not installed
+	if ! has_version -b ${ninja}; then
+		eerror "Value ${NINJA} for \${NINJA} is not installed"
+		die "Please install ${ninja}"
+	fi
+
+	echo ${NINJA}
+}
+
 # @FUNCTION: eninja
 # @USAGE: [<args>...]
 # @DESCRIPTION:
@@ -49,7 +82,7 @@ eninja() {
 	if [[ -z ${NINJAOPTS+set} ]]; then
 		NINJAOPTS="-j$(makeopts_jobs) -l$(makeopts_loadavg "${MAKEOPTS}" 0)"
 	fi
-	set -- ninja -v ${NINJAOPTS} "$@"
+	set -- "$(_ninja_to_use)" -v ${NINJAOPTS} "$@"
 	echo "$@" >&2
 	"$@" || die "${nonfatal_args[@]}" "${*} failed"
 }
-- 
2.31.1

Re: [gentoo-dev] (resent) [PATCH] eclass: Support dev-util/samurai

[Michał Górny]

Hi,

First things first:

- there should be a detailed commit message explaning what samurai is,
  to what degree it is compatible with ninja and what's happening here

- this should be split to one patch per eclass.


On Fri, 2021-04-09 at 18:50 +0200, Lars Wendler wrote:
> From: orbea <orbea@riseup.net>
> 
> Signed-off-by: Lars Wendler <polynomial-c@gentoo.org>
> ---
>  eclass/cmake.eclass       | 22 ++++++++--------------
>  eclass/meson.eclass       |  4 ++--
>  eclass/ninja-utils.eclass | 37 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 45 insertions(+), 18 deletions(-)
> 
> diff --git a/eclass/cmake.eclass b/eclass/cmake.eclass
> index 4bd09459ea6..935ee1d8c81 100644
> --- a/eclass/cmake.eclass
> +++ b/eclass/cmake.eclass
> @@ -52,9 +52,9 @@ _CMAKE_ECLASS=1
>  # @DEFAULT_UNSET
>  # @DESCRIPTION:
>  # Specify a makefile generator to be used by cmake.
> -# At this point only "emake" and "ninja" are supported.
> -# The default is set to "ninja".
> -: ${CMAKE_MAKEFILE_GENERATOR:=ninja}
> +# At this point only "emake", "eninja" and "ninja" are supported.
> +# The default is set to "eninja".
> +: ${CMAKE_MAKEFILE_GENERATOR:=eninja}

At this point, this gets really confusing to end users (what is
the difference between 'ninja' and 'eninja')?  Also, adding a new value
and changing the default breaks ebuild that run code conditionally
on CMAKE_MAKEFILE_GENERATOR.

>  
> 
>  # @ECLASS-VARIABLE: CMAKE_REMOVE_MODULES_LIST
>  # @DESCRIPTION:
> @@ -117,8 +117,8 @@ case ${CMAKE_MAKEFILE_GENERATOR} in
>  	emake)
>  		BDEPEND="sys-devel/make"
>  		;;
> -	ninja)
> -		BDEPEND="dev-util/ninja"
> +	eninja|ninja)
> +		BDEPEND="|| ( dev-util/ninja dev-util/samurai )"

With hardcoded 'ninja' call, I doubt this is going to be true
for 'ninja' choice.

Also, this isn't going to work anyway since you don't check which one
is installed anywhere anyway.

>  		;;
>  	*)
>  		eerror "Unknown value for \${CMAKE_MAKEFILE_GENERATOR}"
> @@ -335,13 +335,6 @@ cmake_src_prepare() {
>  		die "FATAL: Unable to find CMakeLists.txt"
>  	fi
>  
> 
> -	# if ninja is enabled but not installed, the build could fail
> -	# this could happen if ninja is manually enabled (eg. make.conf) but not installed
> -	if [[ ${CMAKE_MAKEFILE_GENERATOR} == ninja ]] && ! has_version -b dev-util/ninja; then
> -		eerror "CMAKE_MAKEFILE_GENERATOR is set to ninja, but ninja is not installed."
> -		die "Please install dev-util/ninja or unset CMAKE_MAKEFILE_GENERATOR."
> -	fi
> -
>  	local modules_list
>  	if [[ $(declare -p CMAKE_REMOVE_MODULES_LIST) == "declare -a"* ]]; then
>  		modules_list=( "${CMAKE_REMOVE_MODULES_LIST[@]}" )
> @@ -534,7 +527,7 @@ cmake_src_configure() {
>  
> 
>  	local generator_name
>  	case ${CMAKE_MAKEFILE_GENERATOR} in
> -		ninja) generator_name="Ninja" ;;
> +		eninja|ninja) generator_name="Ninja" ;;
>  		emake) generator_name="Unix Makefiles" ;;
>  	esac
>  
> 
> @@ -592,8 +585,9 @@ cmake_build() {
>  				*) emake VERBOSE=1 "$@" ;;
>  			esac
>  			;;
> -		ninja)
> +		eninja|ninja)
>  			[[ -e build.ninja ]] || die "build.ninja not found. Error during configure stage."
> +			CMAKE_MAKEFILE_GENERATOR=eninja
>  			eninja "$@"
>  			;;
>  	esac
> diff --git a/eclass/meson.eclass b/eclass/meson.eclass
> index d0ce5adb355..ea02402aa83 100644
> --- a/eclass/meson.eclass
> +++ b/eclass/meson.eclass
> @@ -1,4 +1,4 @@
> -# Copyright 2017-2020 Gentoo Authors
> +# Copyright 2017-2021 Gentoo Authors
>  # Distributed under the terms of the GNU General Public License v2
>  
> 
>  # @ECLASS: meson.eclass
> @@ -55,7 +55,7 @@ if [[ -z ${_MESON_ECLASS} ]]; then
>  _MESON_ECLASS=1
>  
> 
>  MESON_DEPEND=">=dev-util/meson-0.54.0
> -	>=dev-util/ninja-1.8.2
> +	|| ( >=dev-util/ninja-1.8.2 dev-util/samurai )
>  	dev-util/meson-format-array
>  "

Same as for cmake.

>  
> 
> diff --git a/eclass/ninja-utils.eclass b/eclass/ninja-utils.eclass
> index ca8d67191dc..5008564dabf 100644
> --- a/eclass/ninja-utils.eclass
> +++ b/eclass/ninja-utils.eclass
> @@ -1,4 +1,4 @@
> -# Copyright 1999-2018 Gentoo Foundation
> +# Copyright 1999-2021 Gentoo Authors
>  # Distributed under the terms of the GNU General Public License v2
>  
> 
>  # @ECLASS: ninja-utils.eclass
> @@ -27,6 +27,15 @@ case ${EAPI:-0} in
>  	*) die "EAPI=${EAPI} is not yet supported" ;;
>  esac
>  
> 
> +# @ECLASS-VARIABLE: NINJA
> +# @PRE_INHERIT
> +# @DEFAULT_UNSET

'Default unset' conflicts with the default set below.

> +# @DESCRIPTION:
> +# Specify a compatible ninja implementation to be used by eninja.
> +# At this point only "ninja" and "samu" are supported.
> +# The default is set to "ninja".
> +: ${NINJA:=ninja}
> +
>  # @ECLASS-VARIABLE: NINJAOPTS
>  # @DEFAULT_UNSET
>  # @DESCRIPTION:
> @@ -36,6 +45,30 @@ esac
>  
> 
>  inherit multiprocessing
>  
> 
> +_ninja_to_use() {
> +	case "${NINJA}" in
> +		ninja)
> +			local ninja=dev-util/${NINJA}

What's the gain from this variable substitution, exactly?

Using the same variable name in a different case is a sure way towards
a disaster.

> +		;;
> +		samu)
> +			local ninja=dev-util/samurai
> +		;;
> +		*)
> +			eerror "Unknown value for \${NINJA}"
> +			die "Value ${NINJA} is not supported"
> +		;;
> +	esac
> +
> +	# if ninja or samurai are enabled but not installed, the build could fail
> +	# this could happen if they are manually enabled (eg. make.conf) but not installed
> +	if ! has_version -b ${ninja}; then
> +		eerror "Value ${NINJA} for \${NINJA} is not installed"
> +		die "Please install ${ninja}"
> +	fi
> +
> +	echo ${NINJA}
> +}
> +
>  # @FUNCTION: eninja
>  # @USAGE: [<args>...]
>  # @DESCRIPTION:
> @@ -49,7 +82,7 @@ eninja() {
>  	if [[ -z ${NINJAOPTS+set} ]]; then
>  		NINJAOPTS="-j$(makeopts_jobs) -l$(makeopts_loadavg "${MAKEOPTS}" 0)"
>  	fi
> -	set -- ninja -v ${NINJAOPTS} "$@"
> +	set -- "$(_ninja_to_use)" -v ${NINJAOPTS} "$@"
>  	echo "$@" >&2
>  	"$@" || die "${nonfatal_args[@]}" "${*} failed"
>  }

Overall, this patch is very bad.  You're combining two incompatible
concepts.  On one hand, cmake and meson eclasses claim they accept
either ninja and samurai.  On the other, the underlying ninja-utils
only accepts whatever happens to be in NINJA.

In other words, if someone happens to uninstall ninja and install
samurai, everything's going to explode because ninja is not installed.
If someone sets NINJA=samu and installs ninja, emerge will eventually
depclean samurai and have things explode again.

If ninja and samurai are really drop-in replacements, then remove
the whole explicit NINJA var nonsense, and have the eclass check which
of the implementations is actually installed.  And test your patches,
for all combinations that need to work.

-- 
Best regards,
Michał Górny