diff mbox

[06/11,v2] package/cryptodev-linux: use the kernel-module helper

Message ID 329d95315f937a0cbd912b8035fb0ccc0e94a9ed.1433964001.git.yann.morin.1998@free.fr
State Changes Requested
Headers show

Commit Message

Yann E. MORIN June 10, 2015, 7:22 p.m. UTC
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
[Doug: add pre-build hook to build version.h]
Signed-off-by: Doug Kehn <rdkehn@yahoo.com>

---
Cyhanges v1 -> v2:
  - fix missing version.h  (Doug)
---
 package/cryptodev-linux/cryptodev-linux.mk | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Arnout Vandecappelle June 11, 2015, 11:02 p.m. UTC | #1
On 06/10/15 21:22, Yann E. MORIN wrote:
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> [Doug: add pre-build hook to build version.h]

 The version.h thing deserves a slightly more explicit commit message, like:

The cryptodev-linux Makefile defines the following:

build: version.h
        make ${KERNEL_MAKE_OPTS} modules

modules_install:
        make -C $(KERNEL_DIR) SUBDIRS=`pwd` modules_install
        @echo "Installing cryptodev.h in $(PREFIX)/usr/include/crypto ..."
        @install -D crypto/cryptodev.h $(PREFIX)/usr/include/crypto/cryptodev.h

So when we use the kernel-module infrastructure, we have to explicitly handle
version.h and cryptodev.h. cryptodev.h was already handled specifically because
it goes to STAGING_DIR. For version.h, we add a pre-build hook.


 That said, SUBDIRS= does not behave exactly the same as M=, but I don't know
what the difference is. So that part should probably be mentioned explicitly in
the commit message as well.

> Signed-off-by: Doug Kehn <rdkehn@yahoo.com>

[snip]
> -define CRYPTODEV_LINUX_INSTALL_TARGET_CMDS
> -	$(MAKE) -C $(@D) $(LINUX_MAKE_FLAGS) KERNEL_DIR=$(LINUX_DIR) \
> -		PREFIX=$(TARGET_DIR) modules_install
> -endef
> +CRYPTODEV_LINUX_MODULE_MAKE_OPTS = \
> +	KERNEL_DIR=$(LINUX_DIR) \
> +	PREFIX=$(TARGET_DIR)

 PREFIX is only used for installing cryptodev.h, which is now no longer done. So
we don't need to define it, right? Same for KERNEL_DIR, it's only used by the
rules we no longer use.

 If you remove these, it should of course also be mentioned explicitly in the
commit message.


 Regards,
 Arnout

>  
>  define CRYPTODEV_LINUX_INSTALL_STAGING_CMDS
>  	$(INSTALL) -D -m 644 $(@D)/crypto/cryptodev.h \
>  		$(STAGING_DIR)/usr/include/crypto/cryptodev.h
>  endef
>  
> +$(eval $(kernel-module))
>  $(eval $(generic-package))
>
Yann E. MORIN June 12, 2015, 8:48 p.m. UTC | #2
Arnout, All,

On 2015-06-12 01:02 +0200, Arnout Vandecappelle spake thusly:
> On 06/10/15 21:22, Yann E. MORIN wrote:
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > [Doug: add pre-build hook to build version.h]
> 
>  The version.h thing deserves a slightly more explicit commit message, like:
> 
> The cryptodev-linux Makefile defines the following:
> 
> build: version.h
>         make ${KERNEL_MAKE_OPTS} modules
> 
> modules_install:
>         make -C $(KERNEL_DIR) SUBDIRS=`pwd` modules_install
>         @echo "Installing cryptodev.h in $(PREFIX)/usr/include/crypto ..."
>         @install -D crypto/cryptodev.h $(PREFIX)/usr/include/crypto/cryptodev.h

I'm not too fond of quoting that much, so I trimmed it down to what I
believe is the strict minimum for udnerstanding the change, no more.

If you still think that is not enough, you'll have the opportunity to
say so when I respin. OK? ;-)

> So when we use the kernel-module infrastructure, we have to explicitly handle
> version.h and cryptodev.h. cryptodev.h was already handled specifically because
> it goes to STAGING_DIR. For version.h, we add a pre-build hook.
> 
> 
>  That said, SUBDIRS= does not behave exactly the same as M=, but I don't know
> what the difference is. So that part should probably be mentioned explicitly in
> the commit message as well.

From my understanding, the end result is the same, but using SUBDIRS
makes the Linux buildsystem it is building in in-tree module, while M
tells it it is building an out-of-tree module.

> > Signed-off-by: Doug Kehn <rdkehn@yahoo.com>
> 
> [snip]
> > -define CRYPTODEV_LINUX_INSTALL_TARGET_CMDS
> > -	$(MAKE) -C $(@D) $(LINUX_MAKE_FLAGS) KERNEL_DIR=$(LINUX_DIR) \
> > -		PREFIX=$(TARGET_DIR) modules_install
> > -endef
> > +CRYPTODEV_LINUX_MODULE_MAKE_OPTS = \
> > +	KERNEL_DIR=$(LINUX_DIR) \
> > +	PREFIX=$(TARGET_DIR)
> 
>  PREFIX is only used for installing cryptodev.h, which is now no longer done. So
> we don't need to define it, right? Same for KERNEL_DIR, it's only used by the
> rules we no longer use.
> 
>  If you remove these, it should of course also be mentioned explicitly in the
> commit message.

Right, I've dropped both and explained in the commit log.

Thanks! :-)

Regards,
Yann E. MORIN.
diff mbox

Patch

diff --git a/package/cryptodev-linux/cryptodev-linux.mk b/package/cryptodev-linux/cryptodev-linux.mk
index 6f41930..6eec602 100644
--- a/package/cryptodev-linux/cryptodev-linux.mk
+++ b/package/cryptodev-linux/cryptodev-linux.mk
@@ -6,23 +6,23 @@ 
 
 CRYPTODEV_LINUX_VERSION = 1.7
 CRYPTODEV_LINUX_SITE = http://download.gna.org/cryptodev-linux
-CRYPTODEV_LINUX_DEPENDENCIES = linux
 CRYPTODEV_LINUX_INSTALL_STAGING = YES
 CRYPTODEV_LINUX_LICENSE = GPLv2+
 CRYPTODEV_LINUX_LICENSE_FILES = COPYING
 
-define CRYPTODEV_LINUX_BUILD_CMDS
-	$(MAKE) -C $(@D) $(LINUX_MAKE_FLAGS) KERNEL_DIR=$(LINUX_DIR)
+define CRYPTODEV_LINUX_VERSION_H
+	$(MAKE) -C $(@D) version.h
 endef
+CRYPTODEV_LINUX_PRE_BUILD_HOOKS += CRYPTODEV_LINUX_VERSION_H
 
-define CRYPTODEV_LINUX_INSTALL_TARGET_CMDS
-	$(MAKE) -C $(@D) $(LINUX_MAKE_FLAGS) KERNEL_DIR=$(LINUX_DIR) \
-		PREFIX=$(TARGET_DIR) modules_install
-endef
+CRYPTODEV_LINUX_MODULE_MAKE_OPTS = \
+	KERNEL_DIR=$(LINUX_DIR) \
+	PREFIX=$(TARGET_DIR)
 
 define CRYPTODEV_LINUX_INSTALL_STAGING_CMDS
 	$(INSTALL) -D -m 644 $(@D)/crypto/cryptodev.h \
 		$(STAGING_DIR)/usr/include/crypto/cryptodev.h
 endef
 
+$(eval $(kernel-module))
 $(eval $(generic-package))