pkg-download: add optional destination filename

This change prepends the package name to all downloaded patch files to avoid name collisions between patches in different packages.

Specifically, it fixes an issue where the official kernel and u-boot patches on the Intel Edison were named upstream_to_edison.patch.

Signed-off-by Frank Hunleth <[email protected]>

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

package/pkg-generic.mk +10 -4

index d1a1811..f63f553 100644
@@ -73,12 +73,18 @@ $(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 \
+	$(Q)for p in $($(PKG)_SOURCE) $($(PKG)_EXTRA_DOWNLOADS) ; do \
 		if test ! -e $(DL_DIR)/`basename $$p` ; then \
 			$(call MESSAGE,"Downloading") ; \
 			break ; \
 		fi ; \
 	done
+	$(Q)for p in $($(PKG)_PATCH) ; do \
+		if test ! -e $(DL_DIR)/$(RAWNAME)-`basename $$p` ; then \
+			$(call MESSAGE,"Downloading") ; \
+			break ; \
+		fi ; \
+	done
 endif
 	$(if $($(PKG)_SOURCE),$(call DOWNLOAD,$($(PKG)_SITE:/=)/$($(PKG)_SOURCE)))
 	$(foreach p,$($(PKG)_EXTRA_DOWNLOADS),\

@@ -89,8 +95,8 @@ endif
 	$(sep))
 	$(foreach p,$($(PKG)_PATCH),\
 		$(if $(findstring ://,$(p)),\
-			$(call DOWNLOAD,$(p)),\
-			$(call DOWNLOAD,$($(PKG)_SITE:/=)/$(p))\
+			$(call DOWNLOAD,$(p),$(RAWNAME)-$(notdir $(p))),\
+			$(call DOWNLOAD,$($(PKG)_SITE:/=)/$(p),$(RAWNAME)-$(notdir $(p)))\
 		)\
 	$(sep))
 	$(foreach hook,$($(PKG)_POST_DOWNLOAD_HOOKS),$(call $(hook))$(sep))

@@ -147,7 +153,7 @@ $(BUILD_DIR)/%/.stamp_patched:
 	@$(call step_start,patch)
 	@$(call MESSAGE,"Patching")
 	$(foreach hook,$($(PKG)_PRE_PATCH_HOOKS),$(call $(hook))$(sep))
-	$(foreach p,$($(PKG)_PATCH),$(APPLY_PATCHES) $(@D) $(DL_DIR) $(notdir $(p))$(sep))
+	$(foreach p,$($(PKG)_PATCH),$(APPLY_PATCHES) $(@D) $(DL_DIR) $(RAWNAME)-$(notdir $(p))$(sep))
 	$(Q)( \
 	for D in $(PATCH_BASE_DIRS); do \
 	  if test -d $${D}; then \

Quoted 7 lines · Frank Hunleth

This change prepends the package name to all downloaded patch files to avoid name collisions between patches in different packages.

Specifically, it fixes an issue where the official kernel and u-boot patches on the Intel Edison were named upstream_to_edison.patch.

Signed-off-by Frank Hunleth <[email protected]>

Actually, the same issue also sometimes exists for _SOURCE and
_EXTRA_DOWNLOADS. So we should add the package prefix to all of them.

Also, a few packages (e.g. cache-calibrator, nanocom) don't have a version in
their download filename, so that should ideally be added as well.

Ideally, the package and version parts should only be added if they're not
already present.

So I really would like to have an automatic calculation of the downloaded
filename. For instance, adding a function to calculate it:

download-filename = $(DL_DIR)/$(if $(findstring
$(pkgname),$(1)),,$(pkgname)-)$(if $(findstring
$($(PKG)_VERSION),$(1)),,$($(PKG)_VERSION)-)$(1)

And then, wherever a downloaded file is used:

$(call download-filename,...)

Regards,
Arnout

Quoted 41 lines

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

package/pkg-generic.mk +9 -3

index d1a1811..f63f553 100644
@@ -73,12 +73,18 @@ $(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 \
+     $(Q)for p in $($(PKG)_SOURCE) $($(PKG)_EXTRA_DOWNLOADS) ; do \
              if test ! -e $(DL_DIR)/`basename $$p` ; then \
                      $(call MESSAGE,"Downloading") ; \
                      break ; \
              fi ; \
      done
+     $(Q)for p in $($(PKG)_PATCH) ; do \
+             if test ! -e $(DL_DIR)/$(RAWNAME)-`basename $$p` ; then \
+                     $(call MESSAGE,"Downloading") ; \
+                     break ; \
+             fi ; \
+     done
 endif
      $(if $($(PKG)_SOURCE),$(call DOWNLOAD,$($(PKG)_SITE:/=)/$($(PKG)_SOURCE)))
      $(foreach p,$($(PKG)_EXTRA_DOWNLOADS),\

@@ -89,8 +95,8 @@ endif
      $(sep))
      $(foreach p,$($(PKG)_PATCH),\
              $(if $(findstring ://,$(p)),\
-                     $(call DOWNLOAD,$(p)),\
-                     $(call DOWNLOAD,$($(PKG)_SITE:/=)/$(p))\
+                     $(call DOWNLOAD,$(p),$(RAWNAME)-$(notdir $(p))),\
+                     $(call DOWNLOAD,$($(PKG)_SITE:/=)/$(p),$(RAWNAME)-$(notdir $(p)))\
              )\
      $(sep))
      $(foreach hook,$($(PKG)_POST_DOWNLOAD_HOOKS),$(call $(hook))$(sep))

package/pkg-generic.mk $(BUILD_DIR)/%/.stamp_patched: · line 159

Quoted 8 lines

package/pkg-generic.mk +1 -1

@@ -147,7 +153,7 @@ $(BUILD_DIR)/%/.stamp_patched:
      @$(call step_start,patch)
      @$(call MESSAGE,"Patching")
      $(foreach hook,$($(PKG)_PRE_PATCH_HOOKS),$(call $(hook))$(sep))
-     $(foreach p,$($(PKG)_PATCH),$(APPLY_PATCHES) $(@D) $(DL_DIR) $(notdir $(p))$(sep))
+     $(foreach p,$($(PKG)_PATCH),$(APPLY_PATCHES) $(@D) $(DL_DIR) $(RAWNAME)-$(notdir $(p))$(sep))
      $(Q)( \
      for D in $(PATCH_BASE_DIRS); do \
        if test -d $${D}; then \

--

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
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

I don't like the idea of penalizing _SOURCE and _EXTRA_DOWNLOADS with the ugly filenames from _PATCH, so I'm currently in favor of having a different strategies. (I've never used _EXTRA_DOWNLOADS, so maybe it's more like _PATCH?)

Dear Frank Hunleth,

Quoted 4 lines · Frank Hunleth

I don't like the idea of penalizing _SOURCE and _EXTRA_DOWNLOADS with the ugly filenames from _PATCH, so I'm currently in favor of having a different strategies. (I've never used _EXTRA_DOWNLOADS, so maybe it's more like _PATCH?)

Thinking out loud. We're trying to solve the problem of two packages downloading files having the same name, and we're struggling to find a solution by renaming files.

What if we took a different approach, and instead of renaming files, keep the file named as they are, but store them in subdirectories, named per-package. I.e, instead of throwing everything in $(DL_DIR), we create $(DL_DIR)/<package>/ for each package, and we store the downloads only for this package there. This way, two different packages can download files named identically, they will simply be stored in different directories.

There are obviously some implications (organization of files in BR2_PRIMARY_SITE and on sources.buildroot.net), but isn't this the easiest solution, after all? Or am I missing something?

Quoted 3 lines

There are obviously some implications (organization of files in BR2_PRIMARY_SITE and on sources.buildroot.net), but isn't this the easiest solution, after all? Or am I missing something?

Yes. This would be another route barring the new issue that I just ran into with _PATCH.

The issue occurs when Intel updates their kernel or u-boot patches without updating which kernel or u-boot that they use. The remote file name of the patches is still the same (upstream_to_edison.patch). The kernel or u-boot package versions are also still the same. So unless I append some additional versioning information to the filename stored in $(DL_DIR), the new patch won't be downloaded.

Dear Frank Hunleth,

Quoted 7 lines · Frank Hunleth

Quoted 3 lines

There are obviously some implications (organization of files in BR2_PRIMARY_SITE and on sources.buildroot.net), but isn't this the easiest solution, after all? Or am I missing something?

Yes. This would be another route barring the new issue that I just ran into with _PATCH.

Quoted 6 lines

The issue occurs when Intel updates their kernel or u-boot patches without updating which kernel or u-boot that they use. The remote file name of the patches is still the same (upstream_to_edison.patch). The kernel or u-boot package versions are also still the same. So unless I append some additional versioning information to the filename stored in $(DL_DIR), the new patch won't be downloaded.

Indeed my proposal would not solve this problem.

Alternate options:

1/ Fix upstream. Talk to the Intel people doing this sort of awful
mess to fix their workflow and rename patches with appropriate versions.

2/ Have a way of passing the expected hash for custom patches to
download, so that the Buildroot download infra can notice that the hashes are not correct anymore, and will redownload.

Maybe:

BR2_TARGET_UBOOT_PATCH="http://intel.com/stupid.patch{md5:2394fc9ba9094f8e6d0a59490a1fa65e}"

?

Best regards,

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

This adds an optional parameter to the DOWNLOAD macro so that it's possible to specify a destination filename that's different from one being downloaded.

Signed-off-by Frank Hunleth <[email protected]>

---
 package/pkg-download.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

package/pkg-download.mk +2 -1

index e274712..1278018 100644
@@ -244,13 +244,14 @@ endef
 # 3) BR2_BACKUP_SITE if enabled, unless BR2_PRIMARY_SITE_ONLY is set
 #
 # Argument 1 is the source location
+# Argument 2 is an optional destination filename
 #
 # E.G. use like this:
 # $(call DOWNLOAD,$(FOO_SITE))
 ################################################################################
 
 define DOWNLOAD
-	$(call DOWNLOAD_INNER,$(1),$(notdir $(1)))
+	$(call DOWNLOAD_INNER,$(1),$(if $(2),$(2),$(notdir $(1))))
 endef
 
 define DOWNLOAD_INNER

This adds an optional parameter to the DOWNLOAD macro so that it's possible to specify a destination filename that's different from one being downloaded.

Signed-off-by Frank Hunleth <[email protected]>

---
 package/pkg-download.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Dear Frank Hunleth,

Quoted 8 lines · Frank Hunleth

This adds an optional parameter to the DOWNLOAD macro so that it's possible to specify a destination filename that's different from one being downloaded.

Signed-off-by Frank Hunleth <[email protected]>

---
 package/pkg-download.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

This patch is still pending in patchwork, but the discussion in PATCH 2/2 didn't really reached a conclusion other than the fact that you had a very specific case of a patch being updated by upstream without being renamed, and that you were going to talk with upstream about this. We haven't heard from you about this specific problem since end of April, and anyway the proposed implementation wasn't matching our requirements.

So I've marked this patch as Rejected in patchwork. Of course, if you still have the issue, let's reopen the discussion and work on a proper solution for the problem.