Message ID | add407fe211f2032f76fda6442b06b7d8a3af2cc.1389569992.git.yann.morin.1998@free.fr |
---|---|
State | Changes Requested |
Headers | show |
Hi Yann, Yann E. MORIN wrote: > From: "Yann E. MORIN" <yann.morin.1998@free.fr> > > The git download helper is getting a bit more complex, and does not > raise an error when a PKG_VERSION is a sha1 that does not exist in > the repository. Instead, it generates an empty archive. > > Fixing it in the Makefile proves to be challenging, to say the least. > > Move it into a shell script in support/download/git, which will make > it much easier to read, maintain, fix an enhance in the future. > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Peter Korsgaard <jacmet@uclibc.org> > Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> This is a very welcome change, thanks for caring! Just a minor note below, but looks ok anyway: Acked-by: Luca Ceresoli <luca@lucaceresoli.net> > --- > package/pkg-download.mk | 16 +++------------- > support/download/git | 36 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 39 insertions(+), 13 deletions(-) > create mode 100755 support/download/git > > diff --git a/package/pkg-download.mk b/package/pkg-download.mk > index c00689b..8e6f7a3 100644 > --- a/package/pkg-download.mk > +++ b/package/pkg-download.mk > @@ -12,7 +12,7 @@ WGET := $(call qstrip,$(BR2_WGET)) $(QUIET) > SVN := $(call qstrip,$(BR2_SVN)) > CVS := $(call qstrip,$(BR2_CVS)) > BZR := $(call qstrip,$(BR2_BZR)) > -GIT := $(call qstrip,$(BR2_GIT)) > +export GIT := $(call qstrip,$(BR2_GIT)) > HG := $(call qstrip,$(BR2_HG)) $(QUIET) > SCP := $(call qstrip,$(BR2_SCP)) $(QUIET) > SSH := $(call qstrip,$(BR2_SSH)) $(QUIET) > @@ -84,18 +84,8 @@ github = https://github.com/$(1)/$(2)/tarball/$(3) > # problems > define DOWNLOAD_GIT > test -e $(DL_DIR)/$($(PKG)_SOURCE) || \ > - (pushd $(DL_DIR) > /dev/null && \ > - ((test "`git ls-remote $($(PKG)_SITE) $($(PKG)_DL_VERSION)`" && \ > - echo "Doing shallow clone" && \ > - $(GIT) clone --depth 1 -b $($(PKG)_DL_VERSION) --bare $($(PKG)_SITE) $($(PKG)_BASE_NAME)) || \ > - (echo "Doing full clone" && \ > - $(GIT) clone --bare $($(PKG)_SITE) $($(PKG)_BASE_NAME))) && \ > - pushd $($(PKG)_BASE_NAME) > /dev/null && \ > - $(GIT) archive --format=tar --prefix=$($(PKG)_BASE_NAME)/ $($(PKG)_DL_VERSION) | \ > - gzip -c > $(DL_DIR)/$($(PKG)_SOURCE) && \ > - popd > /dev/null && \ > - rm -rf $($(PKG)_DL_DIR) && \ > - popd > /dev/null) > + $(EXTRA_ENV) support/download/git $($(PKG)_SITE) $($(PKG)_DL_VERSION) \ > + $($(PKG)_BASE_NAME)/ $(DL_DIR)/$($(PKG)_SOURCE) > endef > > # TODO: improve to check that the given PKG_DL_VERSION exists on the remote > diff --git a/support/download/git b/support/download/git > new file mode 100755 > index 0000000..99ea1b2 > --- /dev/null > +++ b/support/download/git > @@ -0,0 +1,36 @@ > +#!/bin/sh > +set -e > + > +# Download helper for git > +# Call it with: > +# $1: git repo > +# $2: git cset > +# $3: prefix/ > +# $4: output file > +# And this environment: > +# GIT : the git command to call > +# BUILD_DIR: path to Buildroot's build dir > + > +repo="${1}" > +cset="${2}" > +prefix="${3}" > +output="${4}" > + > +pushd "${BUILD_DIR}" >/dev/null > +rm -rf .git-tmp-repo # In case a previous attempt was interrupted This rm did not exist in the original version. I would move this into a separate commit. > + > +if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then > + printf "Doing shallow clone\n" > + ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" .git-tmp-repo > +else > + printf "Doing full clone\n" > + ${GIT} clone --bare "${repo}" .git-tmp-repo > +fi > + > +pushd .git-tmp-repo >/dev/null > +${GIT} archive --prefix="${prefix}" --format=tar "${cset}" \ > +|gzip -c >"${output}" > +popd >/dev/null > + > +rm -rf .git-tmp-repo > +popd >/dev/null >
Luca, All, On 2014-01-13 15:18 +0100, Luca Ceresoli spake thusly: > Yann E. MORIN wrote: > >From: "Yann E. MORIN" <yann.morin.1998@free.fr> > > > >The git download helper is getting a bit more complex, and does not > >raise an error when a PKG_VERSION is a sha1 that does not exist in > >the repository. Instead, it generates an empty archive. > > > >Fixing it in the Makefile proves to be challenging, to say the least. > > > >Move it into a shell script in support/download/git, which will make > >it much easier to read, maintain, fix an enhance in the future. > > > >Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > >Cc: Peter Korsgaard <jacmet@uclibc.org> > >Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > This is a very welcome change, thanks for caring! > > Just a minor note below, but looks ok anyway: > Acked-by: Luca Ceresoli <luca@lucaceresoli.net> [--SNIP--] > >diff --git a/support/download/git b/support/download/git > >new file mode 100755 > >index 0000000..99ea1b2 > >--- /dev/null > >+++ b/support/download/git > >@@ -0,0 +1,36 @@ > >+#!/bin/sh > >+set -e > >+ > >+# Download helper for git > >+# Call it with: > >+# $1: git repo > >+# $2: git cset > >+# $3: prefix/ > >+# $4: output file > >+# And this environment: > >+# GIT : the git command to call > >+# BUILD_DIR: path to Buildroot's build dir > >+ > >+repo="${1}" > >+cset="${2}" > >+prefix="${3}" > >+output="${4}" > >+ > >+pushd "${BUILD_DIR}" >/dev/null > >+rm -rf .git-tmp-repo # In case a previous attempt was interrupted > > This rm did not exist in the original version. I would move this into a > separate commit. Indeed, I've moved it to the next commit. Thank you! Regards, Yann E. MORIN.
On 13/01/14 00:44, Yann E. MORIN wrote: > From: "Yann E. MORIN" <yann.morin.1998@free.fr> > > The git download helper is getting a bit more complex, and does not > raise an error when a PKG_VERSION is a sha1 that does not exist in > the repository. Instead, it generates an empty archive. > > Fixing it in the Makefile proves to be challenging, to say the least. > > Move it into a shell script in support/download/git, which will make > it much easier to read, maintain, fix an enhance in the future. This patch contains a few more changes than just moving it to a script. These should ideally be done in separate patches. [snip] > diff --git a/support/download/git b/support/download/git > new file mode 100755 > index 0000000..99ea1b2 > --- /dev/null > +++ b/support/download/git > @@ -0,0 +1,36 @@ > +#!/bin/sh > +set -e > + > +# Download helper for git > +# Call it with: > +# $1: git repo > +# $2: git cset > +# $3: prefix/ > +# $4: output file > +# And this environment: > +# GIT : the git command to call > +# BUILD_DIR: path to Buildroot's build dir > + > +repo="${1}" > +cset="${2}" > +prefix="${3}" > +output="${4}" > + > +pushd "${BUILD_DIR}" >/dev/null This used to be DL_DIR. Note that I agree that BUILD_DIR is more appropriate. Although the original indeed does a pushd, this is quite redundant when it's done in a script. So I'd directly make it a simple cd (without redirection). > +rm -rf .git-tmp-repo # In case a previous attempt was interrupted > + > +if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then > + printf "Doing shallow clone\n" > + ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" .git-tmp-repo It used to be extracted into $($(PKG)_BASE_NAME). That has the advantage that it is unique, which makes it possible to do multiple downloads in parallel (which I sometimes do BTW, by manually starting downloads in separate shells). But even better would be $($(PKG)_BASE_NAME).git-tmp-repo :-) > +else > + printf "Doing full clone\n" > + ${GIT} clone --bare "${repo}" .git-tmp-repo > +fi > + > +pushd .git-tmp-repo >/dev/null > +${GIT} archive --prefix="${prefix}" --format=tar "${cset}" \ > +|gzip -c >"${output}" We have about 20 instances of the | at the end of the line (as was the original), and only 4 at the beginning of the line (and two of these were done by you...). In the original, the / at the end of the prefix is added explicitly here in the call to git archive. I think that makes more sense than adding the / in the call itself. Regards, Arnout > +popd >/dev/null > + > +rm -rf .git-tmp-repo > +popd >/dev/null >
Arnout, All, On 2014-01-14 21:39 +0100, Arnout Vandecappelle spake thusly: > On 13/01/14 00:44, Yann E. MORIN wrote: > >From: "Yann E. MORIN" <yann.morin.1998@free.fr> > > > >The git download helper is getting a bit more complex, and does not > >raise an error when a PKG_VERSION is a sha1 that does not exist in > >the repository. Instead, it generates an empty archive. > > > >Fixing it in the Makefile proves to be challenging, to say the least. > > > >Move it into a shell script in support/download/git, which will make > >it much easier to read, maintain, fix an enhance in the future. > > This patch contains a few more changes than just moving it to a script. > These should ideally be done in separate patches. Doh. You're right. I already fixed some, but I missed quite a few. Sigh... > [snip] > >diff --git a/support/download/git b/support/download/git > >new file mode 100755 > >index 0000000..99ea1b2 > >--- /dev/null > >+++ b/support/download/git [--SNIP--] > >+pushd "${BUILD_DIR}" >/dev/null > > This used to be DL_DIR. I do not like to treat DL_DIR as a scratch area, hence the use of BUIL_DIR isntead. > Note that I agree that BUILD_DIR is more appropriate. I'll do the change in a spearate patch. > Although the original indeed does a pushd, this is quite redundant when > it's done in a script. So I'd directly make it a simple cd (without > redirection). I'll simplify this. > >+rm -rf .git-tmp-repo # In case a previous attempt was interrupted > >+ > >+if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then > >+ printf "Doing shallow clone\n" > >+ ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" .git-tmp-repo > > It used to be extracted into $($(PKG)_BASE_NAME). That has the advantage > that it is unique, which makes it possible to do multiple downloads in > parallel (which I sometimes do BTW, by manually starting downloads in > separate shells). No problem, since we're now using BUILD_DIR, not DL_DIR. ;-) > But even better would be $($(PKG)_BASE_NAME).git-tmp-repo :-) OK. > >+else > >+ printf "Doing full clone\n" > >+ ${GIT} clone --bare "${repo}" .git-tmp-repo > >+fi > >+ > >+pushd .git-tmp-repo >/dev/null > >+${GIT} archive --prefix="${prefix}" --format=tar "${cset}" \ > >+|gzip -c >"${output}" > > We have about 20 instances of the | at the end of the line (as was the > original), and only 4 at the beginning of the line (and two of these were > done by you...). Yes, that's my own coding style. I plead guilty! ;-) Will fix. > In the original, the / at the end of the prefix is added explicitly here in > the call to git archive. I think that makes more sense than adding the / in > the call itself. OK, will fix. Thank you! :-) Regards, Yann E. MORIN.
diff --git a/package/pkg-download.mk b/package/pkg-download.mk index c00689b..8e6f7a3 100644 --- a/package/pkg-download.mk +++ b/package/pkg-download.mk @@ -12,7 +12,7 @@ WGET := $(call qstrip,$(BR2_WGET)) $(QUIET) SVN := $(call qstrip,$(BR2_SVN)) CVS := $(call qstrip,$(BR2_CVS)) BZR := $(call qstrip,$(BR2_BZR)) -GIT := $(call qstrip,$(BR2_GIT)) +export GIT := $(call qstrip,$(BR2_GIT)) HG := $(call qstrip,$(BR2_HG)) $(QUIET) SCP := $(call qstrip,$(BR2_SCP)) $(QUIET) SSH := $(call qstrip,$(BR2_SSH)) $(QUIET) @@ -84,18 +84,8 @@ github = https://github.com/$(1)/$(2)/tarball/$(3) # problems define DOWNLOAD_GIT test -e $(DL_DIR)/$($(PKG)_SOURCE) || \ - (pushd $(DL_DIR) > /dev/null && \ - ((test "`git ls-remote $($(PKG)_SITE) $($(PKG)_DL_VERSION)`" && \ - echo "Doing shallow clone" && \ - $(GIT) clone --depth 1 -b $($(PKG)_DL_VERSION) --bare $($(PKG)_SITE) $($(PKG)_BASE_NAME)) || \ - (echo "Doing full clone" && \ - $(GIT) clone --bare $($(PKG)_SITE) $($(PKG)_BASE_NAME))) && \ - pushd $($(PKG)_BASE_NAME) > /dev/null && \ - $(GIT) archive --format=tar --prefix=$($(PKG)_BASE_NAME)/ $($(PKG)_DL_VERSION) | \ - gzip -c > $(DL_DIR)/$($(PKG)_SOURCE) && \ - popd > /dev/null && \ - rm -rf $($(PKG)_DL_DIR) && \ - popd > /dev/null) + $(EXTRA_ENV) support/download/git $($(PKG)_SITE) $($(PKG)_DL_VERSION) \ + $($(PKG)_BASE_NAME)/ $(DL_DIR)/$($(PKG)_SOURCE) endef # TODO: improve to check that the given PKG_DL_VERSION exists on the remote diff --git a/support/download/git b/support/download/git new file mode 100755 index 0000000..99ea1b2 --- /dev/null +++ b/support/download/git @@ -0,0 +1,36 @@ +#!/bin/sh +set -e + +# Download helper for git +# Call it with: +# $1: git repo +# $2: git cset +# $3: prefix/ +# $4: output file +# And this environment: +# GIT : the git command to call +# BUILD_DIR: path to Buildroot's build dir + +repo="${1}" +cset="${2}" +prefix="${3}" +output="${4}" + +pushd "${BUILD_DIR}" >/dev/null +rm -rf .git-tmp-repo # In case a previous attempt was interrupted + +if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then + printf "Doing shallow clone\n" + ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" .git-tmp-repo +else + printf "Doing full clone\n" + ${GIT} clone --bare "${repo}" .git-tmp-repo +fi + +pushd .git-tmp-repo >/dev/null +${GIT} archive --prefix="${prefix}" --format=tar "${cset}" \ +|gzip -c >"${output}" +popd >/dev/null + +rm -rf .git-tmp-repo +popd >/dev/null