Patchwork [1,of,6,resend] pkg-download.mk: allow using localfiles outside of package infrastructure

login
register
mail settings
Submitter Thomas De Schampheleire
Date June 22, 2012, 5:42 a.m.
Message ID <1dcdff3e9437fc63cd4b.1340343754@beantl019720>
Download mbox | patch
Permalink /patch/166504/
State Accepted
Headers show

Comments

Thomas De Schampheleire - June 22, 2012, 5:42 a.m.
The localfiles download method uses $($(PKG)_SITE))) and
$($(PKG)_SOURCE) instead of $(1) and $(2). This means that it can only
be used for package downloads (through gentargets, autotargets, ...)
and not for other downloads like external toolchains.

This patch changes localfiles to allow this, just as the wget and scp
download methods already did.
For the version control download methods, nothing changes.

Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>

---
 package/pkg-download.mk |  8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
Arnout Vandecappelle - July 15, 2012, 11:24 a.m.
On 06/22/12 07:42, Thomas De Schampheleire wrote:
> The localfiles download method uses $($(PKG)_SITE))) and
> $($(PKG)_SOURCE) instead of $(1) and $(2). This means that it can only
> be used for package downloads (through gentargets, autotargets, ...)
> and not for other downloads like external toolchains.
>
> This patch changes localfiles to allow this, just as the wget and scp
> download methods already did.
> For the version control download methods, nothing changes.

  Is there any reason not to do it for the VCS download methods?

>
> Signed-off-by: Thomas De Schampheleire<thomas.de.schampheleire@gmail.com>
>
> ---
>   package/pkg-download.mk |  8 ++++----
>   1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/package/pkg-download.mk b/package/pkg-download.mk
> --- a/package/pkg-download.mk
> +++ b/package/pkg-download.mk
> @@ -174,16 +174,16 @@ define SHOW_EXTERNAL_DEPS_WGET
>   endef
>
>   define DOWNLOAD_LOCALFILES
> -	test -e $(DL_DIR)/$($(PKG)_SOURCE) || \
> -		$(LOCALFILES) $(call qstrip,$(subst file://,,$($(PKG)_SITE)))/$($(PKG)_SOURCE) $(DL_DIR)
> +	test -e $(DL_DIR)/$(2) || \
> +		$(LOCALFILES) $(call qstrip,$(subst file://,,$(1))) $(DL_DIR)

  Shouldn't this be
		$(LOCALFILES) $(call qstrip,$(subst file://,,$(1))) $(DL_DIR)/$(2)

>   endef
>
>   define SOURCE_CHECK_LOCALFILES
> -  test -e $(call qstrip,$(subst file://,,$($(PKG)_SITE)))/$($(PKG)_SOURCE)
> +  test -e $(call qstrip,$(subst file://,,$(1)))
>   endef
>
>   define SHOW_EXTERNAL_DEPS_LOCALFILES
> -  echo $($(PKG)_SITE)/$($(PKG)_SOURCE)
> +  echo $(2)

  Funny, the original was actually wrong...

  Regards,
  Arnout

>   endef
>
>   ################################################################################
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
>
Thomas De Schampheleire - July 16, 2012, 3:14 a.m.
Hi Arnout,

Thanks for your review and acks.

Op 15 jul. 2012 05:24 schreef "Arnout Vandecappelle" <arnout@mind.be> het
volgende:
>
> On 06/22/12 07:42, Thomas De Schampheleire wrote:
>>
>> The localfiles download method uses $($(PKG)_SITE))) and
>> $($(PKG)_SOURCE) instead of $(1) and $(2). This means that it can only
>> be used for package downloads (through gentargets, autotargets, ...)
>> and not for other downloads like external toolchains.
>>
>> This patch changes localfiles to allow this, just as the wget and scp
>> download methods already did.
>> For the version control download methods, nothing changes.
>
>
>  Is there any reason not to do it for the VCS download methods?
>

Just because I didn't really see how you'd use that. For non-package
downloads you'd be downloading a single file (typically a tarball). It
seems odd to me to take that from a version control system. I guess you'll
need to express the version you want in the URL, which may not be possible
for all the systems we support.
Thomas basically asked the same question. Do any of you do see a use case?

>
>>
>> Signed-off-by: Thomas De Schampheleire<thomas.de.schampheleire@gmail.com>
>>
>> ---
>>   package/pkg-download.mk |  8 ++++----
>>   1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/package/pkg-download.mk b/package/pkg-download.mk
>> --- a/package/pkg-download.mk
>> +++ b/package/pkg-download.mk
>> @@ -174,16 +174,16 @@ define SHOW_EXTERNAL_DEPS_WGET
>>   endef
>>
>>   define DOWNLOAD_LOCALFILES
>> -       test -e $(DL_DIR)/$($(PKG)_SOURCE) || \
>> -               $(LOCALFILES) $(call qstrip,$(subst
file://,,$($(PKG)_SITE)))/$($(PKG)_SOURCE) $(DL_DIR)
>> +       test -e $(DL_DIR)/$(2) || \
>> +               $(LOCALFILES) $(call qstrip,$(subst file://,,$(1)))
$(DL_DIR)
>
>
>  Shouldn't this be
>                 $(LOCALFILES) $(call qstrip,$(subst file://,,$(1)))
$(DL_DIR)/$(2)

Well, the behavior is the same as before. But I guess you could safely add
the explicit destination. I can't test it as I'm on holiday, though.

Maybe you could submit that as a separate patch?

>
>
>>   endef
>>
>>   define SOURCE_CHECK_LOCALFILES
>> -  test -e $(call qstrip,$(subst
file://,,$($(PKG)_SITE)))/$($(PKG)_SOURCE)
>> +  test -e $(call qstrip,$(subst file://,,$(1)))
>>   endef
>>
>>   define SHOW_EXTERNAL_DEPS_LOCALFILES
>> -  echo $($(PKG)_SITE)/$($(PKG)_SOURCE)
>> +  echo $(2)
>
>
>  Funny, the original was actually wrong...
>
>  Regards,
>  Arnout
>
>>   endef
>>

Best regards,
Thomas
Arnout Vandecappelle - July 17, 2012, 7:34 a.m.
On 07/16/12 05:14, Thomas De Schampheleire wrote:
> Hi Arnout,
>
> Thanks for your review and acks.

  Thanks for your feedback, this gets my
Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

>
> Op 15 jul. 2012 05:24 schreef "Arnout Vandecappelle" <arnout@mind.be <mailto:arnout@mind.be>> het volgende:
>>
>>  On 06/22/12 07:42, Thomas De Schampheleire wrote:
>> >
>> > The localfiles download method uses $($(PKG)_SITE))) and
>> > $($(PKG)_SOURCE) instead of $(1) and $(2). This means that it can only
>> > be used for package downloads (through gentargets, autotargets, ...)
>> > and not for other downloads like external toolchains.
>> >
>> > This patch changes localfiles to allow this, just as the wget and scp
>> > download methods already did.
>> > For the version control download methods, nothing changes.
>>
>>
>>   Is there any reason not to do it for the VCS download methods?
>>
>
> Just because I didn't really see how you'd use that. For non-package downloads you'd be downloading a single file
> (typically a tarball). It seems odd to me to take that from a version control system. I guess you'll need to express the
> version you want in the URL, which may not be possible for all the systems we support.
> Thomas basically asked the same question. Do any of you do see a use case?

  It's just for symmetry as far as I'm concerned.  It's true that the VCS
methods require a revision as well, so they'll also require the $(PKG) to
be used.

  Taking a tarball from a VCS is not even possible with the current VCS
download methods: they would pack the tarball into another tarball...

>
>>
>> >
>> > Signed-off-by: Thomas De Schampheleire<thomas.de.schampheleire@gmail.com <mailto:thomas.de.schampheleire@gmail.com>>
>> >
>> > ---
>> >   package/pkg-download.mk <http://pkg-download.mk> |  8 ++++----
>> >   1 files changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/package/pkg-download.mk <http://pkg-download.mk> b/package/pkg-download.mk <http://pkg-download.mk>
>> > --- a/package/pkg-download.mk <http://pkg-download.mk>
>> > +++ b/package/pkg-download.mk <http://pkg-download.mk>
>> > @@ -174,16 +174,16 @@ define SHOW_EXTERNAL_DEPS_WGET
>> >   endef
>> >
>> >   define DOWNLOAD_LOCALFILES
>> > -       test -e $(DL_DIR)/$($(PKG)_SOURCE) || \
>> > -               $(LOCALFILES) $(call qstrip,$(subst file://,,$($(PKG)_SITE)))/$($(PKG)_SOURCE) $(DL_DIR)
>> > +       test -e $(DL_DIR)/$(2) || \
>> > +               $(LOCALFILES) $(call qstrip,$(subst file://,,$(1))) $(DL_DIR)
>>
>>
>>   Shouldn't this be
>>                  $(LOCALFILES) $(call qstrip,$(subst file://,,$(1))) $(DL_DIR)/$(2)
>
> Well, the behavior is the same as before. But I guess you could safely add the explicit destination. I can't test it as
> I'm on holiday, though.
>
> Maybe you could submit that as a separate patch?

  OK.

[snip]

  Regards,
  Arnout
Thomas Petazzoni - July 22, 2012, 4:24 p.m.
Le Fri, 22 Jun 2012 07:42:34 +0200,
Thomas De Schampheleire <patrickdepinguin+buildroot@gmail.com> a écrit :

> The localfiles download method uses $($(PKG)_SITE))) and
> $($(PKG)_SOURCE) instead of $(1) and $(2). This means that it can only
> be used for package downloads (through gentargets, autotargets, ...)
> and not for other downloads like external toolchains.
> 
> This patch changes localfiles to allow this, just as the wget and scp
> download methods already did.
> For the version control download methods, nothing changes.
> 
> Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>

Applied all 6 patches, thanks!

Thomas

Patch

diff --git a/package/pkg-download.mk b/package/pkg-download.mk
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -174,16 +174,16 @@  define SHOW_EXTERNAL_DEPS_WGET
 endef
 
 define DOWNLOAD_LOCALFILES
-	test -e $(DL_DIR)/$($(PKG)_SOURCE) || \
-		$(LOCALFILES) $(call qstrip,$(subst file://,,$($(PKG)_SITE)))/$($(PKG)_SOURCE) $(DL_DIR)
+	test -e $(DL_DIR)/$(2) || \
+		$(LOCALFILES) $(call qstrip,$(subst file://,,$(1))) $(DL_DIR)
 endef
 
 define SOURCE_CHECK_LOCALFILES
-  test -e $(call qstrip,$(subst file://,,$($(PKG)_SITE)))/$($(PKG)_SOURCE)
+  test -e $(call qstrip,$(subst file://,,$(1)))
 endef
 
 define SHOW_EXTERNAL_DEPS_LOCALFILES
-  echo $($(PKG)_SITE)/$($(PKG)_SOURCE)
+  echo $(2)
 endef
 
 ################################################################################