diff mbox

[3/3] Don't build host-cmake if it is available on the build host

Message ID 1467388410-28135-4-git-send-email-luca@lucaceresoli.net
State Changes Requested
Headers show

Commit Message

Luca Ceresoli July 1, 2016, 3:53 p.m. UTC
Currently all cmake packages depend on host-cmake. Unfortunately
host-cmake takes a long time to configure and build: almost 7 minutes
on a dual-core i5 with SSD. The time does not change even with ccache
enabled.

Indeed, building host-cmake is avoidable if it is already installed on
the build host: CMake is supposed to be quite portable, and the only
patch in Buildroot for the CMake package seems to only affect
target-cmake.

Thus we introduce the possibility to skip entirely building host-cmake
and use the one on the system. However this feature has to be enabled
with a kconfig knob because it introduces problems in some
cases, described below.

In detail, we avoid building host-cmake if:
 - the knob is enabled and
 - cmake is available on the system and
 - it is recent enough.

First, we leverage the existing infrastructure in
support/dependencies/dependencies.mk to find out whether there's a
suitable cmake executable on the system. Its path can be passed in the
BR2_HOST_CMAKE environment variable, otherwise it defaults to
"cmake". If it is enabled, found and suitable then we set
USE_SYSTEM_HOST_CMAKE. Otherwise we override BR2_HOST_CMAKE with
"$(HOST_DIR)/usr/bin/cmake" to revert to the old behaviour.

Then in pkg-cmake.mk we launch $(BR2_HOST_CMAKE) instead of
$(HOST_DIR)/usr/bin/cmake.

Finally, we skip adding the dependency on host-cmake for all cmake
packages when $(USE_SYSTEM_HOST_CMAKE) = YES.

Unlike what we do for host-tar and host-xzcat, for host-cmake we do
not add host-cmake to DEPENDENCIES_HOST_PREREQ. If we did, host-cmake
would be a dependency for _any_ package when it's not installed on the
host, even when no cmake package is selected.

check-host-cmake.sh requires CMake to be at least 3.0 to consider it
suitable. This is because older versions are affected by the bug
described and fixed in Buildroot in ef2c1970e4bf ("cmake: add patch to
fix Qt mkspecs detection"). The bug was fixed in upstream CMake in
version 3.0 [0].

Besides, among all the cmake packages currently in Buildroot, the
highest version mentioned in cmake_minimum_required() is 3.0 (the
grantlee package). Thus 3.0 should be enough to build all current
packages. Of course, with the addition or bump of packages, the
minimum required version will raise.

Tested on:
 - Ubuntu 14.04 without CMake, with official CMake (2.8), PPA CMake
   (3.2)
 - Ubuntu 15.10 without CMake, with official CMake (3.2)
 - Ubuntu 16.04 without CMake, with official CMake (3.5)

[0] https://cmake.org/gitweb?p=cmake.git;h=e8b8b37ef6fef094940d3384df5a1d421b9fa568

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Samuel Martin <s.martin49@gmail.com>
Cc: Davide Viti <zinosat@tiscali.it>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

---

Discussion:

There's an open point about this patch. Currently check-host-cmake.sh
only selects a cmake executable if it is >= 3.0, which is the highest
requirement among all Buildroot packages on current master. In the
future when bumping cmake-packages we should check whether they need a
more recent CMake and bump the required version. Doing that we will
silently disable the speedup for all users who upgrade Buildroot, even
if they do not use the specific package requiring a more recent CMake.

This issue can be handled in several ways:
 1. remove the version check entirely
 2. let the check as it is in this patch
 3. add a "minimum required version" kconfig parameter

Option 1 makes the Buildroot code simpler but breaks builds for people
using one of the mentioned packages. They can address the issue:
 - installing a more recent cmake (maybe in their $HOME, if they
   cannot do any better)
 - disabling BR2_TRY_SYSTEM_CMAKE

Option 2 is similar, except it silently disables the "speedup" feature
for known-too-old versions of cmake. This is bad because it does it
silently as discussed above. On the other hand this option handles the
use case where exactly the same configuration is built on several
machines with different distros, e.g. developer PC and CI server. The
system cmake will be used where suitable, built otherwise.

Option 3 means adding another parameter (and implement a proper
version number comparison). It would also force the user to take care
of setting it properly, which is easy however: if a package fails at
cmake_minimum_required(), raise the parameter. The advantage is that
users not using a specific package can keep the number lower, thus
enabling the speedup on more build hosts.

None of these options is totally satisfying. For this reason I added
the configration knob and kept it off by default. This forces the user
to think before enabling it, and to check whether he has a recent
enough cmake.

Any comments about the three possible alternatives is very welcome.

Results:

On my build server, this patch reduced the build time for a batch of
16 Buildroot configurations (all building cmake-based packages) from
2h44m to 1h54m. Woah, it's a 30% saving!

Changes v2 -> v3:
 - make this feature optional via the BR2_TRY_SYSTEM_CMAKE kconfig
   variable
 - rename the CMAKE variable to BR2_HOST_CMAKE, so it's coherent
   with the naming of other variables overridable by the
   environment, e.g. BR2_DL_DIR
 - invert the logic of the variable triggering the host-cmake
   dependency: USE_SYSTEM_HOST_CMAKE instead of BUILD_HOST_CMAKE;
   needed to implement BR2_TRY_SYSTEM_CMAKE

Changes v1 -> v2:
 - Require cmake >= 3.0. Fixes qjson failure (Luca, Samuel, Thomas)
 - In check-host-cmake.sh only search $1, not "cmake" (Arnout)
 - typo: host-ccache -> host-cmake (Arnout)
---
 Config.in                                | 19 +++++++++++++++++++
 package/pkg-cmake.mk                     |  6 ++++--
 support/dependencies/check-host-cmake.mk | 11 +++++++++++
 support/dependencies/check-host-cmake.sh | 30 ++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 2 deletions(-)
 create mode 100644 support/dependencies/check-host-cmake.mk
 create mode 100755 support/dependencies/check-host-cmake.sh

Comments

Arnout Vandecappelle July 2, 2016, 2:18 p.m. UTC | #1
On 01-07-16 17:53, Luca Ceresoli wrote:
> Currently all cmake packages depend on host-cmake. Unfortunately
> host-cmake takes a long time to configure and build: almost 7 minutes
> on a dual-core i5 with SSD. The time does not change even with ccache
> enabled.
> 
> Indeed, building host-cmake is avoidable if it is already installed on
> the build host: CMake is supposed to be quite portable, and the only
> patch in Buildroot for the CMake package seems to only affect
> target-cmake.
> 
> Thus we introduce the possibility to skip entirely building host-cmake
> and use the one on the system. However this feature has to be enabled
> with a kconfig knob because it introduces problems in some
> cases, described below.

 I don't find the description of those problems...

> 
> In detail, we avoid building host-cmake if:
>  - the knob is enabled and
>  - cmake is available on the system and
>  - it is recent enough.
> 
> First, we leverage the existing infrastructure in
> support/dependencies/dependencies.mk to find out whether there's a
> suitable cmake executable on the system. Its path can be passed in the
> BR2_HOST_CMAKE environment variable, otherwise it defaults to
> "cmake". If it is enabled, found and suitable then we set
> USE_SYSTEM_HOST_CMAKE. Otherwise we override BR2_HOST_CMAKE with
> "$(HOST_DIR)/usr/bin/cmake" to revert to the old behaviour.
> 
> Then in pkg-cmake.mk we launch $(BR2_HOST_CMAKE) instead of
> $(HOST_DIR)/usr/bin/cmake.
> 
> Finally, we skip adding the dependency on host-cmake for all cmake
> packages when $(USE_SYSTEM_HOST_CMAKE) = YES.
> 
> Unlike what we do for host-tar and host-xzcat, for host-cmake we do
> not add host-cmake to DEPENDENCIES_HOST_PREREQ. If we did, host-cmake
> would be a dependency for _any_ package when it's not installed on the
> host, even when no cmake package is selected.
> 
> check-host-cmake.sh requires CMake to be at least 3.0 to consider it
> suitable. This is because older versions are affected by the bug
> described and fixed in Buildroot in ef2c1970e4bf ("cmake: add patch to
> fix Qt mkspecs detection"). The bug was fixed in upstream CMake in
> version 3.0 [0].
> 
> Besides, among all the cmake packages currently in Buildroot, the
> highest version mentioned in cmake_minimum_required() is 3.0 (the
> grantlee package). Thus 3.0 should be enough to build all current
> packages. Of course, with the addition or bump of packages, the
> minimum required version will raise.
> 
> Tested on:
>  - Ubuntu 14.04 without CMake, with official CMake (2.8), PPA CMake
>    (3.2)
>  - Ubuntu 15.10 without CMake, with official CMake (3.2)
>  - Ubuntu 16.04 without CMake, with official CMake (3.5)
> 
> [0] https://cmake.org/gitweb?p=cmake.git;h=e8b8b37ef6fef094940d3384df5a1d421b9fa568
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Samuel Martin <s.martin49@gmail.com>
> Cc: Davide Viti <zinosat@tiscali.it>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> ---
> 
> Discussion:

 IMHO this discussion text should be part of the commit log. We typically look
at the commit log when we wonder why something is done in a particular way, and
this discussion gives exactly that kind of information.

> 
> There's an open point about this patch. Currently check-host-cmake.sh
> only selects a cmake executable if it is >= 3.0, which is the highest
> requirement among all Buildroot packages on current master. In the
> future when bumping cmake-packages we should check whether they need a
> more recent CMake and bump the required version. Doing that we will
> silently disable the speedup for all users who upgrade Buildroot, even
> if they do not use the specific package requiring a more recent CMake.
> 
> This issue can be handled in several ways:
>  1. remove the version check entirely
>  2. let the check as it is in this patch
>  3. add a "minimum required version" kconfig parameter
> 
> Option 1 makes the Buildroot code simpler but breaks builds for people
> using one of the mentioned packages. They can address the issue:
>  - installing a more recent cmake (maybe in their $HOME, if they
>    cannot do any better)
>  - disabling BR2_TRY_SYSTEM_CMAKE

 Ah, this is the problem you are talking about? So basically,
BR2_TRY_SYSTEM_CMAKE is only relevant if the version check is removed?

> 
> Option 2 is similar, except it silently disables the "speedup" feature
> for known-too-old versions of cmake. This is bad because it does it
> silently as discussed above. On the other hand this option handles the
> use case where exactly the same configuration is built on several
> machines with different distros, e.g. developer PC and CI server. The
> system cmake will be used where suitable, built otherwise.
> 
> Option 3 means adding another parameter (and implement a proper
> version number comparison). It would also force the user to take care
> of setting it properly, which is easy however: if a package fails at
> cmake_minimum_required(), raise the parameter. The advantage is that
> users not using a specific package can keep the number lower, thus
> enabling the speedup on more build hosts.

 There could also be a BR2_PACKAGE_HOST_CMAKE_MIN_VERSION_X_Y that could be
selected by individual packages, and used in the check-host-cmake script to
evaluate the installed cmake version. However, it is not entirely trivial to
check the correctness of this in the autobuilders.

 For me, option 2 is sufficient. There may indeed be a build speed regression
when bumping buildroot with the same config. But since bumping buildroot also
means that package versions have been bumped, this could also be due to the new
package version requiring a more recent cmake. In addition, a build speed
regression I don't seriously consider a regression. Actually, for me, almost
every buildroot commit is a speed regression because bash-completion becomes
slower with each additional package :-)

 So, in my opinion: ditch BR2_TRY_SYSTEM_CMAKE.

> 
> None of these options is totally satisfying. For this reason I added
> the configration knob and kept it off by default. This forces the user
> to think before enabling it, and to check whether he has a recent
> enough cmake.
> 
> Any comments about the three possible alternatives is very welcome.
[snip]
> diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
> index 8d7d265..a96b9cd 100644
> --- a/package/pkg-cmake.mk
> +++ b/package/pkg-cmake.mk
> @@ -85,7 +85,7 @@ define $(2)_CONFIGURE_CMDS
>  	cd $$($$(PKG)_BUILDDIR) && \
>  	rm -f CMakeCache.txt && \
>  	PATH=$$(BR_PATH) \
> -	$$($$(PKG)_CONF_ENV) $$(HOST_DIR)/usr/bin/cmake $$($$(PKG)_SRCDIR) \
> +	$$($$(PKG)_CONF_ENV) $$(BR2_HOST_CMAKE) $$($$(PKG)_SRCDIR) \
>  		-DCMAKE_TOOLCHAIN_FILE="$$(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake" \
>  		-DCMAKE_BUILD_TYPE=$$(if $$(BR2_ENABLE_DEBUG),Debug,Release) \
>  		-DCMAKE_INSTALL_PREFIX="/usr" \
> @@ -110,7 +110,7 @@ define $(2)_CONFIGURE_CMDS
>  	cd $$($$(PKG)_BUILDDIR) && \
>  	rm -f CMakeCache.txt && \
>  	PATH=$$(BR_PATH) \
> -	$$(HOST_DIR)/usr/bin/cmake $$($$(PKG)_SRCDIR) \
> +	$$(BR2_HOST_CMAKE) $$($$(PKG)_SRCDIR) \
>  		-DCMAKE_INSTALL_SO_NO_EXE=0 \
>  		-DCMAKE_FIND_ROOT_PATH="$$(HOST_DIR)" \
>  		-DCMAKE_FIND_ROOT_PATH_MODE_PROGRAM="BOTH" \
> @@ -153,7 +153,9 @@ endif
>  # primitives to find {C,LD}FLAGS, add it to the dependency list.
>  $(2)_DEPENDENCIES += host-pkgconf
>  
> +ifneq ($$(USE_SYSTEM_HOST_CMAKE),YES)

 We typically talk about host-foo and system-foo, so this should be
USE_SYSTEM_CMAKE.

>  $(2)_DEPENDENCIES += host-cmake
> +endif
>  
>  #
>  # Build step. Only define it if not already defined by the package .mk
> diff --git a/support/dependencies/check-host-cmake.mk b/support/dependencies/check-host-cmake.mk
> new file mode 100644
> index 0000000..20ef81f
> --- /dev/null
> +++ b/support/dependencies/check-host-cmake.mk
> @@ -0,0 +1,11 @@
> +BR2_HOST_CMAKE ?= cmake
> +
> +ifeq ($(BR2_TRY_SYSTEM_CMAKE),y)
> +ifneq (,$(call suitable-host-package,cmake,$(BR2_HOST_CMAKE)))
> +USE_SYSTEM_HOST_CMAKE = YES
> +endif
> +endif
> +
> +ifneq ($(USE_SYSTEM_HOST_CMAKE),YES)

 If BR2_TRY_SYSTEM_CMAKE is ditched, this can just be the else branch.


 Regards,
 Arnout

> +BR2_HOST_CMAKE = $(HOST_DIR)/usr/bin/cmake
> +endif
> diff --git a/support/dependencies/check-host-cmake.sh b/support/dependencies/check-host-cmake.sh
> new file mode 100755
> index 0000000..08de60c
> --- /dev/null
> +++ b/support/dependencies/check-host-cmake.sh
> @@ -0,0 +1,30 @@
> +#!/bin/sh
> +
> +candidate="$1"
> +
> +cmake=`which $candidate`
> +if [ ! -x "$cmake" ]; then
> +	# echo nothing: no suitable cmake found
> +	exit 1
> +fi
> +
> +version=`$cmake --version | head -n1 | cut -d\  -f3`
> +major=`echo "$version" | cut -d. -f1`
> +minor=`echo "$version" | cut -d. -f2`
> +
> +# Versions before 3.0 are affected by the bug described in
> +# https://git.busybox.net/buildroot/commit/?id=ef2c1970e4bff3be3992014070392b0e6bc28bd2
> +# and fixed in upstream CMake in version 3.0:
> +# https://cmake.org/gitweb?p=cmake.git;h=e8b8b37ef6fef094940d3384df5a1d421b9fa568
> +major_min=3
> +minor_min=0
> +if [ $major -gt $major_min ]; then
> +	echo $cmake
> +else
> +	if [ $major -eq $major_min -a $minor -ge $minor_min ]; then
> +		echo $cmake
> +	else
> +		# echo nothing: no suitable cmake found
> +		exit 1
> +	fi
> +fi
>
Luca Ceresoli July 4, 2016, 10:36 a.m. UTC | #2
Arnout,

thanks for your comments! Se my replies below.

On 02/07/2016 16:18, Arnout Vandecappelle wrote:
> On 01-07-16 17:53, Luca Ceresoli wrote:
[...]
>> Discussion:
> 
>  IMHO this discussion text should be part of the commit log. We typically look
> at the commit log when we wonder why something is done in a particular way, and
> this discussion gives exactly that kind of information.
> 
>>
>> There's an open point about this patch. Currently check-host-cmake.sh
>> only selects a cmake executable if it is >= 3.0, which is the highest
>> requirement among all Buildroot packages on current master. In the
>> future when bumping cmake-packages we should check whether they need a
>> more recent CMake and bump the required version. Doing that we will
>> silently disable the speedup for all users who upgrade Buildroot, even
>> if they do not use the specific package requiring a more recent CMake.
>>
>> This issue can be handled in several ways:
>>  1. remove the version check entirely
>>  2. let the check as it is in this patch
>>  3. add a "minimum required version" kconfig parameter
>>
>> Option 1 makes the Buildroot code simpler but breaks builds for people
>> using one of the mentioned packages. They can address the issue:
>>  - installing a more recent cmake (maybe in their $HOME, if they
>>    cannot do any better)
>>  - disabling BR2_TRY_SYSTEM_CMAKE
> 
>  Ah, this is the problem you are talking about? So basically,
> BR2_TRY_SYSTEM_CMAKE is only relevant if the version check is removed?

Technically you're right. But for a goor "user experience", I'd like
BR2_TRY_SYSTEM_CMAKE to be there at least to hold docs about this
feature. In case 2 the user should be aware that the speedup might not
exist and might disappear bumping Buildroot, why, and how to re-enable it.

I also would give the users the option to opt out, in case there's any
suspect problems with the system cmake.

>> Option 2 is similar, except it silently disables the "speedup" feature
>> for known-too-old versions of cmake. This is bad because it does it
>> silently as discussed above. On the other hand this option handles the
>> use case where exactly the same configuration is built on several
>> machines with different distros, e.g. developer PC and CI server. The
>> system cmake will be used where suitable, built otherwise.
>>
>> Option 3 means adding another parameter (and implement a proper
>> version number comparison). It would also force the user to take care
>> of setting it properly, which is easy however: if a package fails at
>> cmake_minimum_required(), raise the parameter. The advantage is that
>> users not using a specific package can keep the number lower, thus
>> enabling the speedup on more build hosts.
> 
>  There could also be a BR2_PACKAGE_HOST_CMAKE_MIN_VERSION_X_Y that could be
> selected by individual packages, and used in the check-host-cmake script to
> evaluate the installed cmake version. However, it is not entirely trivial to
> check the correctness of this in the autobuilders.

This would be the most complete solution. However, no matter how complex
it is to implement and test, it still adds a burden on the shoulders of
contributors wanting to "simply" bump a package version. I'm trying to
avoid that.

>  For me, option 2 is sufficient. There may indeed be a build speed regression
> when bumping buildroot with the same config. But since bumping buildroot also
> means that package versions have been bumped, this could also be due to the new
> package version requiring a more recent cmake. In addition, a build speed
> regression I don't seriously consider a regression. Actually, for me, almost
> every buildroot commit is a speed regression because bash-completion becomes
> slower with each additional package :-)

Thanks for your vote! :)

I just want to make sure you understand this can disable the speedup
_without_ any real need. Take this use case:

- in BR 2016.08 we bump package foo to 9.9, which needs CMake 4.0
- the user bumps to BR 2016.08 when it's released
- the user uses Ubuntu 16.04 LTS on his PC, which ships CMake 3.5
- the user does not use package foo

This user's builds will fail in finding a suitable system-cmake and will
build host-cmake, which is not strictly needed because all enabled
packages are happy with CMake <= 3.5.

I'm trying to support users not willing do modify the Buildroot code
(e.g. using only BR2_EXTERNAL to add their own salt). With the current
patch, these users could not touch the minimum required version. They
would have no way to re-enable the speedup from Buildroot, they'd need
to act on the "environment", by installing a newer CMake.

[...]
>> +ifneq ($$(USE_SYSTEM_HOST_CMAKE),YES)
> 
>  We typically talk about host-foo and system-foo, so this should be
> USE_SYSTEM_CMAKE.

Right, will fix!
Arnout Vandecappelle July 5, 2016, 9:21 a.m. UTC | #3
On 04-07-16 12:36, Luca Ceresoli wrote:
> Arnout,
> 
> thanks for your comments! Se my replies below.
> 
> On 02/07/2016 16:18, Arnout Vandecappelle wrote:
>> On 01-07-16 17:53, Luca Ceresoli wrote:
[snip]
>>> Option 3 means adding another parameter (and implement a proper
>>> version number comparison). It would also force the user to take care
>>> of setting it properly, which is easy however: if a package fails at
>>> cmake_minimum_required(), raise the parameter. The advantage is that
>>> users not using a specific package can keep the number lower, thus
>>> enabling the speedup on more build hosts.
>>
>>  There could also be a BR2_PACKAGE_HOST_CMAKE_MIN_VERSION_X_Y that could be
>> selected by individual packages, and used in the check-host-cmake script to
>> evaluate the installed cmake version. However, it is not entirely trivial to
>> check the correctness of this in the autobuilders.
> 
> This would be the most complete solution. However, no matter how complex
> it is to implement and test, it still adds a burden on the shoulders of
> contributors wanting to "simply" bump a package version. I'm trying to
> avoid that.

 I didn't make this clear enough: I absolutely don't like the idea of using
BR2_PACKAGE_HOST_CMAKE_MIN_VERSION_X_Y, because it adds a lot of complexity and
maintainance burden with limited gains.

> 
>>  For me, option 2 is sufficient. There may indeed be a build speed regression
>> when bumping buildroot with the same config. But since bumping buildroot also
>> means that package versions have been bumped, this could also be due to the new
>> package version requiring a more recent cmake. In addition, a build speed
>> regression I don't seriously consider a regression. Actually, for me, almost
>> every buildroot commit is a speed regression because bash-completion becomes
>> slower with each additional package :-)
> 
> Thanks for your vote! :)
> 
> I just want to make sure you understand this can disable the speedup
> _without_ any real need. Take this use case:
> 
> - in BR 2016.08 we bump package foo to 9.9, which needs CMake 4.0
> - the user bumps to BR 2016.08 when it's released
> - the user uses Ubuntu 16.04 LTS on his PC, which ships CMake 3.5
> - the user does not use package foo
> 
> This user's builds will fail in finding a suitable system-cmake and will
> build host-cmake, which is not strictly needed because all enabled
> packages are happy with CMake <= 3.5.

 Yes, I understand that this is possible. And my response is: I don't care. We
did the same (though with lower impact) when we started requiring tar >= 1.17. I
don't think a simple speedup is important enough to warrant additional
user-visible complexity. Given that it is just a speed-up, I don't think
complicated help text is needed either.

 So I really would like to remove the BR2_TRY_SYSTEM_CMAKE option. First of all,
it obviously doesn't help for the speed regression issue: if you set that to y,
you will still see the speed regression. Supposedly you would have read the help
text and know that this is possible, but typically a buildroot bump will happen
months or years after the configuration was first done, so in all likelihood the
user has forgotten all about it.

 The only real reason to keep the option is to work around a problem where the
system cmake seems to be OK but isn't. Since we have various cmake versions in
the autobuilders, the risk should be limited there. And if a user really has a
broken system cmake, you can still set BR2_HOST_CMAKE=missing to override it. It
does make sense to put that in the troubleshooting section of the manual
(together with xzcat and tar).

 Regards,
 Arnout


> I'm trying to support users not willing do modify the Buildroot code
> (e.g. using only BR2_EXTERNAL to add their own salt). With the current
> patch, these users could not touch the minimum required version. They
> would have no way to re-enable the speedup from Buildroot, they'd need
> to act on the "environment", by installing a newer CMake.
> 
> [...]
>>> +ifneq ($$(USE_SYSTEM_HOST_CMAKE),YES)
>>
>>  We typically talk about host-foo and system-foo, so this should be
>> USE_SYSTEM_CMAKE.
> 
> Right, will fix!
>
Luca Ceresoli July 16, 2016, 8:32 p.m. UTC | #4
Dear Arnout,

On 05/07/2016 11:21, Arnout Vandecappelle wrote:
[snip]
>>>> Option 3 means adding another parameter (and implement a proper
>>>> version number comparison). It would also force the user to take care
>>>> of setting it properly, which is easy however: if a package fails at
>>>> cmake_minimum_required(), raise the parameter. The advantage is that
>>>> users not using a specific package can keep the number lower, thus
>>>> enabling the speedup on more build hosts.
>>>
>>>  There could also be a BR2_PACKAGE_HOST_CMAKE_MIN_VERSION_X_Y that could be
>>> selected by individual packages, and used in the check-host-cmake script to
>>> evaluate the installed cmake version. However, it is not entirely trivial to
>>> check the correctness of this in the autobuilders.
>>
>> This would be the most complete solution. However, no matter how complex
>> it is to implement and test, it still adds a burden on the shoulders of
>> contributors wanting to "simply" bump a package version. I'm trying to
>> avoid that.
> 
>  I didn't make this clear enough: I absolutely don't like the idea of using
> BR2_PACKAGE_HOST_CMAKE_MIN_VERSION_X_Y, because it adds a lot of complexity and
> maintainance burden with limited gains.

I couldn't agree more.

>>>  For me, option 2 is sufficient. There may indeed be a build speed regression
>>> when bumping buildroot with the same config. But since bumping buildroot also
>>> means that package versions have been bumped, this could also be due to the new
>>> package version requiring a more recent cmake. In addition, a build speed
>>> regression I don't seriously consider a regression. Actually, for me, almost
>>> every buildroot commit is a speed regression because bash-completion becomes
>>> slower with each additional package :-)
>>
>> Thanks for your vote! :)

Guess what, you won the poll! v4 is coming with option 2 implemented.
Which means it will be equivalent to v2, except for cleanups.

>> I just want to make sure you understand this can disable the speedup
>> _without_ any real need. Take this use case:
>>
>> - in BR 2016.08 we bump package foo to 9.9, which needs CMake 4.0
>> - the user bumps to BR 2016.08 when it's released
>> - the user uses Ubuntu 16.04 LTS on his PC, which ships CMake 3.5
>> - the user does not use package foo
>>
>> This user's builds will fail in finding a suitable system-cmake and will
>> build host-cmake, which is not strictly needed because all enabled
>> packages are happy with CMake <= 3.5.
> 
>  Yes, I understand that this is possible. And my response is: I don't care. We
> did the same (though with lower impact) when we started requiring tar >= 1.17. I
> don't think a simple speedup is important enough to warrant additional
> user-visible complexity. Given that it is just a speed-up, I don't think
> complicated help text is needed either.
> 
>  So I really would like to remove the BR2_TRY_SYSTEM_CMAKE option. First of all,
> it obviously doesn't help for the speed regression issue: if you set that to y,
> you will still see the speed regression. Supposedly you would have read the help
> text and know that this is possible, but typically a buildroot bump will happen
> months or years after the configuration was first done, so in all likelihood the
> user has forgotten all about it.

Although I don't 100% agree with you on the theoretical side, I have
come to think your approach is the best to all practical effects. And it
looks like most packages are fine with a relatively old version of
CMake, which makes my concerns much less relevant. Finally, we can add
more complexity later, should we realize it's needed instead.
diff mbox

Patch

diff --git a/Config.in b/Config.in
index 820b1f0..58eaa79 100644
--- a/Config.in
+++ b/Config.in
@@ -357,6 +357,25 @@  config BR2_CCACHE_USE_BASEDIR
 
 endif
 
+config BR2_TRY_SYSTEM_CMAKE
+	bool "Try to use a CMake installed on the system"
+	help
+	  By default Buildroot builds CMake to run on the host
+	  (host-cmake) when any cmake-package is enabled. However
+	  building host-cmake takes a long time. If you have a
+	  suitable cmake on your build host you can use it by enabling
+	  this options: Buildroot will skip building host-cmake and
+	  use the system-provided one.
+
+	  By default Buildroot looks for an executable named "cmake"
+	  in your $PATH. You can specify another one by setting its
+	  path in the BR2_HOST_CMAKE environment variable.
+
+	  Note the system-provided CMake must be recent enough to
+	  support all enabled packages. Make sure it is no lower than
+	  the value passed to cmake_minimum_required() in the
+	  cmake-packages you use.
+
 config BR2_DEPRECATED
 	bool "Show options and packages that are deprecated or obsolete"
 	help
diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
index 8d7d265..a96b9cd 100644
--- a/package/pkg-cmake.mk
+++ b/package/pkg-cmake.mk
@@ -85,7 +85,7 @@  define $(2)_CONFIGURE_CMDS
 	cd $$($$(PKG)_BUILDDIR) && \
 	rm -f CMakeCache.txt && \
 	PATH=$$(BR_PATH) \
-	$$($$(PKG)_CONF_ENV) $$(HOST_DIR)/usr/bin/cmake $$($$(PKG)_SRCDIR) \
+	$$($$(PKG)_CONF_ENV) $$(BR2_HOST_CMAKE) $$($$(PKG)_SRCDIR) \
 		-DCMAKE_TOOLCHAIN_FILE="$$(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake" \
 		-DCMAKE_BUILD_TYPE=$$(if $$(BR2_ENABLE_DEBUG),Debug,Release) \
 		-DCMAKE_INSTALL_PREFIX="/usr" \
@@ -110,7 +110,7 @@  define $(2)_CONFIGURE_CMDS
 	cd $$($$(PKG)_BUILDDIR) && \
 	rm -f CMakeCache.txt && \
 	PATH=$$(BR_PATH) \
-	$$(HOST_DIR)/usr/bin/cmake $$($$(PKG)_SRCDIR) \
+	$$(BR2_HOST_CMAKE) $$($$(PKG)_SRCDIR) \
 		-DCMAKE_INSTALL_SO_NO_EXE=0 \
 		-DCMAKE_FIND_ROOT_PATH="$$(HOST_DIR)" \
 		-DCMAKE_FIND_ROOT_PATH_MODE_PROGRAM="BOTH" \
@@ -153,7 +153,9 @@  endif
 # primitives to find {C,LD}FLAGS, add it to the dependency list.
 $(2)_DEPENDENCIES += host-pkgconf
 
+ifneq ($$(USE_SYSTEM_HOST_CMAKE),YES)
 $(2)_DEPENDENCIES += host-cmake
+endif
 
 #
 # Build step. Only define it if not already defined by the package .mk
diff --git a/support/dependencies/check-host-cmake.mk b/support/dependencies/check-host-cmake.mk
new file mode 100644
index 0000000..20ef81f
--- /dev/null
+++ b/support/dependencies/check-host-cmake.mk
@@ -0,0 +1,11 @@ 
+BR2_HOST_CMAKE ?= cmake
+
+ifeq ($(BR2_TRY_SYSTEM_CMAKE),y)
+ifneq (,$(call suitable-host-package,cmake,$(BR2_HOST_CMAKE)))
+USE_SYSTEM_HOST_CMAKE = YES
+endif
+endif
+
+ifneq ($(USE_SYSTEM_HOST_CMAKE),YES)
+BR2_HOST_CMAKE = $(HOST_DIR)/usr/bin/cmake
+endif
diff --git a/support/dependencies/check-host-cmake.sh b/support/dependencies/check-host-cmake.sh
new file mode 100755
index 0000000..08de60c
--- /dev/null
+++ b/support/dependencies/check-host-cmake.sh
@@ -0,0 +1,30 @@ 
+#!/bin/sh
+
+candidate="$1"
+
+cmake=`which $candidate`
+if [ ! -x "$cmake" ]; then
+	# echo nothing: no suitable cmake found
+	exit 1
+fi
+
+version=`$cmake --version | head -n1 | cut -d\  -f3`
+major=`echo "$version" | cut -d. -f1`
+minor=`echo "$version" | cut -d. -f2`
+
+# Versions before 3.0 are affected by the bug described in
+# https://git.busybox.net/buildroot/commit/?id=ef2c1970e4bff3be3992014070392b0e6bc28bd2
+# and fixed in upstream CMake in version 3.0:
+# https://cmake.org/gitweb?p=cmake.git;h=e8b8b37ef6fef094940d3384df5a1d421b9fa568
+major_min=3
+minor_min=0
+if [ $major -gt $major_min ]; then
+	echo $cmake
+else
+	if [ $major -eq $major_min -a $minor -ge $minor_min ]; then
+		echo $cmake
+	else
+		# echo nothing: no suitable cmake found
+		exit 1
+	fi
+fi