Gentoo Archives: gentoo-dev

From: "Michał Górny" <mgorny@g.o>
To: gentoo-dev@l.g.o
Cc: orbea <orbea@××××××.net>, Lars Wendler <polynomial-c@g.o>
Subject: Re: [gentoo-dev] (resent) [PATCH] eclass: Support dev-util/samurai
Date: Fri, 09 Apr 2021 17:22:22
Message-Id: e31ce7ae54d479578ba4b79dd8027973d29b04fe.camel@gentoo.org
In Reply to: [gentoo-dev] (resent) [PATCH] eclass: Support dev-util/samurai by Lars Wendler
1 Hi,
2
3 First things first:
4
5 - there should be a detailed commit message explaning what samurai is,
6 to what degree it is compatible with ninja and what's happening here
7
8 - this should be split to one patch per eclass.
9
10
11 On Fri, 2021-04-09 at 18:50 +0200, Lars Wendler wrote:
12 > From: orbea <orbea@××××××.net>
13 >
14 > Signed-off-by: Lars Wendler <polynomial-c@g.o>
15 > ---
16 >  eclass/cmake.eclass | 22 ++++++++--------------
17 >  eclass/meson.eclass | 4 ++--
18 >  eclass/ninja-utils.eclass | 37 +++++++++++++++++++++++++++++++++++--
19 >  3 files changed, 45 insertions(+), 18 deletions(-)
20 >
21 > diff --git a/eclass/cmake.eclass b/eclass/cmake.eclass
22 > index 4bd09459ea6..935ee1d8c81 100644
23 > --- a/eclass/cmake.eclass
24 > +++ b/eclass/cmake.eclass
25 > @@ -52,9 +52,9 @@ _CMAKE_ECLASS=1
26 >  # @DEFAULT_UNSET
27 >  # @DESCRIPTION:
28 >  # Specify a makefile generator to be used by cmake.
29 > -# At this point only "emake" and "ninja" are supported.
30 > -# The default is set to "ninja".
31 > -: ${CMAKE_MAKEFILE_GENERATOR:=ninja}
32 > +# At this point only "emake", "eninja" and "ninja" are supported.
33 > +# The default is set to "eninja".
34 > +: ${CMAKE_MAKEFILE_GENERATOR:=eninja}
35
36 At this point, this gets really confusing to end users (what is
37 the difference between 'ninja' and 'eninja')? Also, adding a new value
38 and changing the default breaks ebuild that run code conditionally
39 on CMAKE_MAKEFILE_GENERATOR.
40
41 >  
42 >
43 >  # @ECLASS-VARIABLE: CMAKE_REMOVE_MODULES_LIST
44 >  # @DESCRIPTION:
45 > @@ -117,8 +117,8 @@ case ${CMAKE_MAKEFILE_GENERATOR} in
46 >   emake)
47 >   BDEPEND="sys-devel/make"
48 >   ;;
49 > - ninja)
50 > - BDEPEND="dev-util/ninja"
51 > + eninja|ninja)
52 > + BDEPEND="|| ( dev-util/ninja dev-util/samurai )"
53
54 With hardcoded 'ninja' call, I doubt this is going to be true
55 for 'ninja' choice.
56
57 Also, this isn't going to work anyway since you don't check which one
58 is installed anywhere anyway.
59
60 >   ;;
61 >   *)
62 >   eerror "Unknown value for \${CMAKE_MAKEFILE_GENERATOR}"
63 > @@ -335,13 +335,6 @@ cmake_src_prepare() {
64 >   die "FATAL: Unable to find CMakeLists.txt"
65 >   fi
66 >  
67 >
68 > - # if ninja is enabled but not installed, the build could fail
69 > - # this could happen if ninja is manually enabled (eg. make.conf) but not installed
70 > - if [[ ${CMAKE_MAKEFILE_GENERATOR} == ninja ]] && ! has_version -b dev-util/ninja; then
71 > - eerror "CMAKE_MAKEFILE_GENERATOR is set to ninja, but ninja is not installed."
72 > - die "Please install dev-util/ninja or unset CMAKE_MAKEFILE_GENERATOR."
73 > - fi
74 > -
75 >   local modules_list
76 >   if [[ $(declare -p CMAKE_REMOVE_MODULES_LIST) == "declare -a"* ]]; then
77 >   modules_list=( "${CMAKE_REMOVE_MODULES_LIST[@]}" )
78 > @@ -534,7 +527,7 @@ cmake_src_configure() {
79 >  
80 >
81 >   local generator_name
82 >   case ${CMAKE_MAKEFILE_GENERATOR} in
83 > - ninja) generator_name="Ninja" ;;
84 > + eninja|ninja) generator_name="Ninja" ;;
85 >   emake) generator_name="Unix Makefiles" ;;
86 >   esac
87 >  
88 >
89 > @@ -592,8 +585,9 @@ cmake_build() {
90 >   *) emake VERBOSE=1 "$@" ;;
91 >   esac
92 >   ;;
93 > - ninja)
94 > + eninja|ninja)
95 >   [[ -e build.ninja ]] || die "build.ninja not found. Error during configure stage."
96 > + CMAKE_MAKEFILE_GENERATOR=eninja
97 >   eninja "$@"
98 >   ;;
99 >   esac
100 > diff --git a/eclass/meson.eclass b/eclass/meson.eclass
101 > index d0ce5adb355..ea02402aa83 100644
102 > --- a/eclass/meson.eclass
103 > +++ b/eclass/meson.eclass
104 > @@ -1,4 +1,4 @@
105 > -# Copyright 2017-2020 Gentoo Authors
106 > +# Copyright 2017-2021 Gentoo Authors
107 >  # Distributed under the terms of the GNU General Public License v2
108 >  
109 >
110 >  # @ECLASS: meson.eclass
111 > @@ -55,7 +55,7 @@ if [[ -z ${_MESON_ECLASS} ]]; then
112 >  _MESON_ECLASS=1
113 >  
114 >
115 >  MESON_DEPEND=">=dev-util/meson-0.54.0
116 > - >=dev-util/ninja-1.8.2
117 > + || ( >=dev-util/ninja-1.8.2 dev-util/samurai )
118 >   dev-util/meson-format-array
119 >  "
120
121 Same as for cmake.
122
123 >  
124 >
125 > diff --git a/eclass/ninja-utils.eclass b/eclass/ninja-utils.eclass
126 > index ca8d67191dc..5008564dabf 100644
127 > --- a/eclass/ninja-utils.eclass
128 > +++ b/eclass/ninja-utils.eclass
129 > @@ -1,4 +1,4 @@
130 > -# Copyright 1999-2018 Gentoo Foundation
131 > +# Copyright 1999-2021 Gentoo Authors
132 >  # Distributed under the terms of the GNU General Public License v2
133 >  
134 >
135 >  # @ECLASS: ninja-utils.eclass
136 > @@ -27,6 +27,15 @@ case ${EAPI:-0} in
137 >   *) die "EAPI=${EAPI} is not yet supported" ;;
138 >  esac
139 >  
140 >
141 > +# @ECLASS-VARIABLE: NINJA
142 > +# @PRE_INHERIT
143 > +# @DEFAULT_UNSET
144
145 'Default unset' conflicts with the default set below.
146
147 > +# @DESCRIPTION:
148 > +# Specify a compatible ninja implementation to be used by eninja.
149 > +# At this point only "ninja" and "samu" are supported.
150 > +# The default is set to "ninja".
151 > +: ${NINJA:=ninja}
152 > +
153 >  # @ECLASS-VARIABLE: NINJAOPTS
154 >  # @DEFAULT_UNSET
155 >  # @DESCRIPTION:
156 > @@ -36,6 +45,30 @@ esac
157 >  
158 >
159 >  inherit multiprocessing
160 >  
161 >
162 > +_ninja_to_use() {
163 > + case "${NINJA}" in
164 > + ninja)
165 > + local ninja=dev-util/${NINJA}
166
167 What's the gain from this variable substitution, exactly?
168
169 Using the same variable name in a different case is a sure way towards
170 a disaster.
171
172 > + ;;
173 > + samu)
174 > + local ninja=dev-util/samurai
175 > + ;;
176 > + *)
177 > + eerror "Unknown value for \${NINJA}"
178 > + die "Value ${NINJA} is not supported"
179 > + ;;
180 > + esac
181 > +
182 > + # if ninja or samurai are enabled but not installed, the build could fail
183 > + # this could happen if they are manually enabled (eg. make.conf) but not installed
184 > + if ! has_version -b ${ninja}; then
185 > + eerror "Value ${NINJA} for \${NINJA} is not installed"
186 > + die "Please install ${ninja}"
187 > + fi
188 > +
189 > + echo ${NINJA}
190 > +}
191 > +
192 >  # @FUNCTION: eninja
193 >  # @USAGE: [<args>...]
194 >  # @DESCRIPTION:
195 > @@ -49,7 +82,7 @@ eninja() {
196 >   if [[ -z ${NINJAOPTS+set} ]]; then
197 >   NINJAOPTS="-j$(makeopts_jobs) -l$(makeopts_loadavg "${MAKEOPTS}" 0)"
198 >   fi
199 > - set -- ninja -v ${NINJAOPTS} "$@"
200 > + set -- "$(_ninja_to_use)" -v ${NINJAOPTS} "$@"
201 >   echo "$@" >&2
202 >   "$@" || die "${nonfatal_args[@]}" "${*} failed"
203 >  }
204
205 Overall, this patch is very bad. You're combining two incompatible
206 concepts. On one hand, cmake and meson eclasses claim they accept
207 either ninja and samurai. On the other, the underlying ninja-utils
208 only accepts whatever happens to be in NINJA.
209
210 In other words, if someone happens to uninstall ninja and install
211 samurai, everything's going to explode because ninja is not installed.
212 If someone sets NINJA=samu and installs ninja, emerge will eventually
213 depclean samurai and have things explode again.
214
215 If ninja and samurai are really drop-in replacements, then remove
216 the whole explicit NINJA var nonsense, and have the eclass check which
217 of the implementations is actually installed. And test your patches,
218 for all combinations that need to work.
219
220 --
221 Best regards,
222 Michał Górny