[Buildroot] [PATCH v6 1/4] pkg-cmake: add option to select the Ninja generator

Sun Aug 6 14:15:06 UTC 2023

Thomas, All,

On 2023-08-02 13:14 +0200, Thomas Devoogdt spake thusly:
> From: Thomas Devoogdt <thomas.devoogdt at barco.com>
> 
> Cmake supports multiple generators. Ninja is a small build system with a
> focus on speed. It is mainly used with the meson build system, but also
> cmake has very good support for it. This adds optional support for the
> Ninja build system.
> 
> Usage:
> <package>_CMAKE_NINJA = YES

With Thomas P., we concluded that this is not a YES/NO question, but
it's rather a THIS-OR-THAT-OR-THIS where we only have a this or a that,
but nonetheless that's semantically different.

So I've tweaked the code to introduce FOO_CMAKE_BACKEND = (make|ninja).
This way it is even more obvious how we'll be able to add other backends
in the future (even if Thomas P. and I doubt we will ever have more,
seeing what cmake currently has that would be meaningful for Buildroot).

> E.g. Commit 16e5c92ff5fd2b44a1126bd7d7538c68ce838213 can now be replaced by:
> WEBKITGTK_CMAKE_NINJA = YES
> 
> Packages that are selecting Ninja (or overtime another generator),
> should also use the _BUILD_{ENV,OPTS} variables iso the _MAKE variables.
> 
> No _INSTALL{,_STAGING,_TARGET}_OPTS used so far, so reuse as cmake install opts:
> 
>     $ grep '_INSTALL_OPTS' $(grep -rl "cmake-package" package/*/*.mk)
>     $ grep '_INSTALL_STAGING_OPTS' $(grep -rl "cmake-package" package/*/*.mk)
>     $ grep '_INSTALL_TARGET_OPTS' $(grep -rl "cmake-package" package/*/*.mk)
> 
> The _MAKE_{ENV,OPTS} are copied to _BUILD_{ENV,OPTS}, involved packages:
> 
>     $ grep '_MAKE_ENV =' $(grep -rl "cmake-package" package/*/*.mk)
> 
>     package/netopeer2/netopeer2.mk:NETOPEER2_MAKE_ENV = \
>     package/racehound/racehound.mk:RACEHOUND_MAKE_ENV = $(LINUX_MAKE_FLAGS)
> 
>     $ grep '_MAKE_OPTS =' $(grep -rl "cmake-package" package/*/*.mk)
> 
>     package/mariadb/mariadb.mk:HOST_MARIADB_MAKE_OPTS = import_executables
>     package/zeek/zeek.mk:HOST_ZEEK_MAKE_OPTS = binpac bifcl
> 
> Only "musepack" seems to overwrite MAKE to enforce -j1, so replace it:
> 
>     $ grep '_MAKE =' $(grep -rl "cmake-package" package/*/*.mk)
> 
>     package/musepack/musepack.mk:MUSEPACK_MAKE = $(MAKE1)
> 
> Signed-off-by: Thomas Devoogdt <thomas.devoogdt at barco.com>
> Reviewed-by: John Keeping <john at metanate.com>
> ---
> v2:
>  - made generator use more generic, other generators can now easily be added if required
> v3:
>  - add _GENERATOR_PROGRAM
>  - add _GENERATOR_PARALLEL for make
>  - dropped BUILD_OPTS
>  - fix gdal.mk

It took me some time to understand what you meant here, as gdal was in
fact not touched by your patch. IIUC, you really meant that, by
propagating the _MAKE_OPTS to the new _BUILD_OPTS, that would fix gdal.

Good news, I actually fixed gdal to drop the remnants of a previous
workaround! ;-)

Still, I kept the propagation to _BUILD_OPTS for out-of-tree packages.

> v4:
>  - restored _MAKE_ENV/_MAKE_OPTS for the Unix Makefiles case
>  - always set -j$(PARALLEL_JOBS)

I left this as-is, but please see 1668e1da390c (packages: fix and
improve support for top-level parallel make) and review the commit log
to see how this is applies now...

[--SNIP--]
> diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
> index 8c375779cb..36ab88d3a1 100644
> --- a/package/pkg-cmake.mk
> +++ b/package/pkg-cmake.mk
> @@ -51,11 +51,6 @@ endif
>  
>  define inner-cmake-package
>  
> -$(2)_MAKE			?= $$(MAKE)
> -$(2)_INSTALL_OPTS		?= install
> -$(2)_INSTALL_STAGING_OPTS	?= DESTDIR=$$(STAGING_DIR) install/fast
> -$(2)_INSTALL_TARGET_OPTS	?= DESTDIR=$$(TARGET_DIR) install/fast
> -
>  $(3)_SUPPORTS_IN_SOURCE_BUILD ?= YES
>  
>  
> @@ -65,6 +60,20 @@ else
>  $(2)_BUILDDIR			= $$($(2)_SRCDIR)/buildroot-build
>  endif
>  
> +ifeq ($$($(3)_CMAKE_NINJA),YES)

I've kept the 'make' generator first, for two reasons: 1. it is the
historical backend, so that's nice that we keep it first in the list,
and add new backends below, and 2. that keeps the list alphabetically
ordered (both are weak reasons, but together they are stringer! ;-])

> +$(2)_DEPENDENCIES	+= host-ninja
> +$(2)_GENERATOR		= "Ninja"
> +$(2)_GENERATOR_PROGRAM	= $(HOST_DIR)/bin/ninja
> +else
> +$(2)_GENERATOR		= "Unix Makefiles"
> +$(2)_GENERATOR_PROGRAM	= $(firstword $(BR2_MAKE))

You did not explain why we needed to use the firstword of $(BR2_MAKE),
nor why we needed to use $(BR2_MAKE) instead of $(MAKE) which is used
everywhere else.

There was no reason that I could spot for BR2_MAKE, so I switched to
MAKE, and I added a blurb about using firstword.

If I missed something, please send a fixup patch with appropriate
explanations.

> +# Generator specific code (make) should be avoided,
> +# but for now, copy them to the new variables.
> +$(2)_BUILD_ENV		?= $$($(2)_MAKE_ENV)
> +$(2)_BUILD_OPTS		?= -- $$($(2)_MAKE_OPTS)

I used an if-elseif-else construct, to detect unsupported backends.

Finally, there was no documentation in the manual for those three new
options, so I've added them.

Please review the new code. If I broke something, please send followup
fixup patches with appropriate explanations (there's a kind of déjà-vu
here...)

Applied to next, thanks.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'