pkg-download: make package name optional in github helper

Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
---
 package/pkg-download.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Hi Ismael,


On 04-10-17 10:57, Ismael Luceno wrote:
> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>
> ---
>  package/pkg-download.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/package/pkg-download.mk b/package/pkg-download.mk
> index dc4ff1c8c755..78307eee8a33 100644
> --- a/package/pkg-download.mk
> +++ b/package/pkg-download.mk
> @@ -52,8 +52,8 @@ notdomain = $(patsubst $(call domain,$(1),$(2))$(call domainseparator,$(2))%,%,$
>  # default domainseparator is /, specify alternative value as first argument
>  domainseparator = $(if $(1),$(1),/)
>  
> -# github(user,package[,version]): returns site of GitHub repository
> -github = https://github.com/$(1)/$(2)/archive/$(or $(3),$($(call UPPERCASE,$(pkgname))_VERSION))

 As noticed by Peter, this was already wrong in the original patch: it only
works if the github helper is used in a := assignment, otherwise $(pkgname)
evaluates to empty by the time the variable is evaluated.

 The solution (probably) is to use $(PKG) instead of $(pkgname).

> +# github(user[,package[,version]]): returns site of GitHub repository
> +github = https://github.com/$(1)/$(or $(2),$(pkgname))/archive/$(or $(3),$($(call UPPERCASE,$(pkgname))_VERSION))

 Could you estimate how many packages could be simplified with this?

 I'm not actually sure this is a good idea anyway. The _VERSION is OK because it
works for *all* packages, the third argument is only needed when you use the
github helper outside a package (e.g. for a custom kernel URL). But this change
will apply only to a limited number of packages on the one hand, and on the
other hand it makes it even more difficult for people to understand what is
meant with this $(call github,foo) construct.

 Regards,
 Arnout

>  
>  # Expressly do not check hashes for those files
>  # Exported variables default to immediately expanded in some versions of
>

On 04/Oct/2017 18:35, Arnout Vandecappelle wrote:
<...>
>  The solution (probably) is to use $(PKG) instead of $(pkgname).

I see.

> > +# github(user[,package[,version]]): returns site of GitHub repository
> > +github = https://github.com/$(1)/$(or $(2),$(pkgname))/archive/$(or $(3),$($(call UPPERCASE,$(pkgname))_VERSION))
> 
>  Could you estimate how many packages could be simplified with this?

A great deal, including axel, in 219 out of 380 the repo matches the
package name, with 30 also matching the username.

> I'm not actually sure this is a good idea anyway. The _VERSION is OK
> because it works for *all* packages, the third argument is only needed
> when you use the github helper outside a package (e.g. for a custom
> kernel URL). But this change will apply only to a limited number of
> packages on the one hand, and on the other hand it makes it even more
> difficult for people to understand what is meant with this $(call
> github,foo) construct.

It's "magic" anyway. It would perhaps make sense to implement this kind
of rules via rewriting AXEL_SITE, it could be something descriptive like:

	AXEL_SITE = @ github user:axel-download-accelerator repo:libtls

Here's a mockup: http://tmp.iodev.co.uk/rtest.mk

Hello,

On Wed,  4 Oct 2017 05:57:45 -0300, Ismael Luceno wrote:
> Signed-off-by: Ismael Luceno <ismael@iodev.co.uk>

An empty commit log is definitely not good for such a commit. Why do we
want this?

Thanks!

Thomas

Hello,

On Wed, 4 Oct 2017 18:35:07 +0200, Arnout Vandecappelle wrote:

>  As noticed by Peter, this was already wrong in the original patch: it only
> works if the github helper is used in a := assignment,

Doh? We're using the github helper almost exclusively in = assignments:

package/assimp/assimp.mk:ASSIMP_SITE = $(call github,assimp,assimp,$(ASSIMP_VERSION))
package/asterisk/asterisk.mk:ASTERISK_SITE = $(call github,asterisk,asterisk,$(ASTERISK_VERSION))
package/atest/atest.mk:ATEST_SITE = $(call github,amouiche,atest,$(ATEST_VERSION))
package/aufs/aufs.mk:AUFS_SITE = $(call github,sfjro,aufs4-standalone,$(AUFS_VERSION))
package/avrdude/avrdude.mk:AVRDUDE_SITE = $(call github,kcuzner,avrdude,$(AVRDUDE_VERSION))
package/axfsutils/axfsutils.mk:AXFSUTILS_SITE = $(call github,jaredeh,axfs,$(AXFSUTILS_VERSION))
package/azmq/azmq.mk:AZMQ_SITE = $(call github,zeromq,azmq,$(AZMQ_VERSION))

Am I missing something in your explanation ?

Or perhaps you meant "when the third argument is not passed, we need to
use a := assignment" ?

Best regards,

Thomas

On 05-10-17 12:23, Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 4 Oct 2017 18:35:07 +0200, Arnout Vandecappelle wrote:
> 
>>  As noticed by Peter, this was already wrong in the original patch: it only
>> works if the github helper is used in a := assignment,
> 
> Doh? We're using the github helper almost exclusively in = assignments:
> 
> package/assimp/assimp.mk:ASSIMP_SITE = $(call github,assimp,assimp,$(ASSIMP_VERSION))
> package/asterisk/asterisk.mk:ASTERISK_SITE = $(call github,asterisk,asterisk,$(ASTERISK_VERSION))
> package/atest/atest.mk:ATEST_SITE = $(call github,amouiche,atest,$(ATEST_VERSION))
> package/aufs/aufs.mk:AUFS_SITE = $(call github,sfjro,aufs4-standalone,$(AUFS_VERSION))
> package/avrdude/avrdude.mk:AVRDUDE_SITE = $(call github,kcuzner,avrdude,$(AVRDUDE_VERSION))
> package/axfsutils/axfsutils.mk:AXFSUTILS_SITE = $(call github,jaredeh,axfs,$(AXFSUTILS_VERSION))
> package/azmq/azmq.mk:AZMQ_SITE = $(call github,zeromq,azmq,$(AZMQ_VERSION))
> 
> Am I missing something in your explanation ?
> 
> Or perhaps you meant "when the third argument is not passed, we need to
> use a := assignment" ?

 Yes, "it" was meant to mean "making the argument optional".

 Regards,
 Arnout

On 05-10-17 12:13, Ismael Luceno wrote:
> On 04/Oct/2017 18:35, Arnout Vandecappelle wrote:
> <...>
>>  The solution (probably) is to use $(PKG) instead of $(pkgname).
> 
> I see.
> 
>>> +# github(user[,package[,version]]): returns site of GitHub repository
>>> +github = https://github.com/$(1)/$(or $(2),$(pkgname))/archive/$(or $(3),$($(call UPPERCASE,$(pkgname))_VERSION))
>>
>>  Could you estimate how many packages could be simplified with this?
> 
> A great deal, including axel, in 219 out of 380 the repo matches the
> package name, with 30 also matching the username.

 Hm, not convincing enough for me. It saves just a tiny little bit of typing, it
does not make things more readable, and it adds complexity to the github macro
itself. No, I don't think it's worth it.


>> I'm not actually sure this is a good idea anyway. The _VERSION is OK
>> because it works for *all* packages, the third argument is only needed
>> when you use the github helper outside a package (e.g. for a custom
>> kernel URL). But this change will apply only to a limited number of
>> packages on the one hand, and on the other hand it makes it even more
>> difficult for people to understand what is meant with this $(call
>> github,foo) construct.
> 
> It's "magic" anyway. It would perhaps make sense to implement this kind
> of rules via rewriting AXEL_SITE, it could be something descriptive like:
> 
> 	AXEL_SITE = @ github user:axel-download-accelerator repo:libtls
> 
> Here's a mockup: http://tmp.iodev.co.uk/rtest.mk

 Here also I don't think the added complexity in the implementation is worth it.
It is indeed a bit more explicit, but now you have to remember that the
arguments are called user: and repo:. For me,
$(call github,axel-download-accelerator,libtls) is an easy-to-remember way to
refer to https://github.com/axel-download-accelerator/libtls.

 Regards,
 Arnout

On 05/Oct/2017 19:50, Arnout Vandecappelle wrote:
> On 05-10-17 12:13, Ismael Luceno wrote:
<...>
> > It's "magic" anyway. It would perhaps make sense to implement this kind
> > of rules via rewriting AXEL_SITE, it could be something descriptive like:
> > 
> > 	AXEL_SITE = @ github user:axel-download-accelerator repo:libtls
> > 
> > Here's a mockup: http://tmp.iodev.co.uk/rtest.mk
> 
> Here also I don't think the added complexity in the implementation is
> worth it.  It is indeed a bit more explicit, but now you have to
> remember that the arguments are called user: and repo:. For me,
> $(call github,axel-download-accelerator,libtls) is an easy-to-remember
> way to refer to https://github.com/axel-download-accelerator/libtls.

Well, it's an interesting exercise anyway; I've simplified it a little
bit (same URL), I think this could be useful for other purposes.

It's perhaps not so obvious where to look for to see how it works, but
documentation fixes that.