On Wednesday 15 February 2012 22:05:38 Arnout Vandecappelle (Essensium/Mind) wrote:
> From: "Alvaro G. M" <alvaro.gamez at hazent.com>
>
> This patch modifies the definition of DOWNLOAD to receive two arguments.
>
> I think this is the generic solution to the problem, even though it is a
> change to the API.
>
> However, by making the second parameter optional, it could be possible to reduce
> code. To keep the more generic way, the .mk files that needed to be changed
> to the new API are kept with both arguments to the call of the function.
It would indeed be a good idea to make the second parameter optional. And
in that case, I would leave out the second parameter whenever possible.
> Same thing with the, to my knowledge, unused SOURCE_CHECK_WGET and SCP functions.
It is used for 'make source-check'.
> The difference is that now $1 stores the full URL of the file, whereas $2
> stores the name of the file as we want it to be named after download
> complets.
^completes
> For example, a full copy-paste from Xilinx git:
> URL='http://git.xilinx.com/?p=xldk/microblaze_v2.0_le.git;a=blob;f=microblazeel-unknown-linux-gnu.tgz;h=d7b493c5dbcc24ba9cc3be2e4c14d6d9701e6805;hb=HEAD'
> FILE='microblazeel-unknown-linux-gnu.tgz'
>
> $(call DOWNLOAD,$(URL),$(FILE))
>
> In case we know for sure the downloaded file is going to have the name we
> want, we can omit the second parameter:
>
> URL=ftp://ftp.idsoftware.com/idstuff/doom/doom-$(DOOM_WAD_VERSION).wad.gz
> $(call DOWNLOAD,$(URL))
Sounds like a great plan!
[snip]
> diff --git a/docs/buildroot.html b/docs/buildroot.html
> index ddfb20a..e0f6ad8 100644
> --- a/docs/buildroot.html
> +++ b/docs/buildroot.html
The manually maintained buildroot.html is deprecated and should be
removed Real Soon Now (Peter?).
> --- a/docs/manual/adding-packages-handwritten.txt
> +++ b/docs/manual/adding-packages-handwritten.txt
> @@ -22,7 +22,7 @@ existing manual makefiles and to help understand how they work.*
> 11: LIBFOO_TARGET_BINARY:=usr/bin/foo
> 12:
> 13: $(DL_DIR)/$(LIBFOO_SOURCE):
> -14: $(call DOWNLOAD,$(LIBFOO_SITE),$(LIBFOO_SOURCE))
> +14: $(call DOWNLOAD,$(LIBFOO_SITE)/$(LIBFOO_SOURCE),$(LIBFOO_SOURCE))
Leave out the second parameter in the example here.
> 15:
> 16: $(LIBFOO_DIR)/.source: $(DL_DIR)/$(LIBFOO_SOURCE)
> 17: $(ZCAT) $(DL_DIR)/$(LIBFOO_SOURCE) | tar -C $(BUILD_DIR) $(TAR_OPTIONS) -
> diff --git a/linux/linux.mk b/linux/linux.mk
> index ae236d4..d1a05a1 100644
> --- a/linux/linux.mk
> +++ b/linux/linux.mk
> @@ -98,7 +98,7 @@ define LINUX_DOWNLOAD_PATCHES
> $(if $(LINUX_PATCHES),
> @$(call MESSAGE,"Download additional patches"))
> $(foreach patch,$(filter ftp://% http://%,$(LINUX_PATCHES)),\
> - $(call DOWNLOAD,$(dir $(patch)),$(notdir $(patch)))$(sep))
> + $(call DOWNLOAD,$(patch),$(notdir $(patch)))$(sep))
So this could become
$(call DOWNLOAD,$(patch))$(sep))
right? That's a
tremendous improvement!
> endef
>
> LINUX_POST_DOWNLOAD_HOOKS += LINUX_DOWNLOAD_PATCHES
> diff --git a/package/Makefile.package.in b/package/Makefile.package.in
> index 33461b4..3714de0 100644
> --- a/package/Makefile.package.in
> +++ b/package/Makefile.package.in
> @@ -203,11 +203,11 @@ endef
> # to prepend the path with a slash: scp://[user@]host:/absolutepath
> define DOWNLOAD_SCP
> test -e $(DL_DIR)/$(2) || \
> - $(SCP) $(call stripurischeme,$(call qstrip,$(1)))/$(2) $(DL_DIR)
> + $(SCP) $(call stripurischeme,$(call qstrip,$(1))) $(DL_DIR)/$(2)
While you're at it, maybe you can add quotes, i.e.
$(SCP) '$(call stripurischeme,$(call qstrip,$(1)))' $(DL_DIR)/$(2)
Note that the second parameter doesn't have to be quoted - we do this
precisely to make sure it's a safe file.
> endef
>
> define SOURCE_CHECK_SCP
> - $(SSH) $(call domain,$(1),:) ls $(call notdomain,$(1)/$(2),:) > /dev/null
> + $(SSH) $(call domain,$(1),:) ls $(call notdomain,$(1),:) > /dev/null
> endef
>
> define SHOW_EXTERNAL_DEPS_SCP
> @@ -237,12 +237,16 @@ endef
>
>
> define DOWNLOAD_WGET
> - test -e $(DL_DIR)/$(2) || \
> - $(WGET) -P $(DL_DIR) $(call qstrip,$(1))/$(2)
> + if test -n "$(2)"; then \
> + test -e $(DL_DIR)/$(2) || \
> + $(WGET) -O $(DL_DIR)/$(2) $(call qstrip,$(1)); \
> + else \
> + $(WGET) $(call qstrip,$(1)) ;\
> + fi
The test should be there also when defaulting $(2). So, to make
life easier, the default should be set in the DOWNLOAD macro itself.
Also, the call to the DOWNLOAD macro in the GENTARGETS wasn't updated.
You should replace the , with a / there.
> endef
>
> define SOURCE_CHECK_WGET
> - $(WGET) --spider $(call qstrip,$(1))/$(2)
> + $(WGET) --spider $(call qstrip,$(1))
> endef
>
> define SHOW_EXTERNAL_DEPS_WGET
> diff --git a/package/cups/cups.mk b/package/cups/cups.mk
> index 4e8db71..ed2e72c 100644
> --- a/package/cups/cups.mk
> +++ b/package/cups/cups.mk
> @@ -65,7 +65,7 @@ else
> endif
>
> $(DL_DIR)/$(CUPS_SOURCE):
> - $(call DOWNLOAD,$(CUPS_SITE),$(CUPS_SOURCE))
> + $(call DOWNLOAD,$(CUPS_SITE)/$(CUPS_SOURCE),$(CUPS_SOURCE))
As I mentioned before: use the default, remove the second $(CUPS_SOURCE).
And the same for all other packages, of course.
[snip]
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