diff mbox series

[RFCv3,04/15] pkg-cmake: install CMake files as part of a package

Message ID 20171201205352.24287-5-thomas.petazzoni@free-electrons.com
State Superseded
Headers show
Series Per-package host/target directory support | expand

Commit Message

Thomas Petazzoni Dec. 1, 2017, 8:53 p.m. UTC
Currently, the toolchainfile.cmake and Buildroot.cmake files are
installed outside of any package, just triggered by the toolchain
target.

As part of the per-package SDK effort, we are trying to avoid anything
that installs to the global $(HOST_DIR), and this is one of the
remaining files installed in $(HOST_DIR) outside of any package. We
fix this by installing such files as part of the toolchain package
post-install staging hooks.

Yes, a post-install staging hook to install things to $(HOST_DIR) is a
bit weird, but the toolchain infrastructure is made of target packages
only, and they all install a lot of stuff to $(HOST_DIR) already.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
Changes since v2:
 - None
Changes since v1:
 - New patch
---
 package/pkg-cmake.mk             | 12 +++++++-----
 toolchain/toolchain/toolchain.mk |  3 ---
 2 files changed, 7 insertions(+), 8 deletions(-)

Comments

Yann E. MORIN Dec. 3, 2017, 10:34 p.m. UTC | #1
Thomas, All,

On 2017-12-01 21:53 +0100, Thomas Petazzoni spake thusly:
> Currently, the toolchainfile.cmake and Buildroot.cmake files are
> installed outside of any package, just triggered by the toolchain
> target.
> 
> As part of the per-package SDK effort, we are trying to avoid anything
> that installs to the global $(HOST_DIR), and this is one of the
> remaining files installed in $(HOST_DIR) outside of any package. We
> fix this by installing such files as part of the toolchain package
> post-install staging hooks.
> 
> Yes, a post-install staging hook to install things to $(HOST_DIR) is a
> bit weird, but the toolchain infrastructure is made of target packages
> only, and they all install a lot of stuff to $(HOST_DIR) already.

What I am more concerned about, is that the cmake package is registering
a hook for another package, toolchain.

Have you tried to run check-package on that?

Regards,
Yann E. MORIN.

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> Changes since v2:
>  - None
> Changes since v1:
>  - New patch
> ---
>  package/pkg-cmake.mk             | 12 +++++++-----
>  toolchain/toolchain/toolchain.mk |  3 ---
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
> index 6739704e3c..6d4f948d07 100644
> --- a/package/pkg-cmake.mk
> +++ b/package/pkg-cmake.mk
> @@ -244,8 +244,8 @@ endif
>  # based on the toolchainfile.cmake file's location: $(HOST_DIR)/share/buildroot
>  # In all the other variables, HOST_DIR will be replaced by RELOCATED_HOST_DIR,
>  # so we have to strip "$(HOST_DIR)/" from the paths that contain it.
> -$(HOST_DIR)/share/buildroot/toolchainfile.cmake:
> -	@mkdir -p $(@D)
> +define TOOLCHAIN_CMAKE_INSTALL_FILES
> +	@mkdir -p $(HOST_DIR)/share/buildroot
>  	sed \
>  		-e 's#@@STAGING_SUBDIR@@#$(call qstrip,$(STAGING_SUBDIR))#' \
>  		-e 's#@@TARGET_CFLAGS@@#$(call qstrip,$(TARGET_CFLAGS))#' \
> @@ -259,7 +259,9 @@ $(HOST_DIR)/share/buildroot/toolchainfile.cmake:
>  		-e 's#@@TOOLCHAIN_HAS_FORTRAN@@#$(if $(BR2_TOOLCHAIN_HAS_FORTRAN),1,0)#' \
>  		-e 's#@@CMAKE_BUILD_TYPE@@#$(if $(BR2_ENABLE_DEBUG),Debug,Release)#' \
>  		$(TOPDIR)/support/misc/toolchainfile.cmake.in \
> -		> $@
> +		> $(HOST_DIR)/share/buildroot/toolchainfile.cmake
> +	$(Q)$(INSTALL) -D -m 0644 support/misc/Buildroot.cmake \
> +		$(HOST_DIR)/share/buildroot/Platform/Buildroot.cmake
> +endef
>  
> -$(HOST_DIR)/share/buildroot/Platform/Buildroot.cmake:
> -	$(Q)$(INSTALL) -D -m 0644 support/misc/Buildroot.cmake $(@)
> +TOOLCHAIN_POST_INSTALL_STAGING_HOOKS += TOOLCHAIN_CMAKE_INSTALL_FILES
> diff --git a/toolchain/toolchain/toolchain.mk b/toolchain/toolchain/toolchain.mk
> index b55b0c712c..283e0a74ee 100644
> --- a/toolchain/toolchain/toolchain.mk
> +++ b/toolchain/toolchain/toolchain.mk
> @@ -38,6 +38,3 @@ TOOLCHAIN_INSTALL_STAGING = YES
>  endif
>  
>  $(eval $(virtual-package))
> -
> -toolchain: $(HOST_DIR)/share/buildroot/toolchainfile.cmake
> -toolchain: $(HOST_DIR)/share/buildroot/Platform/Buildroot.cmake
> -- 
> 2.13.6
>
Thomas Petazzoni Dec. 3, 2017, 10:55 p.m. UTC | #2
Hello,

On Sun, 3 Dec 2017 23:34:54 +0100, Yann E. MORIN wrote:

> On 2017-12-01 21:53 +0100, Thomas Petazzoni spake thusly:
> > Currently, the toolchainfile.cmake and Buildroot.cmake files are
> > installed outside of any package, just triggered by the toolchain
> > target.
> > 
> > As part of the per-package SDK effort, we are trying to avoid anything
> > that installs to the global $(HOST_DIR), and this is one of the
> > remaining files installed in $(HOST_DIR) outside of any package. We
> > fix this by installing such files as part of the toolchain package
> > post-install staging hooks.
> > 
> > Yes, a post-install staging hook to install things to $(HOST_DIR) is a
> > bit weird, but the toolchain infrastructure is made of target packages
> > only, and they all install a lot of stuff to $(HOST_DIR) already.  
> 
> What I am more concerned about, is that the cmake package is registering
> a hook for another package, toolchain.

Do we want to introduce a toolchain-cmake package, that all CMake
packages would depend on, just for the sake of installing those files ?

If we would do this, then a normal toolchain build (i.e without
building any CMake package) would no longer generate the
toolchainfile.cmake file, which is quite useful for SDKs. People could
indeed enable that package explicitly when they are building a SDK.

But all in all, I'm not sure it's really worth the hassle. But I'm open
to comments on that.

Or perhaps you had other suggestions in mind?

> Have you tried to run check-package on that?

Nope, I did not, but it does not complain:

$ ./utils/check-package package/pkg-cmake.mk 
0 lines processed
0 warnings generated

Thanks for the review!

Thomas
Yann E. MORIN Dec. 29, 2017, 3:27 p.m. UTC | #3
Thomas, All,

On 2017-12-03 23:55 +0100, Thomas Petazzoni spake thusly:
> On Sun, 3 Dec 2017 23:34:54 +0100, Yann E. MORIN wrote:
> > On 2017-12-01 21:53 +0100, Thomas Petazzoni spake thusly:
> > > Currently, the toolchainfile.cmake and Buildroot.cmake files are
> > > installed outside of any package, just triggered by the toolchain
> > > target.
> > > 
> > > As part of the per-package SDK effort, we are trying to avoid anything
> > > that installs to the global $(HOST_DIR), and this is one of the
> > > remaining files installed in $(HOST_DIR) outside of any package. We
> > > fix this by installing such files as part of the toolchain package
> > > post-install staging hooks.
> > > 
> > > Yes, a post-install staging hook to install things to $(HOST_DIR) is a
> > > bit weird, but the toolchain infrastructure is made of target packages
> > > only, and they all install a lot of stuff to $(HOST_DIR) already.  
> > 
> > What I am more concerned about, is that the cmake package is registering
> > a hook for another package, toolchain.
> 
> Do we want to introduce a toolchain-cmake package, that all CMake
> packages would depend on, just for the sake of installing those files ?
> 
> If we would do this, then a normal toolchain build (i.e without
> building any CMake package) would no longer generate the
> toolchainfile.cmake file, which is quite useful for SDKs. People could
> indeed enable that package explicitly when they are building a SDK.
> 
> But all in all, I'm not sure it's really worth the hassle. But I'm open
> to comments on that.
> 
> Or perhaps you had other suggestions in mind?

No, I don't have a better solution. And in fact, it is not a package
that is registering a hook for another one; it is the cmake infra that
registers a hook for the toolchain package.

And I'm pretty fine with this, in fact.

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

> > Have you tried to run check-package on that?
> 
> Nope, I did not, but it does not complain:
> 
> $ ./utils/check-package package/pkg-cmake.mk 
> 0 lines processed
> 0 warnings generated

Right, because this is not a package, but a package infra.

Thanks! :-)

Regards,
Yann E. MORIN.

> Thanks for the review!
> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
diff mbox series

Patch

diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
index 6739704e3c..6d4f948d07 100644
--- a/package/pkg-cmake.mk
+++ b/package/pkg-cmake.mk
@@ -244,8 +244,8 @@  endif
 # based on the toolchainfile.cmake file's location: $(HOST_DIR)/share/buildroot
 # In all the other variables, HOST_DIR will be replaced by RELOCATED_HOST_DIR,
 # so we have to strip "$(HOST_DIR)/" from the paths that contain it.
-$(HOST_DIR)/share/buildroot/toolchainfile.cmake:
-	@mkdir -p $(@D)
+define TOOLCHAIN_CMAKE_INSTALL_FILES
+	@mkdir -p $(HOST_DIR)/share/buildroot
 	sed \
 		-e 's#@@STAGING_SUBDIR@@#$(call qstrip,$(STAGING_SUBDIR))#' \
 		-e 's#@@TARGET_CFLAGS@@#$(call qstrip,$(TARGET_CFLAGS))#' \
@@ -259,7 +259,9 @@  $(HOST_DIR)/share/buildroot/toolchainfile.cmake:
 		-e 's#@@TOOLCHAIN_HAS_FORTRAN@@#$(if $(BR2_TOOLCHAIN_HAS_FORTRAN),1,0)#' \
 		-e 's#@@CMAKE_BUILD_TYPE@@#$(if $(BR2_ENABLE_DEBUG),Debug,Release)#' \
 		$(TOPDIR)/support/misc/toolchainfile.cmake.in \
-		> $@
+		> $(HOST_DIR)/share/buildroot/toolchainfile.cmake
+	$(Q)$(INSTALL) -D -m 0644 support/misc/Buildroot.cmake \
+		$(HOST_DIR)/share/buildroot/Platform/Buildroot.cmake
+endef
 
-$(HOST_DIR)/share/buildroot/Platform/Buildroot.cmake:
-	$(Q)$(INSTALL) -D -m 0644 support/misc/Buildroot.cmake $(@)
+TOOLCHAIN_POST_INSTALL_STAGING_HOOKS += TOOLCHAIN_CMAKE_INSTALL_FILES
diff --git a/toolchain/toolchain/toolchain.mk b/toolchain/toolchain/toolchain.mk
index b55b0c712c..283e0a74ee 100644
--- a/toolchain/toolchain/toolchain.mk
+++ b/toolchain/toolchain/toolchain.mk
@@ -38,6 +38,3 @@  TOOLCHAIN_INSTALL_STAGING = YES
 endif
 
 $(eval $(virtual-package))
-
-toolchain: $(HOST_DIR)/share/buildroot/toolchainfile.cmake
-toolchain: $(HOST_DIR)/share/buildroot/Platform/Buildroot.cmake