diff mbox

[1/2] Add BR2_CMAKE_USE_NINJA_BACKEND option

Message ID 20170106223748.2203-1-cedric.marie@openmailbox.org
State Changes Requested
Headers show

Commit Message

Cédric Marie Jan. 6, 2017, 10:37 p.m. UTC
CMake provides several backends. The default one is Make, but Ninja is
also supported. Ninja is a small build system with a focus on speed.

If the new option BR2_CMAKE_USE_NINJA_BACKEND is enabled, CMake will
use Ninja backend.

In CMake package infrastructure, when this option is set:
- Add host-ninja dependency
- Add "-G Ninja" option in CMake configure step
- Use ninja command instead of make ($(MAKE))

Most of make arguments are compatible with ninja command. But there are
a few differences:
- Environment variables such as DESTDIR cannot be given in arguments.
  Instead they must be set before the command (which is compatible with
  make)
- CMake does not handle VERBOSE variable with Ninja backend. Instead,
  we must add -v option when VERBOSE is set.
- install/fast target is specific to make backend. With ninja backend,
  install target must be used (and it will not try to compile as make
  backend does).

Tested on following packages: cannelloni, graphite2, libcuefile,
libubox, rabbitmq-c, ubus.

Signed-off-by: Cédric Marie <cedric.marie@openmailbox.org>
---
 Config.in            |  6 ++++++
 package/pkg-cmake.mk | 26 ++++++++++++++++++++------
 2 files changed, 26 insertions(+), 6 deletions(-)

Comments

Romain Naour Jan. 21, 2017, 10:25 p.m. UTC | #1
Hi Cédric,

Le 06/01/2017 à 23:37, Cédric Marie a écrit :
> CMake provides several backends. The default one is Make, but Ninja is
> also supported. Ninja is a small build system with a focus on speed.
> 
> If the new option BR2_CMAKE_USE_NINJA_BACKEND is enabled, CMake will
> use Ninja backend.
> 
> In CMake package infrastructure, when this option is set:
> - Add host-ninja dependency
> - Add "-G Ninja" option in CMake configure step
> - Use ninja command instead of make ($(MAKE))
> 
> Most of make arguments are compatible with ninja command. But there are
> a few differences:
> - Environment variables such as DESTDIR cannot be given in arguments.
>   Instead they must be set before the command (which is compatible with
>   make)
> - CMake does not handle VERBOSE variable with Ninja backend. Instead,
>   we must add -v option when VERBOSE is set.
> - install/fast target is specific to make backend. With ninja backend,
>   install target must be used (and it will not try to compile as make
>   backend does).
> 
> Tested on following packages: cannelloni, graphite2, libcuefile,
> libubox, rabbitmq-c, ubus.

There are currently 146 packages using CMake in Buildroot, I'm not sure all of
them support ninja backend yet.

I would suggest to enable Ninja backend package by package when it has been tested.

> 
> Signed-off-by: Cédric Marie <cedric.marie@openmailbox.org>
> ---
>  Config.in            |  6 ++++++
>  package/pkg-cmake.mk | 26 ++++++++++++++++++++------
>  2 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/Config.in b/Config.in
> index ccd777e..26ebcf6 100644
> --- a/Config.in
> +++ b/Config.in
> @@ -656,6 +656,12 @@ config BR2_SHARED_STATIC_LIBS
>  
>  endchoice
>  
> +config BR2_CMAKE_USE_NINJA_BACKEND
> +	bool "Compile CMake packages with Ninja backend"
> +        help

Indent with one tab.

> +          CMake provides several backends. The default one is Make, but
> +          Ninja is also supported. Ninja is a small build system with a
> +          focus on speed.

Indent with one tab and 2 spaces.

>  
>  config BR2_PACKAGE_OVERRIDE_FILE
>  	string "location of a package override file"
> diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
> index 4e0e838..83bf79e 100644
> --- a/package/pkg-cmake.mk
> +++ b/package/pkg-cmake.mk
> @@ -53,24 +53,35 @@ define inner-cmake-package
>  
>  $(2)_CONF_ENV			?=
>  $(2)_CONF_OPTS			?=
> +$(2)_INSTALL_STAGING_ENV	?= DESTDIR=$$(STAGING_DIR)
> +$(2)_INSTALL_TARGET_ENV		?= DESTDIR=$$(TARGET_DIR)
> +
> +ifeq ($(BR2_CMAKE_USE_NINJA_BACKEND),y)
> +$(2)_MAKE			?= $$(HOST_DIR)/usr/bin/ninja
> +$(2)_MAKE_ENV			?=
> +$(2)_MAKE_OPTS			?= $(if $(VERBOSE),-v)
> +$(2)_INSTALL_OPTS		?= install
> +else
>  $(2)_MAKE			?= $$(MAKE)
>  $(2)_MAKE_ENV			?=
>  $(2)_MAKE_OPTS			?=
> -$(2)_INSTALL_OPTS		?= install
> -$(2)_INSTALL_STAGING_OPTS	?= DESTDIR=$$(STAGING_DIR) install/fast
> -$(2)_INSTALL_TARGET_OPTS		?= DESTDIR=$$(TARGET_DIR) install/fast
> +$(2)_INSTALL_OPTS		?= install/fast

I think these changes should be in a preparatory patch before adding Ninja
backend support.

Best regards,
Romain

> +endif
>  
>  $(2)_SRCDIR			= $$($(2)_DIR)/$$($(2)_SUBDIR)
>  
>  $(3)_SUPPORTS_IN_SOURCE_BUILD ?= YES
>  
> -
>  ifeq ($$($(3)_SUPPORTS_IN_SOURCE_BUILD),YES)
>  $(2)_BUILDDIR			= $$($(2)_SRCDIR)
>  else
>  $(2)_BUILDDIR			= $$($(2)_SRCDIR)/buildroot-build
>  endif
>  
> +ifeq ($(BR2_CMAKE_USE_NINJA_BACKEND),y)
> +$(2)_CONF_OPTS += -G Ninja
> +endif
> +
>  #
>  # Configure step. Only define it if not already defined by the package
>  # .mk file. And take care of the differences between host and target
> @@ -146,6 +157,9 @@ endif
>  $(2)_DEPENDENCIES += host-pkgconf
>  
>  $(2)_DEPENDENCIES += $(BR2_CMAKE_HOST_DEPENDENCY)
> +ifeq ($(BR2_CMAKE_USE_NINJA_BACKEND),y)
> +$(2)_DEPENDENCIES += host-ninja
> +endif
>  
>  #
>  # Build step. Only define it if not already defined by the package .mk
> @@ -179,7 +193,7 @@ endif
>  #
>  ifndef $(2)_INSTALL_STAGING_CMDS
>  define $(2)_INSTALL_STAGING_CMDS
> -	$$(TARGET_MAKE_ENV) $$($$(PKG)_MAKE_ENV) $$($$(PKG)_MAKE) $$($$(PKG)_MAKE_OPTS) $$($$(PKG)_INSTALL_STAGING_OPTS) -C $$($$(PKG)_BUILDDIR)
> +	$$(TARGET_MAKE_ENV) $$($$(PKG)_MAKE_ENV) $$($$(PKG)_INSTALL_STAGING_ENV) $$($$(PKG)_MAKE) $$($$(PKG)_MAKE_OPTS) $$($$(PKG)_INSTALL_OPTS) -C $$($$(PKG)_BUILDDIR)
>  endef
>  endif
>  
> @@ -189,7 +203,7 @@ endif
>  #
>  ifndef $(2)_INSTALL_TARGET_CMDS
>  define $(2)_INSTALL_TARGET_CMDS
> -	$$(TARGET_MAKE_ENV) $$($$(PKG)_MAKE_ENV) $$($$(PKG)_MAKE) $$($$(PKG)_MAKE_OPTS) $$($$(PKG)_INSTALL_TARGET_OPTS) -C $$($$(PKG)_BUILDDIR)
> +	$$(TARGET_MAKE_ENV) $$($$(PKG)_MAKE_ENV) $$($$(PKG)_INSTALL_TARGET_ENV) $$($$(PKG)_MAKE) $$($$(PKG)_MAKE_OPTS) $$($$(PKG)_INSTALL_OPTS) -C $$($$(PKG)_BUILDDIR)
>  endef
>  endif
>  
>
Cédric Marie Jan. 23, 2017, 1:39 p.m. UTC | #2
Hi,

Le 2017-01-21 23:25, Romain Naour a écrit :
> There are currently 146 packages using CMake in Buildroot, I'm not sure 
> all of
> them support ninja backend yet.

I don't think there is anything specific to a package that can make it 
support ninja backend or not.
CMake provides several backends, and the result is expected to be the 
same whatever the backend. If not, this is a bug in CMake.

> I would suggest to enable Ninja backend package by package when it has
> been tested.

What do you mean exactly "enable Ninja backend package by package"?
- Add an option for each package?
- Switch to ninja backend if supported by the package, without option 
(i.e. force ninja backend)?
- Keep a global option - the one in my patch - but only apply it on 
packages that will define a specific package variable to say "OK I can 
do it"?

My point of view is that we should keep a global option - that could be 
described as experimental - and never enable it in an official 
defconfig.
If someone enables it, and find a package that does not compile with 
ninja backend, he can either disable the option (he knows that it is 
experimental), or/and send a bug to either CMake or the package 
maintainer.

Moreover, I think we can't just say "this package is ninja compliant". 
Maybe if we change a single option in configure step, it will not 
compile anymore.

Let me know what you're expecting exactly...

> Indent with one tab.
> Indent with one tab and 2 spaces.

OK, sorry for that, didn't pay enough attention.


> I think these changes should be in a preparatory patch before adding 
> Ninja
> backend support.

Sorry I don't understand what you mean with "preparatory patch".

Regards,
Romain Naour Jan. 24, 2017, 9:48 p.m. UTC | #3
Hi Cédric,

Le 23/01/2017 à 14:39, Cédric Marie a écrit :
> Hi,
> 
> Le 2017-01-21 23:25, Romain Naour a écrit :
>> There are currently 146 packages using CMake in Buildroot, I'm not sure all of
>> them support ninja backend yet.
> 
> I don't think there is anything specific to a package that can make it support
> ninja backend or not.
> CMake provides several backends, and the result is expected to be the same
> whatever the backend. If not, this is a bug in CMake.

Ok.

> 
>> I would suggest to enable Ninja backend package by package when it has
>> been tested.
> 
> What do you mean exactly "enable Ninja backend package by package"?
> - Add an option for each package?

no

> - Switch to ninja backend if supported by the package, without option (i.e.
> force ninja backend)?

no

> - Keep a global option - the one in my patch - but only apply it on packages
> that will define a specific package variable to say "OK I can do it"?

Yes, something like that but It's just a proposal...

<package>_SUPPORT_NINJA_BACKEND = YES in the .mk
(with default to N0)

> 
> My point of view is that we should keep a global option - that could be
> described as experimental - and never enable it in an official defconfig.
> If someone enables it, and find a package that does not compile with ninja
> backend, he can either disable the option (he knows that it is experimental),
> or/and send a bug to either CMake or the package maintainer.

Keep in mind that this new option will certainly be enabled in the generated
defconfigs for the autobuilders. So it may break several packages quickly.

For example, the openpowerlink package break with Ninja backend but I think it's
a bug in the CMakeLists.txt. I don't think openpowerlink maintainers ever used
Ninja and me neither until now. I'll take a look.

> 
> Moreover, I think we can't just say "this package is ninja compliant". Maybe if
> we change a single option in configure step, it will not compile anymore.

Maybe something like this (untested):

ifeq ($(BR2_CMAKE_USE_NINJA_BACKEND),y)
ifeq ($$($(2)_SUPPORT_NINJA_BACKEND),YES)
$(2)_CONF_OPTS += -G Ninja
$(2)_DEPENDENCIES += host-ninja
endif
endif

It's really a proposal, If you're not convinced let's see other comments/reviews.

Also, did you do some time measurements with/without Ninja ?

Here are some results on my host:

make menuconfig (enable/disable the Ninja option)
make <package> (first build from scratch)
make <package>-dirclean
time make <package>-build

w/ Ninja

cannelloni
real	0m10.009s
user	0m8.771s
sys	0m0.885s

bullet
real	0m43.750s
user	1m4.464s
sys	0m4.356s

w/ Make

cannelloni
real	0m11.478s
user	0m9.282s
sys	0m1.127s

bullet
real	0m45.234s
user	1m4.213s
sys	0m5.501s

So, yes Ninja is slightly faster than Make.
It would be great if you can add some test results in the commit log.

> 
> Let me know what you're expecting exactly...
> 
>> Indent with one tab.
>> Indent with one tab and 2 spaces.
> 
> OK, sorry for that, didn't pay enough attention.

No problem.

> 
> 
>> I think these changes should be in a preparatory patch before adding Ninja
>> backend support.
> 
> Sorry I don't understand what you mean with "preparatory patch".

Sorry, I meant you can do a first patch with these changes in pkg-cmake.mk
(refactoring) and add the Ninja in a second patch.
I suggest to do this since it impact the cmake with Make backend build.

Best regards,
Romain

> 
> Regards,
>
Thomas Petazzoni Jan. 25, 2017, 1:27 a.m. UTC | #4
Hello,

On Tue, 24 Jan 2017 22:48:08 +0100, Romain Naour wrote:

> > - Keep a global option - the one in my patch - but only apply it on packages
> > that will define a specific package variable to say "OK I can do it"?  
> 
> Yes, something like that but It's just a proposal...
> 
> <package>_SUPPORT_NINJA_BACKEND = YES in the .mk
> (with default to N0)

But then that's a bit annoying because we would have to explicitly
set this option to "YES" on all packages that support the Ninja backend
(most likely the majority).

I think it would make more sense to default the other way around, i.e
default to YES, and set it to NO on the few packages that do not
properly support the Ninja backend.

Or maybe better: do not introduce a per-package option for the moment,
have only the global one, see in practice how many packages work /
don't work and decide if we need a per-package option, and what default
value it should have.

> w/ Ninja
> 
> cannelloni
> real	0m10.009s
> user	0m8.771s
> sys	0m0.885s
> 
> bullet
> real	0m43.750s
> user	1m4.464s
> sys	0m4.356s
> 
> w/ Make
> 
> cannelloni
> real	0m11.478s
> user	0m9.282s
> sys	0m1.127s
> 
> bullet
> real	0m45.234s
> user	1m4.213s
> sys	0m5.501s

So we're saving between 1 and 2 seconds of build time, while host-ninja
requires building host-python or host-python3 ?

Seeing those numbers, the whole thing seems really pointless to me.
You're saving 1-2 seconds of build time, but you've got a full Python
interpreter to build.

Would it be possible to get numbers on the overall build time showing
what Ninja is improving?

> >> I think these changes should be in a preparatory patch before adding Ninja
> >> backend support.  
> > 
> > Sorry I don't understand what you mean with "preparatory patch".  
> 
> Sorry, I meant you can do a first patch with these changes in pkg-cmake.mk
> (refactoring) and add the Ninja in a second patch.
> I suggest to do this since it impact the cmake with Make backend build.

I looked again at the patch, and I don't really see where it can be
split in two parts. Which changes do you see as "preparatory" ?

Thanks!

Thomas
Thomas Petazzoni Jan. 25, 2017, 1:37 a.m. UTC | #5
Hello,

On Fri,  6 Jan 2017 23:37:47 +0100, Cédric Marie wrote:
>  $(2)_CONF_ENV			?=
>  $(2)_CONF_OPTS			?=
> +$(2)_INSTALL_STAGING_ENV	?= DESTDIR=$$(STAGING_DIR)
> +$(2)_INSTALL_TARGET_ENV		?= DESTDIR=$$(TARGET_DIR)

We don't have such options in any other package infrastructure. So I
think I'd prefer if you kept just INSTALL_STAGING_OPTS and
INSTALL_TARGET_OPTS, and define them to the appropriate value depending
on whether you're using Ninja or Make.

Are there any existing CMake package that overrides
INSTALL_STAGING_OPTS or INSTALL_TARGET_OPTS ? If that's the case, then
we cannot change its semantic like this without taking care of those
packages first.

But as I said in my other reply on this thread, I'd really like to get
some justification of why adding support for the Ninja backend is
useful.

Thanks!

Thomas
Cédric Marie Jan. 26, 2017, 5:27 p.m. UTC | #6
Hi Romain, Thomas, and all,

Le 2017-01-24 22:48, Romain Naour a écrit :
> Yes, something like that but It's just a proposal...
> 
> <package>_SUPPORT_NINJA_BACKEND = YES in the .mk
> (with default to N0)
> 
> ifeq ($(BR2_CMAKE_USE_NINJA_BACKEND),y)
> ifeq ($$($(2)_SUPPORT_NINJA_BACKEND),YES)
> $(2)_CONF_OPTS += -G Ninja
> $(2)_DEPENDENCIES += host-ninja
> endif
> endif

Yes, that could be a good solution to use ninja whenever possible and 
keep other few packages compiling with make backend.

> So, yes Ninja is slightly faster than Make.
> It would be great if you can add some test results in the commit log.

OK, I can do that.


Le 2017-01-25 02:27, Thomas Petazzoni a écrit :
> But then that's a bit annoying because we would have to explicitly
> set this option to "YES" on all packages that support the Ninja backend
> (most likely the majority).
> 
> I think it would make more sense to default the other way around, i.e
> default to YES, and set it to NO on the few packages that do not
> properly support the Ninja backend.
> 
> Or maybe better: do not introduce a per-package option for the moment,
> have only the global one, see in practice how many packages work /
> don't work and decide if we need a per-package option, and what default
> value it should have.

OK, but it seems that Romain has already noticed a problem with 
openpowerlink, so it is likely that this option will be necessary - and 
hopefully default to YES (supported).

> So we're saving between 1 and 2 seconds of build time, while host-ninja
> requires building host-python or host-python3 ?
> 
> Seeing those numbers, the whole thing seems really pointless to me.
> You're saving 1-2 seconds of build time, but you've got a full Python
> interpreter to build.
> 
> Would it be possible to get numbers on the overall build time showing
> what Ninja is improving?

Yes, you're right. Unless we use python and ninja from the host if 
available, instead of compiling them, but this is not the right solution 
I suppose (although this is done for CMake!...).

I agree that a few seconds are not much!... And I agree that we are 
loosing much more time to compile host packages.

So, why did I want to introduce this option?

Because I'm not using Buildroot in one-shot mode. I'm developping in a 
Buildroot environment. So I often make foo-rebuild my package, and I'm 
interested in saving time for each rebuild.

I know this is not what Buildroot is designed for. (Yet, there are a few 
things that seem to be present for dev mode (rsync with OVERRIDE_SRCDIR 
for example), so this is not "pure one-shot" either.)

In conclusion, I would say that this option is useful to me - not much, 
just for a few seconds - but maybe makes no sense for buildroot 
upstream, since it increases build time.

It's up to you. I won't be offended if the patch is refused. I don't 
want to introduce something that will break Buildroot philosophy.

Regards,
Thomas Petazzoni Jan. 30, 2017, 9:23 a.m. UTC | #7
Hello,

On Thu, 26 Jan 2017 18:27:36 +0100, Cédric Marie wrote:

> Le 2017-01-25 02:27, Thomas Petazzoni a écrit :
> > But then that's a bit annoying because we would have to explicitly
> > set this option to "YES" on all packages that support the Ninja backend
> > (most likely the majority).
> > 
> > I think it would make more sense to default the other way around, i.e
> > default to YES, and set it to NO on the few packages that do not
> > properly support the Ninja backend.
> > 
> > Or maybe better: do not introduce a per-package option for the moment,
> > have only the global one, see in practice how many packages work /
> > don't work and decide if we need a per-package option, and what default
> > value it should have.  
> 
> OK, but it seems that Romain has already noticed a problem with 
> openpowerlink, so it is likely that this option will be necessary - and 
> hopefully default to YES (supported).

Indeed.

> So, why did I want to introduce this option?
> 
> Because I'm not using Buildroot in one-shot mode. I'm developping in a 
> Buildroot environment. So I often make foo-rebuild my package, and I'm 
> interested in saving time for each rebuild.

OK, understood. But do you have numbers showing that "make foo-rebuild"
is actually faster with Ninja ?

In the tests done by Romain, it's only saving 1-2 seconds on the
total build of a package. So I would suspect that the savings on a
partial build are even smaller.

Even if your CMake packages are private, can you give us some numbers
that show the time benefits of Ninja?

> I know this is not what Buildroot is designed for. (Yet, there are a few 
> things that seem to be present for dev mode (rsync with OVERRIDE_SRCDIR 
> for example), so this is not "pure one-shot" either.)

Well, if we do have features such as OVERRIDE_SRCDIR, it's precisely to
make Buildroot usable/useful also during the development. So if there
are things we can improve in this area, it's good to have.

Thanks,

Thomas
Cédric Marie Feb. 1, 2017, 5:01 p.m. UTC | #8
Hi,

Le 2017-01-30 10:23, Thomas Petazzoni a écrit :
> OK, understood. But do you have numbers showing that "make foo-rebuild"
> is actually faster with Ninja ?
> 
> In the tests done by Romain, it's only saving 1-2 seconds on the
> total build of a package. So I would suspect that the savings on a
> partial build are even smaller.
> 
> Even if your CMake packages are private, can you give us some numbers
> that show the time benefits of Ninja?

You're right, it is a private package, that's why I had to test on other 
packages I'm not particularly using, to demonstrate.

The benefit is rather small for my package too.

Make:
real	1m1.517s
user	2m23.996s
sys	0m36.197s

Ninja:
real	0m56.312s
user	2m19.078s
sys	0m29.119s

NB: It includes the whole "time make foo-rebuild" command, because time 
output is "strange" when inserted in pkg-cmake.mk (... time 
$$($$(PKG)_MAKE) ...), don't know why...
Looks like:
130.50user 19.88system 0:42.32elapsed 355%CPU (0avgtext+0avgdata 
24680maxresident)k
0inputs+50416outputs (0major+7602195minor)pagefaults 0swaps

To be honest, when I started to add ninja possibility, I expected much 
bigger savings :)
Yet I have the option, so even if the benefit is small, I keep using it.

But in the end, I don't know whether it's worth pushing upstream or 
not...

Besides Buildroot, I also switched another package from CMake/Make to 
Meson (based on Ninja), and the benefit was much more interesting - 
although I have not kept any measure to give here.
I expected the improvement to be caused by Ninja, not Meson. But it 
seems that Meson makes the difference, rather than Make vs Ninja.
Thomas Petazzoni Feb. 1, 2017, 8:12 p.m. UTC | #9
Hello,

On Wed, 01 Feb 2017 18:01:31 +0100, Cédric Marie wrote:

> You're right, it is a private package, that's why I had to test on other 
> packages I'm not particularly using, to demonstrate.
> 
> The benefit is rather small for my package too.
> 
> Make:
> real	1m1.517s
> user	2m23.996s
> sys	0m36.197s
> 
> Ninja:
> real	0m56.312s
> user	2m19.078s
> sys	0m29.119s

Indeed the benefit is quite small here.

> NB: It includes the whole "time make foo-rebuild" command, because time 
> output is "strange" when inserted in pkg-cmake.mk (... time 
> $$($$(PKG)_MAKE) ...), don't know why...
> Looks like:
> 130.50user 19.88system 0:42.32elapsed 355%CPU (0avgtext+0avgdata 
> 24680maxresident)k
> 0inputs+50416outputs (0major+7602195minor)pagefaults 0swaps

In the first case you're using the shell built-in "time" program, in
the later you're using the separate /usr/bin/time program. Try:

	/usr/bin/time make foo-rebuild

> To be honest, when I started to add ninja possibility, I expected much 
> bigger savings :)
> Yet I have the option, so even if the benefit is small, I keep using it.
> 
> But in the end, I don't know whether it's worth pushing upstream or 
> not...

I'm indeed not sure it's really worth the effort for such a small
saving.

> Besides Buildroot, I also switched another package from CMake/Make to 
> Meson (based on Ninja), and the benefit was much more interesting - 
> although I have not kept any measure to give here.
> I expected the improvement to be caused by Ninja, not Meson. But it 
> seems that Meson makes the difference, rather than Make vs Ninja.

We had some contribution some months ago to add support for Meson in
Buildroot. The problem was that the contribution was only composed of
host packages, not used by any target package in Buildroot itself.
Which means it was only adding dead code, that wasn't used at all by
Buildroot unless you have some in-house package that uses Meson.

Best regards,

Thomas
Cédric Marie Feb. 3, 2017, 10:44 a.m. UTC | #10
Le 2017-02-01 21:12, Thomas Petazzoni a écrit :
> In the first case you're using the shell built-in "time" program, in
> the later you're using the separate /usr/bin/time program. Try:

Thank you, I was not aware of that.

> I'm indeed not sure it's really worth the effort for such a small
> saving.

OK, so let's forget it. Moreover, as you have mentioned in your reply to 
the second patch (on documentation), there are some problems with 
$(2)_INSTALL_STAGING/TARGET_OPTS being changed to 
$(2)_INSTALL_STAGING/TARGET_ENV, and also with parallel build support 
being not supported for some packages.

Regards.
Thomas Petazzoni July 11, 2017, 11:56 a.m. UTC | #11
Hello,

Reviving a fairly old thread.

On Fri,  6 Jan 2017 23:37:47 +0100, Cédric Marie wrote:

> Tested on following packages: cannelloni, graphite2, libcuefile,
> libubox, rabbitmq-c, ubus.

I ended up needing this patch to experiment for a project, so I took it
and tested. It generally works fine, but I found one Buildroot CMake
package that doesn't behave properly when the Ninja backend is used:
the "bullet" package.

With the "make" backend, it builds and installs fine, and everything
you expect is install to staging and target.

With the "ninja" backend, it builds and installs fine, but in fact the
only thing it installs to staging and target are two .cmake files. None
of the shared libraries or headers are installed.

The build log with the "make" backend clearly shows that lots of stuff
are being installed: http://code.bulix.org/sx2yb8-160329.

The build log with the "ninja" backend shows just two .cmake files
being installed to staging and target:
http://code.bulix.org/u145dq-160328.

Do you know why a CMake package could be working with the make backend,
but not with the ninja backend ?

Best regards,

Thomas
Cédric Marie July 11, 2017, 1:25 p.m. UTC | #12
Hi,

Le 2017-07-11 13:56, Thomas Petazzoni a écrit :
> Do you know why a CMake package could be working with the make backend,
> but not with the ninja backend ?

In file CMakeLists.txt, in the root directory of the bullet package, I 
have found:

IF("${CMAKE_GENERATOR}" MATCHES "Unix Makefiles")
	OPTION(INSTALL_LIBS "Set when you want to install libraries" ON)
ELSE()
	IF(APPLE AND FRAMEWORK)
		OPTION(INSTALL_LIBS "Set when you want to install libraries" ON)
	ELSE()
#by default, don't enable the 'INSTALL' option for Xcode and MSVC 
projectfiles
		OPTION(INSTALL_LIBS "Set when you want to install libraries" OFF)
	ENDIF()
ENDIF()


I believe that's the reason :)

I suppose the test was expecting to detect the type of machine, while it 
is based on the type of backend...
This should be fixed in "bullet" package, either by testing Ninja 
backend as well, or changing the test (I don't know exactly what he's 
trying to test...)

Regards,
Thomas Petazzoni July 11, 2017, 1:35 p.m. UTC | #13
Hello,

On Tue, 11 Jul 2017 15:25:46 +0200, Cédric Marie wrote:

> Le 2017-07-11 13:56, Thomas Petazzoni a écrit :
> > Do you know why a CMake package could be working with the make backend,
> > but not with the ninja backend ?  
> 
> In file CMakeLists.txt, in the root directory of the bullet package, I 
> have found:
> 
> IF("${CMAKE_GENERATOR}" MATCHES "Unix Makefiles")
> 	OPTION(INSTALL_LIBS "Set when you want to install libraries" ON)
> ELSE()
> 	IF(APPLE AND FRAMEWORK)
> 		OPTION(INSTALL_LIBS "Set when you want to install libraries" ON)
> 	ELSE()
> #by default, don't enable the 'INSTALL' option for Xcode and MSVC 
> projectfiles
> 		OPTION(INSTALL_LIBS "Set when you want to install libraries" OFF)
> 	ENDIF()
> ENDIF()
> 
> 
> I believe that's the reason :)

Indeed! I skimmed through the CMakeLists.txt file, but didn't spot this
obvious thing.

> I suppose the test was expecting to detect the type of machine, while it 
> is based on the type of backend...
> This should be fixed in "bullet" package, either by testing Ninja 
> backend as well, or changing the test (I don't know exactly what he's 
> trying to test...)

I think it's just a bug on their side. I've filed an issue
(https://github.com/bulletphysics/bullet3/issues/1225). Though with 286
open issues, I'm not sure how long it will take for this issue to be
looked at :)

But anyway, it is a very package-specific issue here, unlikely to
affect many other packages. Thanks for having investigated this!

Thomas
diff mbox

Patch

diff --git a/Config.in b/Config.in
index ccd777e..26ebcf6 100644
--- a/Config.in
+++ b/Config.in
@@ -656,6 +656,12 @@  config BR2_SHARED_STATIC_LIBS
 
 endchoice
 
+config BR2_CMAKE_USE_NINJA_BACKEND
+	bool "Compile CMake packages with Ninja backend"
+        help
+          CMake provides several backends. The default one is Make, but
+          Ninja is also supported. Ninja is a small build system with a
+          focus on speed.
 
 config BR2_PACKAGE_OVERRIDE_FILE
 	string "location of a package override file"
diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
index 4e0e838..83bf79e 100644
--- a/package/pkg-cmake.mk
+++ b/package/pkg-cmake.mk
@@ -53,24 +53,35 @@  define inner-cmake-package
 
 $(2)_CONF_ENV			?=
 $(2)_CONF_OPTS			?=
+$(2)_INSTALL_STAGING_ENV	?= DESTDIR=$$(STAGING_DIR)
+$(2)_INSTALL_TARGET_ENV		?= DESTDIR=$$(TARGET_DIR)
+
+ifeq ($(BR2_CMAKE_USE_NINJA_BACKEND),y)
+$(2)_MAKE			?= $$(HOST_DIR)/usr/bin/ninja
+$(2)_MAKE_ENV			?=
+$(2)_MAKE_OPTS			?= $(if $(VERBOSE),-v)
+$(2)_INSTALL_OPTS		?= install
+else
 $(2)_MAKE			?= $$(MAKE)
 $(2)_MAKE_ENV			?=
 $(2)_MAKE_OPTS			?=
-$(2)_INSTALL_OPTS		?= install
-$(2)_INSTALL_STAGING_OPTS	?= DESTDIR=$$(STAGING_DIR) install/fast
-$(2)_INSTALL_TARGET_OPTS		?= DESTDIR=$$(TARGET_DIR) install/fast
+$(2)_INSTALL_OPTS		?= install/fast
+endif
 
 $(2)_SRCDIR			= $$($(2)_DIR)/$$($(2)_SUBDIR)
 
 $(3)_SUPPORTS_IN_SOURCE_BUILD ?= YES
 
-
 ifeq ($$($(3)_SUPPORTS_IN_SOURCE_BUILD),YES)
 $(2)_BUILDDIR			= $$($(2)_SRCDIR)
 else
 $(2)_BUILDDIR			= $$($(2)_SRCDIR)/buildroot-build
 endif
 
+ifeq ($(BR2_CMAKE_USE_NINJA_BACKEND),y)
+$(2)_CONF_OPTS += -G Ninja
+endif
+
 #
 # Configure step. Only define it if not already defined by the package
 # .mk file. And take care of the differences between host and target
@@ -146,6 +157,9 @@  endif
 $(2)_DEPENDENCIES += host-pkgconf
 
 $(2)_DEPENDENCIES += $(BR2_CMAKE_HOST_DEPENDENCY)
+ifeq ($(BR2_CMAKE_USE_NINJA_BACKEND),y)
+$(2)_DEPENDENCIES += host-ninja
+endif
 
 #
 # Build step. Only define it if not already defined by the package .mk
@@ -179,7 +193,7 @@  endif
 #
 ifndef $(2)_INSTALL_STAGING_CMDS
 define $(2)_INSTALL_STAGING_CMDS
-	$$(TARGET_MAKE_ENV) $$($$(PKG)_MAKE_ENV) $$($$(PKG)_MAKE) $$($$(PKG)_MAKE_OPTS) $$($$(PKG)_INSTALL_STAGING_OPTS) -C $$($$(PKG)_BUILDDIR)
+	$$(TARGET_MAKE_ENV) $$($$(PKG)_MAKE_ENV) $$($$(PKG)_INSTALL_STAGING_ENV) $$($$(PKG)_MAKE) $$($$(PKG)_MAKE_OPTS) $$($$(PKG)_INSTALL_OPTS) -C $$($$(PKG)_BUILDDIR)
 endef
 endif
 
@@ -189,7 +203,7 @@  endif
 #
 ifndef $(2)_INSTALL_TARGET_CMDS
 define $(2)_INSTALL_TARGET_CMDS
-	$$(TARGET_MAKE_ENV) $$($$(PKG)_MAKE_ENV) $$($$(PKG)_MAKE) $$($$(PKG)_MAKE_OPTS) $$($$(PKG)_INSTALL_TARGET_OPTS) -C $$($$(PKG)_BUILDDIR)
+	$$(TARGET_MAKE_ENV) $$($$(PKG)_MAKE_ENV) $$($$(PKG)_INSTALL_TARGET_ENV) $$($$(PKG)_MAKE) $$($$(PKG)_MAKE_OPTS) $$($$(PKG)_INSTALL_OPTS) -C $$($$(PKG)_BUILDDIR)
 endef
 endif