Message ID | 20171004085745.15457-1-ismael@iodev.co.uk |
---|---|
State | Rejected |
Headers | show |
Series | pkg-download: make package name optional in github helper | expand |
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.
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)) +# github(user[,package[,version]]): returns site of GitHub repository +github = https://github.com/$(1)/$(or $(2),$(pkgname))/archive/$(or $(3),$($(call UPPERCASE,$(pkgname))_VERSION)) # Expressly do not check hashes for those files # Exported variables default to immediately expanded in some versions of
Signed-off-by: Ismael Luceno <ismael@iodev.co.uk> --- package/pkg-download.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)