Package based 'source', 'legal-info', 'source-check' and 'external-deps'

Commit 4570dbcb14b394d8f3c97baf4c9e5f4f225d1d37 ("4570dbcb14b394d8f3c97baf4c9e5f4f225d1d37") added the dependencies of rootfs building logic to PACKAGES, only thinking about the case where rootfs building logic actually depends on real, normal packages (host-mtd, host-genext2fs, etc.). But some file systems (rootfs-ubi) depend on other filesystem images (rootfs-ubifs). And such targets should not be added to PACKAGES, other an incorrect circular

dependency is created.

This commit fixes that by only adding the rootfs building logic dependencies that do *not* start with rootfs- to the global PACKAGES variable.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 fs/common.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/common.mk +1 -1

index 41ee86d..4387d06 100644
@@ -106,7 +106,7 @@ rootfs-$(1): $$(BINARIES_DIR)/rootfs.$(1) $$(ROOTFS_$(2)_POST_TARGETS)
 
 ifeq ($$(BR2_TARGET_ROOTFS_$(2)),y)
 TARGETS_ROOTFS += rootfs-$(1)
-PACKAGES += $$(ROOTFS_$(2)_DEPENDENCIES)
+PACKAGES += $$(filter-out rootfs-%,$$(ROOTFS_$(2)_DEPENDENCIES))
 endif
 endef
 

Signature · 1 line

2.1.0

The $(1)-all-{source,external-deps,legal-info} targets currently only take care of the dependencies in <pkg>_DEPENDENCIES, but not <pkg>_PATCH_DEPENDENCIES. This patch fixes that.

Long term, we might want to refactor this to have a single variable containing all dependencies. But this requires more work in pkg-generic than we want to do at the moment.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 package/pkg-generic.mk | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

package/pkg-generic.mk +0 -0

index 455bdf1..de63f2f 100644
@@ -592,11 +592,20 @@ $(1)-graph-depends: graph-depends-requirements
 			|tee $$(GRAPHS_DIR)/$$(@).dot \
 			|dot $$(BR2_GRAPH_DOT_OPTS) -T$$(BR_GRAPH_OUT) -o $$(GRAPHS_DIR)/$$(@).$$(BR_GRAPH_OUT)

package/pkg-generic.mk +9 -0

+$(1)-all-external-deps:	$$(foreach p, \
+				$$($(2)_FINAL_DEPENDENCIES) $$($(2)_FINAL_PATCH_DEPENDENCIES), \
+				$$(p)-all-external-deps) \
+			$(1)-external-deps
+
+$(1)-all-legal-info:	$$(foreach p, \
+				$$($(2)_FINAL_DEPENDENCIES) $$($(2)_FINAL_PATCH_DEPENDENCIES), \
+				$$(p)-all-legal-info) \
+			$(1)-legal-info
 
 $(1)-dirclean:		$$($(2)_TARGET_DIRCLEAN)
 

Signature · 1 line

2.1.0

Thomas, All,

On 2015-04-25 16:42 +0200, Thomas Petazzoni spake thusly:

Quoted 4 lines

The $(1)-all-{source,external-deps,legal-info} targets currently only take care of the dependencies in <pkg>_DEPENDENCIES, but not <pkg>_PATCH_DEPENDENCIES. This patch fixes that.

Quoted 3 lines

Long term, we might want to refactor this to have a single variable containing all dependencies. But this requires more work in pkg-generic than we want to do at the moment.

Well, you can still introduce $(2)_FINAL_ALL_DEPENDENCIES. That would make the code reall much more readable.

Well, that's really unreadable.

What about:

    $(1)-all-source: $(1)-source
    $(1)-all-source: $$(foreach p,$$($(2)_FINAL_ALL_DEPENDENCIES),$$(p)-all-source)

Of course that require _FINAL_ALL_DEPENDENCIEs, but I guess the cost is worth the gain.

As part of moving to a package infrastructure based source-check implementation, we are going to move away from the global DL_MODE variable to select the behavior of the DOWNLOAD_INNER macro.

As a preparation to this, this commit makes the DOWNLOAD_INNER macro take a third argument, which is the action to be done: either DOWNLOAD or SOURCE_CHECK. For now, the DOWNLOAD macro passes $(DL_MODE) as this third argument, in order to keep the existing behavior.

In addition, a SOURCE_CHECK macro is added, which calls DOWNLOAD_INNER with the appropriate action. This macro will be used in the upcoming package infra based implementation of source-check.

Signed-off-by Thomas Petazzoni <[email protected]>

Reviewed-by "Yann E. MORIN" <[email protected]>

Tested-by "Yann E. MORIN" <[email protected]>

[tested by doing a "make source" on a randpackageconfig]

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Signed-off-by Thomas Petazzoni <[email protected]>

---
 package/pkg-download.mk | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

package/pkg-download.mk +16 -12

index eee9e59..76afced 100644
@@ -212,14 +212,18 @@ endef
 ################################################################################
 
 define DOWNLOAD
-	$(call DOWNLOAD_INNER,$(1),$(notdir $(1)))
+	$(call DOWNLOAD_INNER,$(1),$(notdir $(1)),$(DL_MODE))
+endef
+
+define SOURCE_CHECK
+	$(call DOWNLOAD_INNER,$(1),$(notdir $(1)),SOURCE_CHECK)
 endef
 
 define DOWNLOAD_INNER
 	$(Q)if test -n "$(call qstrip,$(BR2_PRIMARY_SITE))" ; then \
 		case "$(call geturischeme,$(BR2_PRIMARY_SITE))" in \
-			scp) $(call $(DL_MODE)_SCP,$(BR2_PRIMARY_SITE)/$(2),$(2)) && exit ;; \
-			*) $(call $(DL_MODE)_WGET,$(BR2_PRIMARY_SITE)/$(2),$(2)) && exit ;; \
+			scp) $(call $(3)_SCP,$(BR2_PRIMARY_SITE)/$(2),$(2)) && exit ;; \
+			*) $(call $(3)_WGET,$(BR2_PRIMARY_SITE)/$(2),$(2)) && exit ;; \
 		esac ; \
 	fi ; \
 	if test "$(BR2_PRIMARY_SITE_ONLY)" = "y" ; then \

@@ -232,18 +236,18 @@ define DOWNLOAD_INNER
 			scheme="$($(PKG)_SITE_METHOD)" ; \
 		fi ; \
 		case "$$scheme" in \
-			git) $($(DL_MODE)_GIT) && exit ;; \
-			svn) $($(DL_MODE)_SVN) && exit ;; \
-			cvs) $($(DL_MODE)_CVS) && exit ;; \
-			bzr) $($(DL_MODE)_BZR) && exit ;; \
-			file) $($(DL_MODE)_LOCALFILES) && exit ;; \
-			scp) $($(DL_MODE)_SCP) && exit ;; \
-			hg) $($(DL_MODE)_HG) && exit ;; \
-			*) $(call $(DL_MODE)_WGET,$(1),$(2)) && exit ;; \
+			git) $($(3)_GIT) && exit ;; \
+			svn) $($(3)_SVN) && exit ;; \
+			cvs) $($(3)_CVS) && exit ;; \
+			bzr) $($(3)_BZR) && exit ;; \
+			file) $($(3)_LOCALFILES) && exit ;; \
+			scp) $($(3)_SCP) && exit ;; \
+			hg) $($(3)_HG) && exit ;; \
+			*) $(call $(3)_WGET,$(1),$(2)) && exit ;; \
 		esac ; \
 	fi ; \
 	if test -n "$(call qstrip,$(BR2_BACKUP_SITE))" ; then \
-		$(call $(DL_MODE)_WGET,$(BR2_BACKUP_SITE)/$(2),$(2)) && exit ; \
+		$(call $(3)_WGET,$(BR2_BACKUP_SITE)/$(2),$(2)) && exit ; \
 	fi ; \
 	exit 1
 endef

Signature · 1 line

2.1.0

This commits extends the common package infrastructure with the $(1)-source-check and $(1)-all-source-check targets.

The $(1)-source-check target simply calls the newly added SOURCE_CHECK macro on all items to be downloaded.

The $(1)-all-source-check target will depend on the $(1)-all-source-check targets of all dependent packages and the $(1)-source-check target of the current package, which allows to do a recursive source-check in the dependency tree.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 package/pkg-generic.mk | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

package/pkg-generic.mk +10 -0

index a12b868..5a9bcf1 100644
@@ -579,6 +579,16 @@ endif
 $(1)-show-version:
 			@echo $$($(2)_VERSION)
 
+$(1)-source-check:
+ifeq ($$($(2)_OVERRIDE_SRCDIR),)
+	$$(foreach p,$$($(2)_SOURCE) $$($(2)_EXTRA_DOWNLOADS) $$($(2)_PATCH),\
+		$$(if $$(findstring ://,$$(p)),\
+			$$(call SOURCE_CHECK,$$(p)),\
+			$$(call SOURCE_CHECK,$$($(2)_SITE:/=)/$$(p)))$$(sep))
+else
+	test -d $$($(2)_OVERRIDE_SRCDIR)
+endif
+
 $(1)-show-depends:
 			@echo $$(sort $$($(2)_FINAL_DEPENDENCIES) $$($(2)_FINAL_PATCH_DEPENDENCIES))
 

package/pkg-generic.mk +3 -1

 $(1)-all-external-deps:	$$(foreach p, \
 				$$($(2)_FINAL_DEPENDENCIES) $$($(2)_FINAL_PATCH_DEPENDENCIES), \
 				$$(p)-all-external-deps) \

@@ -793,6 +808,7 @@ endif
 	$(1)-all-external-deps \
 	$(1)-all-legal-info \
 	$(1)-all-source \
+	$(1)-all-source-check \
 	$(1)-build \
 	$(1)-clean-for-rebuild \
 	$(1)-clean-for-reconfigure \

@@ -816,7 +832,8 @@ endif
 	$(1)-rsync \
 	$(1)-show-depends \
 	$(1)-show-version \
-	$(1)-source
+	$(1)-source \
+	$(1)-source-check
 
 endif # $(2)_KCONFIG_VAR
 endef # inner-generic-package

Signature · 1 line

2.1.0

To be noted: packages hosted at github, when we download a released tarball, are broken, because the backing-store is AWS, which does not support the HEAD request. But that is not a change due to this patch, it was already broken previously.

package/pkg-generic.mk $(1)-all-source: $$(foreach p, \ · line 610

Quoted 4 lines

+$(1)-all-source-check: $$(foreach p, \

package/pkg-generic.mk +3 -0

+				$$($(2)_FINAL_DEPENDENCIES) $$($(2)_FINAL_PATCH_DEPENDENCIES), \
+				$$(p)-all-source-check) \
+			$(1)-source-check

If you do the little trick I mentioned as a comment to your patch 2, you'd have to do it here, too.

Quoted 4 lines

To be noted: packages hosted at github, when we download a released tarball, are broken, because the backing-store is AWS, which does not support the HEAD request. But that is not a change due to this patch, it was already broken previously.

Right. Let's not fix all the problems in this patch series :-)

package/pkg-generic.mk +0 -2

 $(1)-external-deps:
 	@echo "file://$$($(2)_OVERRIDE_SRCDIR)"

@@ -652,8 +643,6 @@ $$($(2)_TARGET_BUILD):			PKG=$(2)
 $$($(2)_TARGET_CONFIGURE):		PKG=$(2)
 $$($(2)_TARGET_RSYNC):                  SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
 $$($(2)_TARGET_RSYNC):                  PKG=$(2)
-$$($(2)_TARGET_RSYNC_SOURCE):		SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
-$$($(2)_TARGET_RSYNC_SOURCE):		PKG=$(2)
 $$($(2)_TARGET_PATCH):			PKG=$(2)
 $$($(2)_TARGET_PATCH):			RAWNAME=$$(patsubst host-%,%,$(1))
 $$($(2)_TARGET_PATCH):			PKGDIR=$(pkgdir)

Signature · 1 line

2.1.0

Quoted 2 lines

-$(1)-source: $$($(2)_TARGET_RSYNC_SOURCE)
+$(1)-source:

The reason that line existed was to be able to define a dependency on the stamp file. There is now no reason to keep this line at all; all you have above is declaring a make goal without dependency or rule, so it is entirely useless.

Quoted 8 lines

Quoted 3 lines

-$(1)-source: $$($(2)_TARGET_RSYNC_SOURCE)
+$(1)-source:

The reason that line existed was to be able to define a dependency on the stamp file. There is now no reason to keep this line at all; all you have above is declaring a make goal without dependency or rule, so it is entirely useless.

I don't think a target with no dependencies or rules is useless. It can be useful to not have an error when this target is triggered.

When you are going to do a 'make source', it will do a 'make $(1)-source' on all packages, including the ones that have <pkg>_OVERRIDE_SRCDIR. So there must be a $(1)-source target, even if it doesn't do anything.

Dear Yann E. MORIN,

Quoted 7 lines · Yann E. MORIN

Quoted 2 lines

-$(1)-source: $$($(2)_TARGET_RSYNC_SOURCE)
+$(1)-source:

The reason that line existed was to be able to define a dependency on the stamp file. There is now no reason to keep this line at all; all you have above is declaring a make goal without dependency or rule, so it is entirely useless.

I don't think a target with no dependencies or rules is useless. It can be useful to not have an error when this target is triggered.

When you are going to do a 'make source', it will do a 'make $(1)-source' on all packages, including the ones that have <pkg>_OVERRIDE_SRCDIR. So there must be a $(1)-source target, even if it doesn't do anything.

Am I missing something?

The only reason for the .stamp_rsync_sourced fake stamp file target to exist was to handle the SOURCE_CHECK operation on packages using the OVERRIDE_SRCDIR mechanism. Now that this is handled directly inside $(1)-source-check, there is no longer any need for this part of the code.

Signed-off-by Thomas Petazzoni <[email protected]>

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Tested-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

---
 package/pkg-generic.mk | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

package/pkg-generic.mk +1 -12

index 5a9bcf1..05831d4 100644
@@ -122,14 +122,6 @@ $(BUILD_DIR)/%/.stamp_rsynced:
 	$(foreach hook,$($(PKG)_POST_RSYNC_HOOKS),$(call $(hook))$(sep))
 	$(Q)touch $@
 
-# Handle the SOURCE_CHECK case for rsynced packages
-$(BUILD_DIR)/%/.stamp_rsync_sourced:
-ifeq ($(DL_MODE),SOURCE_CHECK)
-	test -d $(SRCDIR)
-else
-	@true # Nothing to do to source a local package
-endif
-
 # Patch
 #
 # The RAWNAME variable is the lowercased package name, which allows to

@@ -441,7 +433,6 @@ $(2)_TARGET_INSTALL_HOST =      $$($(2)_DIR)/.stamp_host_installed
 $(2)_TARGET_BUILD =		$$($(2)_DIR)/.stamp_built
 $(2)_TARGET_CONFIGURE =		$$($(2)_DIR)/.stamp_configured
 $(2)_TARGET_RSYNC =	        $$($(2)_DIR)/.stamp_rsynced
-$(2)_TARGET_RSYNC_SOURCE =      $$($(2)_DIR)/.stamp_rsync_sourced
 $(2)_TARGET_PATCH =		$$($(2)_DIR)/.stamp_patched
 $(2)_TARGET_EXTRACT =		$$($(2)_DIR)/.stamp_extracted
 $(2)_TARGET_SOURCE =		$$($(2)_DIR)/.stamp_downloaded

@@ -570,7 +561,7 @@ $(1)-extract:		$(1)-rsync
 
 $(1)-rsync:		$$($(2)_TARGET_RSYNC)
 
-$(1)-source:		$$($(2)_TARGET_RSYNC_SOURCE)
+$(1)-source:
 
 $(1)-external-deps:
 	@echo "file://$$($(2)_OVERRIDE_SRCDIR)"

@@ -652,8 +643,6 @@ $$($(2)_TARGET_BUILD):			PKG=$(2)
 $$($(2)_TARGET_CONFIGURE):		PKG=$(2)
 $$($(2)_TARGET_RSYNC):                  SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
 $$($(2)_TARGET_RSYNC):                  PKG=$(2)
-$$($(2)_TARGET_RSYNC_SOURCE):		SRCDIR=$$($(2)_OVERRIDE_SRCDIR)
-$$($(2)_TARGET_RSYNC_SOURCE):		PKG=$(2)
 $$($(2)_TARGET_PATCH):			PKG=$(2)
 $$($(2)_TARGET_PATCH):			RAWNAME=$$(patsubst host-%,%,$(1))
 $$($(2)_TARGET_PATCH):			PKGDIR=$(pkgdir)

Signature · 1 line

2.1.0

The .stamp_downloaded target is now only being used to really download, and no longer for other activities like "source check" or "external deps". So the check on DL_MODE being equal to DOWNLOAD is no longer necessary.

Signed-off-by Thomas Petazzoni <[email protected]>

Tested-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Acked-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

---
 package/pkg-generic.mk | 4 ----
 1 file changed, 4 deletions(-)

package/pkg-generic.mk +0 -4

index 05831d4..cb901ae 100644
@@ -71,7 +71,6 @@ endif
 # Retrieve the archive
 $(BUILD_DIR)/%/.stamp_downloaded:
 	$(foreach hook,$($(PKG)_PRE_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
-ifeq ($(DL_MODE),DOWNLOAD)
 # Only show the download message if it isn't already downloaded
 	$(Q)for p in $($(PKG)_SOURCE) $($(PKG)_PATCH) $($(PKG)_EXTRA_DOWNLOADS) ; do \
 		if test ! -e $(DL_DIR)/`basename $$p` ; then \

@@ -79,7 +78,6 @@ ifeq ($(DL_MODE),DOWNLOAD)
 			break ; \
 		fi ; \
 	done
-endif
 	$(if $($(PKG)_SOURCE),$(call DOWNLOAD,$($(PKG)_SITE:/=)/$($(PKG)_SOURCE)))
 	$(foreach p,$($(PKG)_EXTRA_DOWNLOADS),\
 		$(if $(findstring ://,$(p)),\

@@ -94,10 +92,8 @@ endif
 		)\
 	$(sep))
 	$(foreach hook,$($(PKG)_POST_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
-ifeq ($(DL_MODE),DOWNLOAD)
 	$(Q)mkdir -p $(@D)
 	$(Q)touch $@
-endif
 
 # Unpack the archive
 $(BUILD_DIR)/%/.stamp_extracted:

Signature · 1 line

2.1.0

The DL_MODE variable is now no longer used with any other value than "DOWNLOAD", so it no longer makes sense to have this variable at all. Therefore, this commit gets rid of it.

Signed-off-by Thomas Petazzoni <[email protected]>

Acked-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Tested-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

---
 package/pkg-download.mk | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

package/pkg-download.mk +1 -5

index 76afced..a145011 100644
@@ -20,10 +20,6 @@ export LOCALFILES := $(call qstrip,$(BR2_LOCALFILES))
 
 DL_WRAPPER = support/download/dl-wrapper
 
-# Default spider mode is 'DOWNLOAD'. Other possible value is
-# 'SOURCE_CHECK' used by the _source-check target.
-DL_MODE = DOWNLOAD
-
 # DL_DIR may have been set already from the environment
 ifeq ($(origin DL_DIR),undefined)
 DL_DIR ?= $(call qstrip,$(BR2_DL_DIR))

@@ -212,7 +208,7 @@ endef
 ################################################################################
 
 define DOWNLOAD
-	$(call DOWNLOAD_INNER,$(1),$(notdir $(1)),$(DL_MODE))
+	$(call DOWNLOAD_INNER,$(1),$(notdir $(1)),DOWNLOAD)
 endef
 
 define SOURCE_CHECK

Signature · 1 line

2.1.0

Some of the SOURCE_CHECK_* macros are using a non-standard two-spaces indentation. This commit switches them to use a single tab based indentation, like in the rest of Buildroot.

Signed-off-by Thomas Petazzoni <[email protected]>

Acked-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Tested-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

[with git show -w]

Signed-off-by Thomas Petazzoni <[email protected]>

---
 package/pkg-download.mk | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

package/pkg-download.mk +5 -5

index a145011..a38062e 100644
@@ -90,7 +90,7 @@ endef
 # TODO: improve to check that the given PKG_DL_VERSION exists on the remote
 # repository
 define SOURCE_CHECK_GIT
-  $(GIT) ls-remote --heads $($(PKG)_SITE) > /dev/null
+	$(GIT) ls-remote --heads $($(PKG)_SITE) > /dev/null
 endef
 
 define DOWNLOAD_BZR

@@ -134,7 +134,7 @@ define DOWNLOAD_SVN
 endef
 
 define SOURCE_CHECK_SVN
-  $(SVN) ls $($(PKG)_SITE)@$($(PKG)_DL_VERSION) > /dev/null
+	$(SVN) ls $($(PKG)_SITE)@$($(PKG)_DL_VERSION) > /dev/null
 endef
 
 # SCP URIs should be of the form scp://[user@]host:filepath

@@ -166,7 +166,7 @@ endef
 # TODO: improve to check that the given PKG_DL_VERSION exists on the remote
 # repository
 define SOURCE_CHECK_HG
-  $(HG) incoming --force -l1 $($(PKG)_SITE) > /dev/null
+	$(HG) incoming --force -l1 $($(PKG)_SITE) > /dev/null
 endef
 
 define DOWNLOAD_WGET

@@ -179,7 +179,7 @@ define DOWNLOAD_WGET
 endef
 
 define SOURCE_CHECK_WGET
-  $(WGET) --spider '$(call qstrip,$(1))'
+	$(WGET) --spider '$(call qstrip,$(1))'
 endef
 
 define DOWNLOAD_LOCALFILES

@@ -192,7 +192,7 @@ define DOWNLOAD_LOCALFILES
 endef
 
 define SOURCE_CHECK_LOCALFILES
-  test -e $(call stripurischeme,$(call qstrip,$(1)))
+	test -e $(call stripurischeme,$(call qstrip,$(1)))
 endef
 
 ################################################################################

Signature · 1 line

2.1.0

Now, both the download and source-check code are iterating over <pkg>_SOURCE, <pkg>_PATCH and <pkg>_EXTRA_DOWNLOADS elements, figuring out whether they contain full URLs or not. Instead of doing this repeatdly, this patch introduces an internal <pkg>_ALL_DOWNLOADS variable, which contains the list of everything that needs to be downloaded, with URLs already expanded to take into account <pkg>_SITE if needed.

This allows to simplify quite significantly the .stamp_download and source-check implementation.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 package/pkg-generic.mk | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

package/pkg-generic.mk +8 -18

index cb901ae..517c02f 100644
@@ -72,25 +72,13 @@ endif
 $(BUILD_DIR)/%/.stamp_downloaded:
 	$(foreach hook,$($(PKG)_PRE_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
 # Only show the download message if it isn't already downloaded
-	$(Q)for p in $($(PKG)_SOURCE) $($(PKG)_PATCH) $($(PKG)_EXTRA_DOWNLOADS) ; do \
+	$(Q)for p in $($(PKG)_ALL_DOWNLOADS); do \
 		if test ! -e $(DL_DIR)/`basename $$p` ; then \
 			$(call MESSAGE,"Downloading") ; \
 			break ; \
 		fi ; \
 	done
-	$(if $($(PKG)_SOURCE),$(call DOWNLOAD,$($(PKG)_SITE:/=)/$($(PKG)_SOURCE)))
-	$(foreach p,$($(PKG)_EXTRA_DOWNLOADS),\
-		$(if $(findstring ://,$(p)),\
-			$(call DOWNLOAD,$(p)),\
-			$(call DOWNLOAD,$($(PKG)_SITE:/=)/$(p))\
-		)\
-	$(sep))
-	$(foreach p,$($(PKG)_PATCH),\
-		$(if $(findstring ://,$(p)),\
-			$(call DOWNLOAD,$(p)),\
-			$(call DOWNLOAD,$($(PKG)_SITE:/=)/$(p))\
-		)\
-	$(sep))
+	$(foreach p,$($(PKG)_ALL_DOWNLOADS),$(call DOWNLOAD,$(p))$(sep))
 	$(foreach hook,$($(PKG)_POST_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
 	$(Q)mkdir -p $(@D)
 	$(Q)touch $@

@@ -355,6 +343,11 @@ ifndef $(2)_PATCH
  endif
 endif
 
+$(2)_ALL_DOWNLOADS = \
+	$$(foreach p,$$($(2)_SOURCE) $$($(2)_PATCH) $$($(2)_EXTRA_DOWNLOADS),\
+		$$(if $$(findstring ://,$$(p)),$$(p),\
+			$$($(2)_SITE:/=)/$$(p)))
+
 ifndef $(2)_SITE
  ifdef $(3)_SITE
   $(2)_SITE = $$($(3)_SITE)

@@ -568,10 +561,7 @@ $(1)-show-version:
 
 $(1)-source-check:
 ifeq ($$($(2)_OVERRIDE_SRCDIR),)
-	$$(foreach p,$$($(2)_SOURCE) $$($(2)_EXTRA_DOWNLOADS) $$($(2)_PATCH),\
-		$$(if $$(findstring ://,$$(p)),\
-			$$(call SOURCE_CHECK,$$(p)),\
-			$$(call SOURCE_CHECK,$$($(2)_SITE:/=)/$$(p)))$$(sep))
+	$$(foreach p,$$($(2)_ALL_DOWNLOADS),$$(call SOURCE_CHECK,$$(p))$$(sep))
 else
 	test -d $$($(2)_OVERRIDE_SRCDIR)
 endif

Signature · 1 line

2.1.0

Now that all the bits are in place, switch the global 'source' target to use the package infrastructure logic.

Signed-off-by Thomas Petazzoni <[email protected]>

Acked-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Tested-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

[with 'make source' (actually together with the next patch).]

Signed-off-by Thomas Petazzoni <[email protected]>

---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Makefile +1 -1

index f5ff574..e8a694b 100644
@@ -612,7 +612,7 @@ target-post-image: $(TARGETS_ROOTFS) target-finalize
 		$(call MESSAGE,"Executing post-image script $(s)"); \
 		$(EXTRA_ENV) $(s) $(BINARIES_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep))
 
-source: $(PACKAGES_SOURCE) $(HOST_SOURCE)
+source: $(foreach p,$(PACKAGES),$(p)-all-source)
 
 _external-deps: $(foreach p,$(PACKAGES),$(p)-all-external-deps)
 external-deps:

Signature · 1 line

2.1.0

Now that all the external-deps, source-check and source targets are properly implemented based on the package infrastructure, the PACKAGES_SOURCE, TARGET_HOST_DEPS, HOST_DEPS and HOST_SOURCE variables are no longer needed. This is a good thing since they were anyway incorrect, as they were only doing a two level recursion in the dependencies of host packages.

Signed-off-by Thomas Petazzoni <[email protected]>

Acked-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Tested-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

---
 Makefile | 21 ---------------------
 1 file changed, 21 deletions(-)

Makefile +0 -21

index e8a694b..0ba341b 100644
@@ -389,27 +389,6 @@ include fs/common.mk
 
 include $(BR2_EXTERNAL)/external.mk
 
-PACKAGES_SOURCE := $(patsubst %,%-source,$(PACKAGES))
-
-# host-* dependencies have to be handled specially, as those aren't
-# visible in Kconfig and hence not added to a variable like PACKAGES.
-# instead, find all the host-* targets listed in each <PKG>_DEPENDENCIES
-# variable for each enabled target.
-# Notice: this only works for newstyle gentargets/autotargets packages
-TARGETS_HOST_DEPS = $(sort $(filter host-%,$(foreach dep,\
-		$(addsuffix _DEPENDENCIES,\
-			$(call UPPERCASE,$(PACKAGES) $(TARGETS_ROOTFS))),\
-		$($(dep)))))
-# Host packages can in turn have their own dependencies. Likewise find
-# all the package names listed in the HOST_<PKG>_DEPENDENCIES for each
-# host package found above. Ideally this should be done recursively until
-# no more packages are found, but that's hard to do in make, so limit to
-# 1 level for now.
-HOST_DEPS = $(sort $(foreach dep,\
-		$(addsuffix _DEPENDENCIES,$(call UPPERCASE,$(TARGETS_HOST_DEPS))),\
-		$($(dep))))
-HOST_SOURCE += $(addsuffix -source,$(sort $(TARGETS_HOST_DEPS) $(HOST_DEPS)))
-
 dirs: $(BUILD_DIR) $(STAGING_DIR) $(TARGET_DIR) \
 	$(HOST_DIR) $(BINARIES_DIR)
 

Signature · 1 line

2.1.0

Some packages do some sanity checks on their configuration, for example linux checks that the defconfig string is not empty when a defconfig is used. Such checks are currently always performed, except when the 'source' target is part of make goals.

This is problematic for two reasons:

- Other targets such as 'source-check', 'external-deps' or 'legal-info', that do not consist in doing a build, cannot be executed in such situations.

- The current code removes the check as soon as one of the targets is source. But if there are other non-source targets called at the same time, the checks are ignored.

This commit therefore introduces an internal variable called BR_BUILDING, which tells packages if we are actually building or not. A variable nobuild_targets indicates the targets that we do not consider as being build targets.

For the moment, nobuild_targets only contains 'source', to be completely iso-functional.

Signed-off-by Thomas Petazzoni <[email protected]>

---
 Makefile | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Makefile +16 -0

index 0ba341b..e0be636 100644
@@ -77,6 +77,22 @@ noconfig_targets := menuconfig nconfig gconfig xconfig config oldconfig randconf
 	randpackageconfig allyespackageconfig allnopackageconfig \
 	print-version olddefconfig
 
+# Some global targets do not trigger a build, but are used to collect
+# metadata, or do various checks. When such targets are triggered,
+# some packages should not do their configuration sanity
+# checks. Provide them a BR_BUILDING variable set to 'y' when we're
+# actually building and they should do their sanity checks.
+#
+# We're building in two situations: when MAKECMDGOALS is empty
+# (default target is to build), or when MAKECMDGOALS contains
+# something else than one of the nobuild_targets.
+nobuild_targets := source
+ifeq ($(MAKECMDGOALS),)
+BR_BUILDING = y
+else ifneq ($(filter-out $(nobuild_targets),$(MAKECMDGOALS)),)
+BR_BUILDING = y
+endif
+
 # Strip quotes and then whitespaces
 qstrip = $(strip $(subst ",,$(1)))
 #"))

Signature · 1 line

2.1.0

On 2015-04-25 16:43 +0200, Thomas Petazzoni spake thusly:

Quoted 20 lines

Some packages do some sanity checks on their configuration, for example linux checks that the defconfig string is not empty when a defconfig is used. Such checks are currently always performed, except when the 'source' target is part of make goals.

This is problematic for two reasons:

- Other targets such as 'source-check', 'external-deps' or 'legal-info', that do not consist in doing a build, cannot be executed in such situations.

- The current code removes the check as soon as one of the targets is source. But if there are other non-source targets called at the same time, the checks are ignored.

This commit therefore introduces an internal variable called BR_BUILDING, which tells packages if we are actually building or not. A variable nobuild_targets indicates the targets that we do not consider as being build targets.

Quoted 4 lines

For the moment, nobuild_targets only contains 'source', to be completely iso-functional.

Signed-off-by Thomas Petazzoni <[email protected]>

I'm not too fond of the variable name, but I can't find a better one...

Acked-by "Yann E. MORIN" <[email protected]>

I maintian my ack, but I have a deeper concern about not doing those checks in the first place.

So, what does it mean not to have a config/board/... name to start with?

It means the specific configuration is not complete, and that the package will fail to configure or build. So it does not make sense to accept a configuration where such config name are missing.

Furthermore, what about a package that needs the config name to decide what tarball to download? In that case, we can't do the check at all, and we must fail as soon as the config name is empty.

As such, I'd prefer we do not condition the check on whether we'r ebuilding or not, but that we do it always, fo all packages.

However, I can see one reason where we would still want the checks to be
conditional: make rand*config && make source-check . And that is a tricky case to solve.

We could do it by always defining defaults (in the Config.in) to such config names; those defaults could be conditional, of course. But coming up with sane defaults is not easy either...

Thoughts?

Quoted 24 lines

I maintian my ack, but I have a deeper concern about not doing those checks in the first place.

So, what does it mean not to have a config/board/... name to start with?

It means the specific configuration is not complete, and that the package will fail to configure or build. So it does not make sense to accept a configuration where such config name are missing.

Furthermore, what about a package that needs the config name to decide what tarball to download? In that case, we can't do the check at all, and we must fail as soon as the config name is empty.

As such, I'd prefer we do not condition the check on whether we'r ebuilding or not, but that we do it always, fo all packages.

However, I can see one reason where we would still want the checks to be
conditional: make rand*config && make source-check . And that is a tricky case to solve.

We could do it by always defining defaults (in the Config.in) to such config names; those defaults could be conditional, of course. But coming up with sane defaults is not easy either...

Quoted 1 line

Thoughts?

I'd answer that this is a deeper discussion, into which I'd prefer to not get into at this point. The patch series is trying to keep the existing behavior, and I'd like to not change that for now.

Once this patch series gets merged, I'll be happy to rediscuss why we have this special casing on 'make source' / 'make source-check' to disable the checks.

Instead of manually testing MAKECMDGOALS, use the newly introduced BR_BUILDING variable to know if we're building or not.

Signed-off-by Thomas Petazzoni <[email protected]>

boot/at91bootstrap/at91bootstrap.mk +1 -4

index 802e85e..f655058 100644
@@ -40,11 +40,8 @@ endef
 
 $(eval $(generic-package))
 
-ifeq ($(BR2_TARGET_AT91BOOTSTRAP),y)
-# we NEED a board name unless we're at make source
-ifeq ($(filter source,$(MAKECMDGOALS)),)
+ifeq ($(BR2_TARGET_AT91BOOTSTRAP)$(BR_BUILDING),yy)
 ifeq ($(AT91BOOTSTRAP_BOARD),)
 $(error No AT91Bootstrap board name set. Check your BR2_TARGET_AT91BOOTSTRAP_BOARD setting)
 endif
 endif
-endif

boot/at91bootstrap3/at91bootstrap3.mk +0 -0

index 3e3b78e..ff49e14 100644

On 2015-04-25 16:43 +0200, Thomas Petazzoni spake thusly:

Quoted 2 lines

Instead of manually testing MAKECMDGOALS, use the newly introduced BR_BUILDING variable to know if we're building or not.

boot/at91bootstrap3/at91bootstrap3.mk AT91BOOTSTRAP3_KCONFIG_OPTS = $(AT91BOOTSTRAP3_MAKE_OPTS) · line 47

Quoted 4 lines

# Checks to give errors that the user can understand

boot/at91bootstrap3/at91bootstrap3.mk +1 -1

-ifeq ($(filter source,$(MAKECMDGOALS)),)
+ifeq ($(BR_BUILDING),y)
 ifeq ($(BR2_TARGET_AT91BOOTSTRAP3_USE_DEFCONFIG),y)

I think it would be good to have a single way we do those checks.

For example, compare at91bootstrap above and at91bootstrap3 here. The former is doing the BR2_BUILDING check and package-enabled check in a single ifeq, while the latter does it with two ifeq.

I don't really care which we use, but we should use the same everywhere. Maybe we could favour doing it with feq, for those packages that want to do multiple checks.

boot/at91bootstrap3/at91bootstrap3.mk AT91BOOTSTRAP3_KCONFIG_OPTS = $(AT91BOOTSTRAP3_MAKE_OPTS) · line 47

Quoted 4 lines

# Checks to give errors that the user can understand

boot/at91bootstrap3/at91bootstrap3.mk +1 -1

-ifeq ($(filter source,$(MAKECMDGOALS)),)
+ifeq ($(BR_BUILDING),y)
 ifeq ($(BR2_TARGET_AT91BOOTSTRAP3_USE_DEFCONFIG),y)

I think it would be good to have a single way we do those checks.

For example, compare at91bootstrap above and at91bootstrap3 here. The former is doing the BR2_BUILDING check and package-enabled check in a single ifeq, while the latter does it with two ifeq.

I don't really care which we use, but we should use the same everywhere. Maybe we could favour doing it with feq, for those packages that want to do multiple checks.

Applicable to the other packages, of course.

boot/xloader/xloader.mk endef · line 28

Quoted 4 lines

boot/xloader/xloader.mk +1 -3

-ifeq ($(BR2_TARGET_XLOADER),y)
-# we NEED a board name unless we're at make source
-ifeq ($(filter source,$(MAKECMDGOALS)),)
+ifeq ($(BR2_TARGET_XLOADER)$(BR_BUILDING),y)

This is broken: it should be compared to 'yy', not a single 'y'.

Quoted 12 lines

boot/at91bootstrap3/at91bootstrap3.mk AT91BOOTSTRAP3_KCONFIG_OPTS = $(AT91BOOTSTRAP3_MAKE_OPTS) · line 47

Quoted 5 lines

# Checks to give errors that the user can understand

boot/at91bootstrap3/at91bootstrap3.mk +1 -1

-ifeq ($(filter source,$(MAKECMDGOALS)),)
+ifeq ($(BR_BUILDING),y)
 ifeq ($(BR2_TARGET_AT91BOOTSTRAP3_USE_DEFCONFIG),y)

I think it would be good to have a single way we do those checks.

For example, compare at91bootstrap above and at91bootstrap3 here. The former is doing the BR2_BUILDING check and package-enabled check in a single ifeq, while the latter does it with two ifeq.

Quoted 5 lines

I don't really care which we use, but we should use the same everywhere. Maybe we could favour doing it with feq, for those packages that want to do multiple checks.

Applicable to the other packages, of course.

I don't quite agree. For at91bootstrap, there is only one check being done, while for at91bootstrap3, two checks are being done.

So basically, the current code is:

ifeq ($(BR_BUILDING),y)
ifeq ($(BR2_TARGET_AT91BOOTSTRAP3_USE_DEFCONFIG),y)
ifeq ($(call qstrip,$(BR2_TARGET_AT91BOOTSTRAP3_DEFCONFIG)),)
$(error No at91bootstrap3 defconfig name specified, check your BR2_TARGET_AT91BOOTSTRAP3_DEFCONFIG setting) endif
endif

ifeq ($(BR2_TARGET_AT91BOOTSTRAP3_USE_CUSTOM_CONFIG),y)
ifeq ($(call qstrip,$(BR2_TARGET_AT91BOOTSTRAP3_CUSTOM_CONFIG_FILE)),)
$(error No at91bootstrap3 configuration file specified, check your BR2_TARGET_AT91BOOTSTRAP3_CUSTOM_CONFIG_FILE setting) endif
endif
endif

While you're proposing to unfactorize the BR_BUILDING test:

This is a new iteration of the "Package based 'source', 'legal-info', 'source-check' and 'external-deps'" series.