Skip to contentSkip to conversation

eigen: (optionally) install unsupported modules

PATCH New Davide Viti 3 participants 7 messages Mar 18, 2014 – Mar 19, 2014

lore
Share & tools
lore

Davide Viti

author
#0 lore raw

PATCH v2 1/1 eigen: (optionally) install unsupported modules

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

author
#1 lore raw

PATCH 1/1 eigen: (optionally) install unsupported modules

From: Davide Viti <[email protected]>

Signed-off-by Davide Viti <[email protected]>
---
 package/eigen/Config.in |    8 ++++++++
 package/eigen/eigen.mk  |    8 ++++++++
 2 files changed, 16 insertions(+)
package/eigen/Config.in +8 -0
index e94f9a3..a653e37 100644
@@ -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
package/eigen/eigen.mk +8 -0
index 5abd464..388dd63 100644
@@ -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
Signature · 1 line
1.7.10.4
#2 lore raw

PATCH 1/1 eigen: (optionally) install unsupported modules

Dear Davide Viti,

Quoted 4 lines · Davide Viti

From: Davide Viti <[email protected]>

Signed-off-by Davide Viti <[email protected]>

We probably want the title of the patch to be:

eigen: add an option to install unsupported modules

package/eigen/Config.in config BR2_PACKAGE_EIGEN · line 19

Quoted 5 lines
package/eigen/Config.in +4 -0
+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.

Quoted 11 lines
package/eigen/Config.in +4 -0
+	help
+	  Install eigen unsupported modules
+endif
+
 comment "eigen needs a toolchain w/ C++"
 	depends on !BR2_INSTALL_LIBSTDCPP
package/eigen/eigen.mk +0 -0
index 5abd464..388dd63 100644

package/eigen/eigen.mk EIGEN_LICENSE_FILES = COPYING.MPL2 COPYING.BSD COPYING.LGPL COPYING.README · line 17

Quoted 5 lines
package/eigen/eigen.mk +2 -0
@@ -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?

package/eigen/eigen.mk EIGEN_LICENSE_FILES = COPYING.MPL2 COPYING.BSD COPYING.LGPL COPYING.README · line 22

Quoted 5 lines
package/eigen/eigen.mk +5 -0
+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

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#3 lore raw

PATCH 1/1 eigen: (optionally) install unsupported modules

Thomas, Davide, all,

Quoted 4 lines · Thomas Petazzoni

Dear Davide Viti,

Quoted 1 line · Davide Viti

From: Davide Viti <[email protected]>

[...]

Quoted 8 lines

package/eigen/eigen.mk EIGEN_LICENSE_FILES = COPYING.MPL2 COPYING.BSD COPYING.LGPL COPYING.README · line 17

Quoted 3 lines
package/eigen/eigen.mk +2 -0
+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,

Signature · 1 line
Samuel
author
#4 lore raw

PATCH 1/1 eigen: (optionally) install unsupported modules

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

Quoted 34 lines

Il giorno 19/mar/2014, alle ore 08:25, Samuel Martin <[email protected]> ha scritto:

Thomas, Davide, all,

Quoted 4 lines · Thomas Petazzoni

Dear Davide Viti,

Quoted 2 lines · Davide Viti

From: Davide Viti <[email protected]>

[...]

Quoted 8 lines

package/eigen/eigen.mk EIGEN_LICENSE_FILES = COPYING.MPL2 COPYING.BSD COPYING.LGPL COPYING.README · line 17

Quoted 3 lines
package/eigen/eigen.mk +2 -0
+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

#5 lore raw

PATCH 1/1 eigen: (optionally) install unsupported modules

Dear Davide Viti,

Quoted 3 lines · Davide Viti

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.

Quoted 1 line

I guess separate patches are preferred.

Great, thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#6 lore raw

PATCH v2 1/1 eigen: (optionally) install unsupported modules

In reply to Davide Viti

Dear Davide Viti,

Quoted 6 lines · 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!

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

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