Re: [OpenWrt-Devel] [PATCH v2] build: download code from github using archive API
On Fri, 29 Jun 2018 at 14:17, Jo-Philipp Wich wrote: > > Hi, > > > Looks like it's caused by the excessive python script call. This is > > indeed unexpected. I just pushed a commit to disable it altogether > > for now. Sorry for the inconvenience ;( > > is there anything preventing you from doing a change like below to wire > in the github archiving script? > > -- 8< -- > diff --git a/include/download.mk b/include/download.mk > index 3634e777c9..5a9328b55d 100644 > --- a/include/download.mk > +++ b/include/download.mk > @@ -23,13 +23,15 @@ DOWNLOAD_RDEP=$(STAMP_PREPARED) $(HOST_STAMP_PREPARED) > define dl_method > $(strip \ >$(if $(2),$(2), \ > -$(if $(filter @APACHE/% @GITHUB/% @GNOME/% @GNU/% @KERNEL/% @SF/% > @SAVANNAH/% ftp://% http://% https://% file://%,$(1)),default, \ > - $(if $(filter git://%,$(1)),git, \ > -$(if $(filter svn://%,$(1)),svn, \ > - $(if $(filter cvs://%,$(1)),cvs, \ > -$(if $(filter hg://%,$(1)),hg, \ > - $(if $(filter sftp://%,$(1)),bzr, \ > -unknown \ > +$(if $(filter git://github.com/% > https://github.com/%,$(1)),github-tarball, \ It's possible that https://github.com/% is a URL to uploaded static release tarballs and in that case the "default" should be used. I chose to implement the dl_method in python thinking that it should be more expressive without so many levels of nested if/else when writing such logic. yousong > + $(if $(filter @APACHE/% @GITHUB/% @GNOME/% @GNU/% @KERNEL/% @SF/% > @SAVANNAH/% ftp://% http://% https://% file://%,$(1)),default, \ > +$(if $(filter git://%,$(1)),git, \ > + $(if $(filter svn://%,$(1)),svn, \ > +$(if $(filter cvs://%,$(1)),cvs, \ > + $(if $(filter hg://%,$(1)),hg, \ > +$(if $(filter sftp://%,$(1)),bzr, \ > + unknown \ > +) \ >) \ > ) \ >) \ > -- >8 -- > > ~ Jo > > ___ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH v2] build: download code from github using archive API
Hi, > Looks like it's caused by the excessive python script call. This is > indeed unexpected. I just pushed a commit to disable it altogether > for now. Sorry for the inconvenience ;( is there anything preventing you from doing a change like below to wire in the github archiving script? -- 8< -- diff --git a/include/download.mk b/include/download.mk index 3634e777c9..5a9328b55d 100644 --- a/include/download.mk +++ b/include/download.mk @@ -23,13 +23,15 @@ DOWNLOAD_RDEP=$(STAMP_PREPARED) $(HOST_STAMP_PREPARED) define dl_method $(strip \ $(if $(2),$(2), \ -$(if $(filter @APACHE/% @GITHUB/% @GNOME/% @GNU/% @KERNEL/% @SF/% @SAVANNAH/% ftp://% http://% https://% file://%,$(1)),default, \ - $(if $(filter git://%,$(1)),git, \ -$(if $(filter svn://%,$(1)),svn, \ - $(if $(filter cvs://%,$(1)),cvs, \ -$(if $(filter hg://%,$(1)),hg, \ - $(if $(filter sftp://%,$(1)),bzr, \ -unknown \ +$(if $(filter git://github.com/% https://github.com/%,$(1)),github-tarball, \ + $(if $(filter @APACHE/% @GITHUB/% @GNOME/% @GNU/% @KERNEL/% @SF/% @SAVANNAH/% ftp://% http://% https://% file://%,$(1)),default, \ +$(if $(filter git://%,$(1)),git, \ + $(if $(filter svn://%,$(1)),svn, \ +$(if $(filter cvs://%,$(1)),cvs, \ + $(if $(filter hg://%,$(1)),hg, \ +$(if $(filter sftp://%,$(1)),bzr, \ + unknown \ +) \ ) \ ) \ ) \ -- >8 -- ~ Jo signature.asc Description: OpenPGP digital signature ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH v2] build: download code from github using archive API
On Fri, 29 Jun 2018 at 13:16, Jo-Philipp Wich wrote: > > Hi Yousong, > > this change seems to introduce serious runtime regressions, see > https://bugs.openwrt.org/index.php?do=details_id=1621 > > It seems to be like that script does things even if the tarballs are > already existing locally. > > On a slightly related note, I find the name to be too generic. It > implies that it handles all kinds of download tasks but in the end it is > only a Github archive wrapper. Maybe you should rename it to github.py > or similar. > > ~ Jo Looks like it's caused by the excessive python script call. This is indeed unexpected. I just pushed a commit to disable it altogether for now. Sorry for the inconvenience ;( yousong ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH v2] build: download code from github using archive API
Hi, > It seems to be like that script does things even if the tarballs are > already existing locally. the main problem I see is that you moved the download method detection from inline make code into the external Python script ... this will cause a lot of overhead as during the metadata scanning, your script is getting executed once for every Makefile which can easily be several thousand times with feeds installed. ~ Jo signature.asc Description: OpenPGP digital signature ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH v2] build: download code from github using archive API
Hi Yousong, this change seems to introduce serious runtime regressions, see https://bugs.openwrt.org/index.php?do=details_id=1621 It seems to be like that script does things even if the tarballs are already existing locally. On a slightly related note, I find the name to be too generic. It implies that it handles all kinds of download tasks but in the end it is only a Github archive wrapper. Maybe you should rename it to github.py or similar. ~ Jo signature.asc Description: OpenPGP digital signature ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[OpenWrt-Devel] [PATCH v2] build: download code from github using archive API
A new python script scripts/download.py is added to fetch tarballs using GitHub archive API [1], then repack in a reproducible way same as the current DownloadMethod/git GitHub imposes a 60 reqs/hour rate limit on unauthenticated API access[2]. This affects fetching get commit date for tar --mtime= argument. Current observation indicates that archive download is not subject to this limit at the moment. In the rare cases where download fails due to the limit, we will falback to using DownloadMethod/git The missing piece in the GitHub API is that it cannot provide in the tarball dependent submodules's source code. In that case, the implementation will also fallback to using DownloadMethod/git [1] Get archive link, https://developer.github.com/v3/repos/contents/#get-archive-link [2] Rate limiting, https://developer.github.com/v3/#rate-limiting v2 <- v1: - allow passing multiple urls with --urls argument - add commit ts cache. can be helpful on retry Signed-off-by: Yousong Zhou --- include/download.mk | 74 +- scripts/download.py | 391 2 files changed, 434 insertions(+), 31 deletions(-) create mode 100755 scripts/download.py diff --git a/include/download.mk b/include/download.mk index 2ba8a7bdf4..91a22c2316 100644 --- a/include/download.mk +++ b/include/download.mk @@ -21,23 +21,7 @@ DOWNLOAD_RDEP=$(STAMP_PREPARED) $(HOST_STAMP_PREPARED) # Try to guess the download method from the URL define dl_method -$(strip \ - $(if $(2),$(2), \ -$(if $(filter @APACHE/% @GITHUB/% @GNOME/% @GNU/% @KERNEL/% @SF/% @SAVANNAH/% ftp://% http://% https://% file://%,$(1)),default, \ - $(if $(filter git://%,$(1)),git, \ -$(if $(filter svn://%,$(1)),svn, \ - $(if $(filter cvs://%,$(1)),cvs, \ -$(if $(filter hg://%,$(1)),hg, \ - $(if $(filter sftp://%,$(1)),bzr, \ -unknown \ - ) \ -) \ - ) \ -) \ - ) \ -) \ - ) \ -) +$(shell $(SCRIPT_DIR)/download.py dl_method --url $(foreach url,$(1),"$(url)") --proto="$(2)") endef # code for creating tarballs from cvs/svn/git/bzr/hg/darcs checkouts - useful for mirror support @@ -56,6 +40,10 @@ ifdef CHECK check_escape=$(subst ','\'',$(1)) #') +# $(1): suffix of the F_, C_ variables, e.g. hash_deprecated, hash_mismatch, etc. +# $(2): filename +# $(3): expected hash value +# $(4): hash var name: MD5SUM, HASH check_warn_nofix = $(info $(shell printf "$(_R)WARNING: %s$(_N)" '$(call check_escape,$(call C_$(1),$(2),$(3),$(4)))')) ifndef FIXUP check_warn = $(check_warn_nofix) @@ -71,6 +59,9 @@ F_hash_mismatch = $(F_hash_deprecated) F_hash_missing = $(SCRIPT_DIR)/fixup-makefile.pl $(CURDIR)/Makefile add-hash $(3) $(call gen_sha256sum,$(1)) endif +# $(1): filename +# $(2): expected hash value +# $(3): hash var name: MD5SUM, HASH C_download_missing = $(1) is missing, please run make download before re-running this check C_hash_mismatch = $(3) does not match $(1) hash $(call gen_sha256sum,$(1)) C_hash_deprecated = $(3) uses deprecated hash, set to $(call gen_sha256sum,$(1)) @@ -116,6 +107,9 @@ define DownloadMethod/default ) endef +# $(1): "check" +# $(2): "PKG_" if as in Download/ is "default", otherwise "Download/:" +# $(3): shell command sequence to do the download define wrap_mirror $(if $(if $(MIRROR),$(filter-out x,$(MIRROR_HASH))),$(SCRIPT_DIR)/download.pl "$(DL_DIR)" "$(FILE)" "$(MIRROR_HASH)" "" || ( $(3) ),$(3)) \ $(if $(filter check,$(1)), \ @@ -159,23 +153,41 @@ endef define DownloadMethod/git $(call wrap_mirror,$(1),$(2), \ - echo "Checking out files from the git repository..."; \ - mkdir -p $(TMP_DIR)/dl && \ - cd $(TMP_DIR)/dl && \ - rm -rf $(SUBDIR) && \ - [ \! -d $(SUBDIR) ] && \ - git clone $(OPTS) $(URL) $(SUBDIR) && \ - (cd $(SUBDIR) && git checkout $(VERSION) && \ - git submodule update --init --recursive) && \ - echo "Packing checkout..." && \ - export TAR_TIMESTAMP=`cd $(SUBDIR) && git log -1 --format='@%ct'` && \ - rm -rf $(SUBDIR)/.git && \ - $(call dl_tar_pack,$(TMP_DIR)/dl/$(FILE),$(SUBDIR)) && \ - mv $(TMP_DIR)/dl/$(FILE) $(DL_DIR)/ && \ - rm -rf $(SUBDIR); \ + $(call DownloadMethod/git-raw) \ ) endef +define DownloadMethod/github-tarball + $(call wrap_mirror,$(1),$(2), \ + $(SCRIPT_DIR)/download.py dl \ + --dl-dir="$(DL_DIR)" \ + --url $(foreach url,$(URL),"$(url)") \ + --proto="$(PROTO)" \ + --version="$(VERSION)" \ + --subdir="$(SUBDIR)" \ + --source="$(FILE)" \ + || ( $(call DownloadMethod/git-raw) ); \ + ) +endef + +# Only intends to
