[Buildroot] [v2] eigen: (optionally) install unsupported modules

[Buildroot] [v2] eigen: (optionally) install unsupported modules

[Davide Viti]

I'm working on a package that includes some files inside the
"unsupported" folder. The unsupported modules can be optionally
selected.

Let's see if I can manage to properly send a signed patch with
introductory message: sorry for the noise!

regards,
Davide

[Buildroot] [PATCH] eigen: (optionally) install unsupported modules

[Davide Viti]

From: Davide Viti <d.viti@infosolution.it>


Signed-off-by: Davide Viti <zinosat@tiscali.it>
---
 package/eigen/Config.in |    8 ++++++++
 package/eigen/eigen.mk  |    8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/package/eigen/Config.in b/package/eigen/Config.in
index e94f9a3..a653e37 100644
--- a/package/eigen/Config.in
+++ b/package/eigen/Config.in
@@ -13,5 +13,13 @@ config BR2_PACKAGE_EIGEN
 
 	  http://eigen.tuxfamily.org/
 
+if BR2_PACKAGE_EIGEN
+
+config BR2_PACKAGE_EIGEN_UNSUPPORTED_MODULES
+        bool "unsupported modules"
+	help
+	  Install eigen unsupported modules
+endif
+
 comment "eigen needs a toolchain w/ C++"
 	depends on !BR2_INSTALL_LIBSTDCPP
diff --git a/package/eigen/eigen.mk b/package/eigen/eigen.mk
index 5abd464..388dd63 100644
--- a/package/eigen/eigen.mk
+++ b/package/eigen/eigen.mk
@@ -13,6 +13,14 @@ EIGEN_LICENSE_FILES = COPYING.MPL2 COPYING.BSD COPYING.LGPL COPYING.README
 EIGEN_INSTALL_STAGING = YES
 EIGEN_INSTALL_TARGET = NO
 
+define EIGEN_INSTALL_UNSUPPORTED_MODULES
+	cp -a $(@D)/unsupported $(STAGING_DIR)/usr/include/
+endef
+
+ifeq ($(BR2_PACKAGE_EIGEN_UNSUPPORTED_MODULES),y)
+	EIGEN_POST_INSTALL_STAGING_HOOKS += EIGEN_INSTALL_UNSUPPORTED_MODULES
+endif
+
 # This package only consists of headers that need to be
 # copied over to the sysroot for compile time use
 define EIGEN_INSTALL_STAGING_CMDS
-- 
1.7.10.4

[Buildroot] [v2] eigen: (optionally) install unsupported modules

[Thomas Petazzoni]

Dear Davide Viti,

On Tue, 18 Mar 2014 21:18:13 +0100, Davide Viti wrote:
> I'm working on a package that includes some files inside the
> "unsupported" folder. The unsupported modules can be optionally
> selected.
> 
> Let's see if I can manage to properly send a signed patch with
> introductory message: sorry for the noise!

Almost :-)

For a single patch, there is no real need to send a separate
introduction message, the patch itself is enough.

To indicate the new version, you should generate the patch as follows:

	git format-patch --suject-prefix="PATCHv2" HEAD^

so that the patch title becomes:

	[PATCHv2] title of the patch

You manually added [v2] to the title of your introduction e-mail, but
it's actually more important to have it on the patches themselves. And
don't do it manually, use git --subject-prefix option to do it!

See
http://buildroot.org/downloads/manual/manual.html#_patch_revision_changelog
for more details.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] eigen: (optionally) install unsupported modules

[Thomas Petazzoni]

Dear Davide Viti,

On Tue, 18 Mar 2014 21:18:14 +0100, Davide Viti wrote:
> From: Davide Viti <d.viti@infosolution.it>
> 
> 
> Signed-off-by: Davide Viti <zinosat@tiscali.it>

We probably want the title of the patch to be:

	eigen: add an option to install unsupported modules

>  
> +if BR2_PACKAGE_EIGEN
> +
> +config BR2_PACKAGE_EIGEN_UNSUPPORTED_MODULES
> +        bool "unsupported modules"

Indentation is wrong here: there should be one tab before the "bool"
statement.

> +	help
> +	  Install eigen unsupported modules
> +endif
> +
>  comment "eigen needs a toolchain w/ C++"
>  	depends on !BR2_INSTALL_LIBSTDCPP
> diff --git a/package/eigen/eigen.mk b/package/eigen/eigen.mk
> index 5abd464..388dd63 100644
> --- a/package/eigen/eigen.mk
> +++ b/package/eigen/eigen.mk
> @@ -13,6 +13,14 @@ EIGEN_LICENSE_FILES = COPYING.MPL2 COPYING.BSD COPYING.LGPL COPYING.README
>  EIGEN_INSTALL_STAGING = YES
>  EIGEN_INSTALL_TARGET = NO
>  
> +define EIGEN_INSTALL_UNSUPPORTED_MODULES
> +	cp -a $(@D)/unsupported $(STAGING_DIR)/usr/include/

I'm a bit worried about having a directory called
$(STAGING_DIR)/usr/include/unsupported. Shouldn't this directory be
installed *below* the $(STAGING_DIR)/usr/include/Eigen directory
created by the normal installation of Eigen?

> +endef
> +
> +ifeq ($(BR2_PACKAGE_EIGEN_UNSUPPORTED_MODULES),y)
> +	EIGEN_POST_INSTALL_STAGING_HOOKS += EIGEN_INSTALL_UNSUPPORTED_MODULES
> +endif

I believe we normally don't really use post install hooks when there is
a definition of the staging install commands. Instead, we use the
following idiom:

ifeq ($(BR2_PACKAGE_EIGEN_UNSUPPORTED_MODULES),y)
define EIGEN_INSTALL_UNSUPPORTED_MODULES_CMDS
	cp -a ...
endef
endif

define EIGEN_INSTALL_STAGING_CMDS
	... existing installation steps ...
	$(EIGEN_INSTALL_UNSUPPORTED_MODULES_CMDS)
endef

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] eigen: (optionally) install unsupported modules

[Samuel Martin]

Thomas, Davide, all,

On Tue, Mar 18, 2014 at 11:55 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Davide Viti,
>
> On Tue, 18 Mar 2014 21:18:14 +0100, Davide Viti wrote:
>> From: Davide Viti <d.viti@infosolution.it>
[...]
>>
>> +define EIGEN_INSTALL_UNSUPPORTED_MODULES
>> +     cp -a $(@D)/unsupported $(STAGING_DIR)/usr/include/
>
> I'm a bit worried about having a directory called
> $(STAGING_DIR)/usr/include/unsupported. Shouldn't this directory be
> installed *below* the $(STAGING_DIR)/usr/include/Eigen directory
> created by the normal installation of Eigen?

This goes a bit against what eigen's build-system (cmake) does by default.
The "Eigen" and "unsupported" directories are installed in the same
include directory
(defaults: /usr/include/eigen3).

I agree such a change may break some code in Buildroot users' code,
but I'm also worried about not following default install scheme done
by upstream projects.


Regards,

-- 
Samuel

[Buildroot] [PATCH] eigen: (optionally) install unsupported modules

[Davide Viti]

Hi Samuel and Thomas,
I can provide a patch that installs the files under
/usr/include/eigen3 and modify my latest patch to install unsupported
under that directory too.

I guess separate patches are preferred.

I can do it later on today.

Regards,
Davide

> Il giorno 19/mar/2014, alle ore 08:25, Samuel Martin <s.martin49@gmail.com> ha scritto:
>
> Thomas, Davide, all,
>
> On Tue, Mar 18, 2014 at 11:55 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
>> Dear Davide Viti,
>>
>>> On Tue, 18 Mar 2014 21:18:14 +0100, Davide Viti wrote:
>>> From: Davide Viti <d.viti@infosolution.it>
> [...]
>>>
>>> +define EIGEN_INSTALL_UNSUPPORTED_MODULES
>>> +     cp -a $(@D)/unsupported $(STAGING_DIR)/usr/include/
>>
>> I'm a bit worried about having a directory called
>> $(STAGING_DIR)/usr/include/unsupported. Shouldn't this directory be
>> installed *below* the $(STAGING_DIR)/usr/include/Eigen directory
>> created by the normal installation of Eigen?
>
> This goes a bit against what eigen's build-system (cmake) does by default.
> The "Eigen" and "unsupported" directories are installed in the same
> include directory
> (defaults: /usr/include/eigen3).
>
> I agree such a change may break some code in Buildroot users' code,
> but I'm also worried about not following default install scheme done
> by upstream projects.
>
>
> Regards,
>
> --
> Samuel

[Buildroot] [PATCH] eigen: (optionally) install unsupported modules

[Thomas Petazzoni]

Dear Davide Viti,

On Wed, 19 Mar 2014 09:07:10 +0100, Davide Viti wrote:

> I can provide a patch that installs the files under
> /usr/include/eigen3 and modify my latest patch to install unsupported
> under that directory too.

Seems like a good choice to me.

> I guess separate patches are preferred.

Great, thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com