diff mbox

Don't build host-cmake if it is available on the build host

Message ID 1454363063-21105-1-git-send-email-luca@lucaceresoli.net
State Changes Requested
Headers show

Commit Message

Luca Ceresoli Feb. 1, 2016, 9:44 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-ccache is not very useful if it is already
installed on the build host: it is supposed to be quite portable, and
the only patch we have for cmake seems to only affect target-cmake.

We avoid building host-cmake if cmake is already available on the host
using a technique similar to the one used for host-tar and host-xzcat.

First, we leverage the existing infrastructure in
support/dependencies/dependencies.mk to set CMAKE to either "cmake" or
"$(HOST_DIR)/usr/bin/cmake". In the latter case we also set
BUILD_HOST_CMAKE = YES meaning we need to build host-cmake.

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

Finally, we do skip adding the dependency on host-cmake for all cmake
packages when $(BUILD_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. Otherwise host-cmake
would be a dependency for _any_ package when it's not installed on the
host, even if no cmake package is selected.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Samuel Martin <s.martin49@gmail.com>
Cc: Davide Viti <zinosat@tiscali.it>
---
 package/pkg-cmake.mk                     |  6 ++++--
 support/dependencies/check-host-cmake.mk |  6 ++++++
 support/dependencies/check-host-cmake.sh | 14 ++++++++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 support/dependencies/check-host-cmake.mk
 create mode 100755 support/dependencies/check-host-cmake.sh

Comments

Arnout Vandecappelle Feb. 2, 2016, 8:44 a.m. UTC | #1
On 01-02-16 22:44, 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-ccache is not very useful if it is already

 host-ccache -> host-cmake

> installed on the build host: it is supposed to be quite portable, and
> the only patch we have for cmake seems to only affect target-cmake.
> 
> We avoid building host-cmake if cmake is already available on the host
> using a technique similar to the one used for host-tar and host-xzcat.
> 
> First, we leverage the existing infrastructure in
> support/dependencies/dependencies.mk to set CMAKE to either "cmake" or
> "$(HOST_DIR)/usr/bin/cmake". In the latter case we also set
> BUILD_HOST_CMAKE = YES meaning we need to build host-cmake.
> 
> Then in pkg-cmake.mk we launch $(CMAKE) instead of
> $(HOST_DIR)/usr/bin/cmake.
> 
> Finally, we do skip adding the dependency on host-cmake for all cmake
> packages when $(BUILD_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. Otherwise host-cmake
> would be a dependency for _any_ package when it's not installed on the
> host, even if no cmake package is selected.

 Nice and long commit message, thanks!

> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Samuel Martin <s.martin49@gmail.com>
> Cc: Davide Viti <zinosat@tiscali.it>
> ---
>  package/pkg-cmake.mk                     |  6 ++++--
>  support/dependencies/check-host-cmake.mk |  6 ++++++
>  support/dependencies/check-host-cmake.sh | 14 ++++++++++++++
>  3 files changed, 24 insertions(+), 2 deletions(-)
>  create mode 100644 support/dependencies/check-host-cmake.mk
>  create mode 100755 support/dependencies/check-host-cmake.sh
> 
> diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
> index 81dcfcc..e3bd603 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) $$(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) \
> +	$$(CMAKE) $$($$(PKG)_SRCDIR) \
>  		-DCMAKE_INSTALL_SO_NO_EXE=0 \
>  		-DCMAKE_FIND_ROOT_PATH="$$(HOST_DIR)" \
>  		-DCMAKE_FIND_ROOT_PATH_MODE_PROGRAM="BOTH" \
> @@ -149,7 +149,9 @@ $(2)_DEPENDENCIES ?= $$(filter-out host-skeleton host-toolchain $(1),\
>  	$$(patsubst host-host-%,host-%,$$(addprefix host-,$$($(3)_DEPENDENCIES))))
>  endif
>  
> +ifeq ($$(BUILD_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..fe16322
> --- /dev/null
> +++ b/support/dependencies/check-host-cmake.mk
> @@ -0,0 +1,6 @@
> +CMAKE ?= cmake
> +
> +ifeq (,$(call suitable-host-package,cmake,$(CMAKE)))
> +BUILD_HOST_CMAKE = YES
> +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..76a81e0
> --- /dev/null
> +++ b/support/dependencies/check-host-cmake.sh
> @@ -0,0 +1,14 @@
> +#!/bin/sh
> +
> +candidate="$1"
> +
> +cmake=`which $candidate`

 $candidate could be an absolute path, in which case which returns nothing.

> +if [ ! -x "$cmake" ]; then
> +	cmake=`which cmake`

 Since there is already CMAKE ?= cmake in the caller, this is pointless.

 How about a

for cmake in "$cmake" "$( which "$cmake" )"; do
 ...

instead?

> +	if [ ! -x "$cmake" ]; then
> +		# echo nothing: no suitable cmake found
> +		exit 1
> +	fi
> +fi

 I have the feeling that there should also be a version check here, though I
don't know which version we should use.

 Regards,
 Arnout

> +
> +echo $cmake
>
Luca Ceresoli Feb. 2, 2016, 9:01 a.m. UTC | #2
Hi Arnout,

thanks for the review.

Unfortunately this patch is not working, at least for qjson with qt4.

qjson fails with:

CMake Error in src/CMakeLists.txt:
   Imported target "Qt4::QtCore" includes non-existent path

     "QT_MKSPECS_DIR-NOTFOUND/default"

   in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include:

   * The path was deleted, renamed, or moved to another location.

   * An install or uninstall procedure did not complete successfully.

   * The installation package was faulty and references files it does not
   provide.

I am investigating right now.

Below my replies to your comments.

Arnout Vandecappelle wrote:
> On 01-02-16 22:44, 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-ccache is not very useful if it is already
>
>   host-ccache -> host-cmake

Oops. :)

>
>> installed on the build host: it is supposed to be quite portable, and
>> the only patch we have for cmake seems to only affect target-cmake.
>>
>> We avoid building host-cmake if cmake is already available on the host
>> using a technique similar to the one used for host-tar and host-xzcat.
>>
>> First, we leverage the existing infrastructure in
>> support/dependencies/dependencies.mk to set CMAKE to either "cmake" or
>> "$(HOST_DIR)/usr/bin/cmake". In the latter case we also set
>> BUILD_HOST_CMAKE = YES meaning we need to build host-cmake.
>>
>> Then in pkg-cmake.mk we launch $(CMAKE) instead of
>> $(HOST_DIR)/usr/bin/cmake.
>>
>> Finally, we do skip adding the dependency on host-cmake for all cmake
>> packages when $(BUILD_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. Otherwise host-cmake
>> would be a dependency for _any_ package when it's not installed on the
>> host, even if no cmake package is selected.
>
>   Nice and long commit message, thanks!
>
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>> Cc: Samuel Martin <s.martin49@gmail.com>
>> Cc: Davide Viti <zinosat@tiscali.it>
>> ---
>>   package/pkg-cmake.mk                     |  6 ++++--
>>   support/dependencies/check-host-cmake.mk |  6 ++++++
>>   support/dependencies/check-host-cmake.sh | 14 ++++++++++++++
>>   3 files changed, 24 insertions(+), 2 deletions(-)
>>   create mode 100644 support/dependencies/check-host-cmake.mk
>>   create mode 100755 support/dependencies/check-host-cmake.sh
>>
>> diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
>> index 81dcfcc..e3bd603 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) $$(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) \
>> +	$$(CMAKE) $$($$(PKG)_SRCDIR) \
>>   		-DCMAKE_INSTALL_SO_NO_EXE=0 \
>>   		-DCMAKE_FIND_ROOT_PATH="$$(HOST_DIR)" \
>>   		-DCMAKE_FIND_ROOT_PATH_MODE_PROGRAM="BOTH" \
>> @@ -149,7 +149,9 @@ $(2)_DEPENDENCIES ?= $$(filter-out host-skeleton host-toolchain $(1),\
>>   	$$(patsubst host-host-%,host-%,$$(addprefix host-,$$($(3)_DEPENDENCIES))))
>>   endif
>>
>> +ifeq ($$(BUILD_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..fe16322
>> --- /dev/null
>> +++ b/support/dependencies/check-host-cmake.mk
>> @@ -0,0 +1,6 @@
>> +CMAKE ?= cmake
>> +
>> +ifeq (,$(call suitable-host-package,cmake,$(CMAKE)))
>> +BUILD_HOST_CMAKE = YES
>> +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..76a81e0
>> --- /dev/null
>> +++ b/support/dependencies/check-host-cmake.sh
>> @@ -0,0 +1,14 @@
>> +#!/bin/sh
>> +
>> +candidate="$1"
>> +
>> +cmake=`which $candidate`
>
>   $candidate could be an absolute path, in which case which returns nothing.
>
>> +if [ ! -x "$cmake" ]; then
>> +	cmake=`which cmake`
>
>   Since there is already CMAKE ?= cmake in the caller, this is pointless.
>
>   How about a
>
> for cmake in "$cmake" "$( which "$cmake" )"; do
>   ...
>
> instead?

Your suggestions are absolutely correct. I shamelessly copied other
check-host-*.sh scripts and just concentrated on having the thing
working. Seems like thos other script need a cleanup now. I'll have a
look (if/after I can make this thing working).

>
>> +	if [ ! -x "$cmake" ]; then
>> +		# echo nothing: no suitable cmake found
>> +		exit 1
>> +	fi
>> +fi
>
>   I have the feeling that there should also be a version check here, though I
> don't know which version we should use.

Right. My idea is to download all the cmake packages that are currently
in Buildroot, find the highest version mentioned in their
CMAKE_MINIMUM_REQUIRED() statement and check for that version in the
script.
Luca Ceresoli Feb. 4, 2016, 1:37 p.m. UTC | #3
Hi Arnout,

Luca Ceresoli wrote:
> Hi Arnout,
>
> thanks for the review.
>
> Unfortunately this patch is not working, at least for qjson with qt4.

It's fixed now (thanks to Samuel Martin for the help). So it's now time
for the refinements.

[...]

>>> new file mode 100644
>>> index 0000000..fe16322
>>> --- /dev/null
>>> +++ b/support/dependencies/check-host-cmake.mk
>>> @@ -0,0 +1,6 @@
>>> +CMAKE ?= cmake
>>> +
>>> +ifeq (,$(call suitable-host-package,cmake,$(CMAKE)))
>>> +BUILD_HOST_CMAKE = YES
>>> +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..76a81e0
>>> --- /dev/null
>>> +++ b/support/dependencies/check-host-cmake.sh
>>> @@ -0,0 +1,14 @@
>>> +#!/bin/sh
>>> +
>>> +candidate="$1"
>>> +
>>> +cmake=`which $candidate`
>>
>>   $candidate could be an absolute path, in which case which returns
>> nothing.

Not sure I got what you mean, sorry. which + absolute path returns the
absolute path itself, if it exists:

$ which cmake
/usr/bin/cmake
$ which /usr/bin/cmake
/usr/bin/cmake
$ which /not/quite/cmake
$

And the which(1) manpage confirms this is correct.

>>> +if [ ! -x "$cmake" ]; then
>>> +    cmake=`which cmake`
>>
>>   Since there is already CMAKE ?= cmake in the caller, this is pointless.

Aah, yes. Removing the second if will still allow to override CMAKE
('make CMAKE=/my/custom/cmake qjson') but if it does not exist it will
not search for one in the path. Sounds good.

>>   How about a
>>
>> for cmake in "$cmake" "$( which "$cmake" )"; do
>>   ...
>>
>> instead?

Given the above discussion about which(1), I think this is useless.

>>> +    if [ ! -x "$cmake" ]; then
>>> +        # echo nothing: no suitable cmake found
>>> +        exit 1
>>> +    fi
>>> +fi
>>
>>   I have the feeling that there should also be a version check here,
>> though I
>> don't know which version we should use.
>
> Right. My idea is to download all the cmake packages that are currently
> in Buildroot, find the highest version mentioned in their
> CMAKE_MINIMUM_REQUIRED() statement and check for that version in the
> script.

I added it, will be in v2. This was absolutely needed to make it work on
a Ubuntu 14.04 LTS host. More details will be in the commit log (which
will be even longer, ooh yeah).
Arnout Vandecappelle Feb. 5, 2016, 9:47 p.m. UTC | #4
On 04-02-16 14:37, Luca Ceresoli wrote:
> Hi Arnout,
> 
> Luca Ceresoli wrote:

>>>> +cmake=`which $candidate`
>>>
>>>   $candidate could be an absolute path, in which case which returns
>>> nothing.
> 
> Not sure I got what you mean, sorry. which + absolute path returns the
> absolute path itself, if it exists:
> 
> $ which cmake
> /usr/bin/cmake
> $ which /usr/bin/cmake
> /usr/bin/cmake
> $ which /not/quite/cmake
> $
> 
> And the which(1) manpage confirms this is correct.
> 


 Oops, my bad, I tested it with a file that wasn't executable...


 Regards,
 Arnout

[snip]
diff mbox

Patch

diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
index 81dcfcc..e3bd603 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) $$(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) \
+	$$(CMAKE) $$($$(PKG)_SRCDIR) \
 		-DCMAKE_INSTALL_SO_NO_EXE=0 \
 		-DCMAKE_FIND_ROOT_PATH="$$(HOST_DIR)" \
 		-DCMAKE_FIND_ROOT_PATH_MODE_PROGRAM="BOTH" \
@@ -149,7 +149,9 @@  $(2)_DEPENDENCIES ?= $$(filter-out host-skeleton host-toolchain $(1),\
 	$$(patsubst host-host-%,host-%,$$(addprefix host-,$$($(3)_DEPENDENCIES))))
 endif
 
+ifeq ($$(BUILD_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..fe16322
--- /dev/null
+++ b/support/dependencies/check-host-cmake.mk
@@ -0,0 +1,6 @@ 
+CMAKE ?= cmake
+
+ifeq (,$(call suitable-host-package,cmake,$(CMAKE)))
+BUILD_HOST_CMAKE = YES
+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..76a81e0
--- /dev/null
+++ b/support/dependencies/check-host-cmake.sh
@@ -0,0 +1,14 @@ 
+#!/bin/sh
+
+candidate="$1"
+
+cmake=`which $candidate`
+if [ ! -x "$cmake" ]; then
+	cmake=`which cmake`
+	if [ ! -x "$cmake" ]; then
+		# echo nothing: no suitable cmake found
+		exit 1
+	fi
+fi
+
+echo $cmake