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 |