[Buildroot] [git commit] pkg-*targets.mk: factorize and fix $(PKG)_SRCDIR and $(PKG)_BUILDDIR declaration

[Buildroot] [git commit] pkg-*targets.mk: factorize and fix $(PKG)_SRCDIR and $(PKG)_BUILDDIR declaration

[Thomas Petazzoni]

commit: http://git.buildroot.net/buildroot/commit/?id=9ba9bfb9a02706fa414bcf4c6dcceac1b68a5c9a
branch: http://git.buildroot.net/buildroot/commit/?id=refs/heads/master

Signed-off-by: Samuel Martin <s.martin49@gmail.com>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 package/pkg-autotools.mk |   10 ----------
 package/pkg-cmake.mk     |    9 ---------
 package/pkg-generic.mk   |   11 +++++++++++
 3 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/package/pkg-autotools.mk b/package/pkg-autotools.mk
index e454050..785daab 100644
--- a/package/pkg-autotools.mk
+++ b/package/pkg-autotools.mk
@@ -54,15 +54,6 @@ endef
 
 define inner-autotools-package
 
-# define package-specific variables to default values
-ifndef $(2)_SUBDIR
- ifdef $(3)_SUBDIR
-  $(2)_SUBDIR = $($(3)_SUBDIR)
- else
-  $(2)_SUBDIR ?=
- endif
-endif
-
 ifndef $(2)_LIBTOOL_PATCH
  ifdef $(3)_LIBTOOL_PATCH
   $(2)_LIBTOOL_PATCH = $($(3)_LIBTOOL_PATCH)
@@ -91,7 +82,6 @@ $(2)_CLEAN_OPT			?= clean
 $(2)_UNINSTALL_STAGING_OPT	?= DESTDIR=$$(STAGING_DIR) uninstall
 $(2)_UNINSTALL_TARGET_OPT	?= DESTDIR=$$(TARGET_DIR)  uninstall
 
-$(2)_SRCDIR                     = $$($(2)_DIR)/$$($(2)_SUBDIR)
 
 #
 # Configure step. Only define it if not already defined by the package
diff --git a/package/pkg-cmake.mk b/package/pkg-cmake.mk
index f9a5934..626a0b0 100644
--- a/package/pkg-cmake.mk
+++ b/package/pkg-cmake.mk
@@ -37,15 +37,6 @@
 
 define inner-cmake-package
 
-# define package-specific variables to default values
-ifndef $(2)_SUBDIR
- ifdef $(3)_SUBDIR
-  $(2)_SUBDIR = $($(3)_SUBDIR)
- else
-  $(2)_SUBDIR ?=
- endif
-endif
-
 $(2)_CONF_ENV			?=
 $(2)_CONF_OPT			?=
 $(2)_MAKE			?= $(MAKE)
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index c01440e..8a730c0 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -204,6 +204,17 @@ $(2)_BASE_NAME	=  $(1)-$$($(2)_VERSION)
 $(2)_DL_DIR	=  $$(DL_DIR)/$$($(2)_BASE_NAME)
 $(2)_DIR	=  $$(BUILD_DIR)/$$($(2)_BASE_NAME)
 
+ifndef $(3)_SUBDIR
+ ifdef $(2)_SUBDIR
+  $(3)_SUBDIR = $$($(2)_SUBDIR)
+ else
+  $(3)_SUBDIR ?=
+ endif
+endif
+
+$(2)_SRCDIR		       = $$($(2)_DIR)/$$($(2)_SUBDIR)
+$(2)_BUILDDIR		       ?= $$($(2)_SRCDIR)
+
 ifneq ($$($(2)_OVERRIDE_SRCDIR),)
 $(2)_VERSION = custom
 endif

[Buildroot] [git commit] pkg-*targets.mk: factorize and fix $(PKG)_SRCDIR and $(PKG)_BUILDDIR declaration

[Arnout Vandecappelle]

On 07/22/12 19:28, Thomas Petazzoni wrote:
> +ifndef $(3)_SUBDIR
> + ifdef $(2)_SUBDIR
> +  $(3)_SUBDIR = $$($(2)_SUBDIR)
> + else
> +  $(3)_SUBDIR ?=
> + endif
> +endif

  Actually, I think this should be

$(2)_SUBDIR ?= $$($(3)_SUBDIR))

  There definitely shouldn't be an assignment to $(3)_SUBDIR here,
and also the whole ifdef construct is unnecessary.

> +
> +$(2)_SRCDIR		       = $$($(2)_DIR)/$$($(2)_SUBDIR)
> +$(2)_BUILDDIR		       ?= $$($(2)_SRCDIR)

  This looks like a missed refactoring opportunity:

_SRCDIR is still assigned to in pkg-cmake.mk

I don't see why anybody would want to override _BUILDDIR, so the ?= is
redundant.  And since it's always equal to _SRCDIR, why not just use
_SRCDIR?

  Regards,
  Arnout
-- 
Arnout Vandecappelle                               arnout at mind be
Senior Embedded Software Architect                 +32-16-286540
Essensium/Mind                                     http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium                BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [git commit] pkg-*targets.mk: factorize and fix $(PKG)_SRCDIR and $(PKG)_BUILDDIR declaration

[Samuel Martin]

Hi Arnout,

2012/7/25 Arnout Vandecappelle <arnout@mind.be>:
> On 07/22/12 19:28, Thomas Petazzoni wrote:
>>
>> +ifndef $(3)_SUBDIR
>> + ifdef $(2)_SUBDIR
>> +  $(3)_SUBDIR = $$($(2)_SUBDIR)
>> + else
>> +  $(3)_SUBDIR ?=
>> + endif
>> +endif
>
>
>  Actually, I think this should be
>
> $(2)_SUBDIR ?= $$($(3)_SUBDIR))
>
>  There definitely shouldn't be an assignment to $(3)_SUBDIR here,
> and also the whole ifdef construct is unnecessary.
Yep, I messed-up inner-generic-package argument order :/, Thomas fixed it:
http://git.buildroot.net/buildroot/commit/?id=8e26abecd00f4899fb122c3eeb03fbdf20cda32e

>
>
>> +
>> +$(2)_SRCDIR                   = $$($(2)_DIR)/$$($(2)_SUBDIR)
>> +$(2)_BUILDDIR                 ?= $$($(2)_SRCDIR)
>
>
>  This looks like a missed refactoring opportunity:
>
> _SRCDIR is still assigned to in pkg-cmake.mk
>
> I don't see why anybody would want to override _BUILDDIR, so the ?= is
> redundant.  And since it's always equal to _SRCDIR, why not just use
> _SRCDIR?
Recently, I played with eigen an tryied to integrate it in BR.
Though it's a header only package using cmake as buid-system, it does
not support in-source-tree build.

So, here I use ?=, in the case someday we implement out-of-source-tree
build; it's easy with cmake, not so easy with autotools, and I have to
think about this for the rest.


Cheers,

-- 
Sam