| Message ID | c270e9e49dbc69c08629893f7c5e8fa952e37da7.1404167061.git.yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: > The git download helper is getting a bit more complex. Fixing it in the > Makefile when it breaks (like the recent breakage with a non-existing > sha1-cset) 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 and 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> > Acked-by: Luca Ceresoli <luca@lucaceresoli.net> > Cc: Arnout Vandecappelle <arnout@mind.be> > Reviewed-by: Samuel Martin <s.martin49@gmail.com> > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Thanks. I've verified that the (non-gzip'ed) tarball is identical to what we had before, but I noticed that we no longer delete the temporary repo in DL_DIR. Did you do that change on purpose? I don't think we want to keep it, do we?
Dear Peter Korsgaard, On Wed, 02 Jul 2014 17:28:04 +0200, Peter Korsgaard wrote: > Thanks. I've verified that the (non-gzip'ed) tarball is identical to > what we had before, but I noticed that we no longer delete the temporary > repo in DL_DIR. > > Did you do that change on purpose? I don't think we want to keep it, do > we? Hum, I'm confused, didn't we say that we should no longer do any temporary thing in $(DL_DIR) in order to allow parallel builds of separate Buildroot instances to not mess up with each other? I think we said that the process should be: 1/ Clone the repo in $(BUILD_DIR) 2/ Create the tarball of the repo in $(BUILD_DIR) 3/ Move the tarball from $(BUILD_DIR) to $(DL_DIR) with a temporary unique file name. 4/ Rename the tarball in $(DL_DIR) to its final name Steps (3) and (4) are separated so that if $(DL_DIR) and $(BUILD_DIR) are in separate filesystems, the rename to the final name remains an atomic operation. And yes, the git download helper from Yann doesn't seem to implement this logic (or I got lost with the variable names, which is very possible). Yann? Thomas
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> schreef: >Dear Peter Korsgaard, > >On Wed, 02 Jul 2014 17:28:04 +0200, Peter Korsgaard wrote: > >> Thanks. I've verified that the (non-gzip'ed) tarball is identical to >> what we had before, but I noticed that we no longer delete the temporary >> repo in DL_DIR. >> >> Did you do that change on purpose? I don't think we want to keep it, do >> we? > >Hum, I'm confused, didn't we say that we should no longer do any >temporary thing in $(DL_DIR) in order to allow parallel builds of >separate Buildroot instances to not mess up with each other? I think we >said that the process should be: > > 1/ Clone the repo in $(BUILD_DIR) > 2/ Create the tarball of the repo in $(BUILD_DIR) > 3/ Move the tarball from $(BUILD_DIR) to $(DL_DIR) with a temporary > unique file name. > 4/ Rename the tarball in $(DL_DIR) to its final name > >Steps (3) and (4) are separated so that if $(DL_DIR) and $(BUILD_DIR) >are in separate filesystems, the rename to the final name remains an >atomic operation. > >And yes, the git download helper from Yann doesn't seem to implement >this logic (or I got lost with the variable names, which is very >possible). This patch (3/14) only moves the existing logic to a separate script, just like for the other helpers. Later patches in the same series improve the logic. In particular, not using DL_DIR as scratchpad is implemented in patch 11/14 http://patchwork.ozlabs.org/patch/365791/ Best regards, Thomas
Dear Thomas De Schampheleire, On Wed, 2 Jul 2014 19:19:42 +0200, Thomas De Schampheleire wrote: > >> Thanks. I've verified that the (non-gzip'ed) tarball is identical to > >> what we had before, but I noticed that we no longer delete the temporary > >> repo in DL_DIR. > >> > >> Did you do that change on purpose? I don't think we want to keep it, do > >> we? > > > >Hum, I'm confused, didn't we say that we should no longer do any > >temporary thing in $(DL_DIR) in order to allow parallel builds of > >separate Buildroot instances to not mess up with each other? I think we > >said that the process should be: > > > > 1/ Clone the repo in $(BUILD_DIR) > > 2/ Create the tarball of the repo in $(BUILD_DIR) > > 3/ Move the tarball from $(BUILD_DIR) to $(DL_DIR) with a temporary > > unique file name. > > 4/ Rename the tarball in $(DL_DIR) to its final name > > > >Steps (3) and (4) are separated so that if $(DL_DIR) and $(BUILD_DIR) > >are in separate filesystems, the rename to the final name remains an > >atomic operation. > > > >And yes, the git download helper from Yann doesn't seem to implement > >this logic (or I got lost with the variable names, which is very > >possible). > > This patch (3/14) only moves the existing logic to a separate script, > just like for the other helpers. Later patches in the same series > improve the logic. In particular, not using DL_DIR as scratchpad is > implemented in patch 11/14 http://patchwork.ozlabs.org/patch/365791/ Ah, ok, thanks for the clarification. However that doesn't explain why the temporary repo in $(DL_DIR) is not being removed. That's a regression compared to the original code in the .mk file, no? Thomas
On Wed, Jul 2, 2014 at 7:22 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Thomas De Schampheleire, > > On Wed, 2 Jul 2014 19:19:42 +0200, Thomas De Schampheleire wrote: > >> >> Thanks. I've verified that the (non-gzip'ed) tarball is identical to >> >> what we had before, but I noticed that we no longer delete the temporary >> >> repo in DL_DIR. >> >> >> >> Did you do that change on purpose? I don't think we want to keep it, do >> >> we? >> > >> >Hum, I'm confused, didn't we say that we should no longer do any >> >temporary thing in $(DL_DIR) in order to allow parallel builds of >> >separate Buildroot instances to not mess up with each other? I think we >> >said that the process should be: >> > >> > 1/ Clone the repo in $(BUILD_DIR) >> > 2/ Create the tarball of the repo in $(BUILD_DIR) >> > 3/ Move the tarball from $(BUILD_DIR) to $(DL_DIR) with a temporary >> > unique file name. >> > 4/ Rename the tarball in $(DL_DIR) to its final name >> > >> >Steps (3) and (4) are separated so that if $(DL_DIR) and $(BUILD_DIR) >> >are in separate filesystems, the rename to the final name remains an >> >atomic operation. >> > >> >And yes, the git download helper from Yann doesn't seem to implement >> >this logic (or I got lost with the variable names, which is very >> >possible). >> >> This patch (3/14) only moves the existing logic to a separate script, >> just like for the other helpers. Later patches in the same series >> improve the logic. In particular, not using DL_DIR as scratchpad is >> implemented in patch 11/14 http://patchwork.ozlabs.org/patch/365791/ > > Ah, ok, thanks for the clarification. However that doesn't explain why > the temporary repo in $(DL_DIR) is not being removed. That's a > regression compared to the original code in the .mk file, no? Patch 11 does: -rm -rf "${repodir}" +rm -rf "${repodir}" "${tmp_tar}" "${tmp_output}" +exit ${ret} and -repodir="${BR2_DL_DIR}/${basename}" +repodir="${basename}.tmp-git-checkout" +tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )" +tmp_output="$( mktemp "${output}.XXXXXX" )" So from the code (haven't verified this now) there is no repo at all in DL_DIR, so that repo does not need to be removed. The repo $repodir _is_ removed, including all of the other temporary stuff tmp_tar and tmp_output. Peter, did you test this with all patches applied? Thanks, Thomas
Peter, All, On 2014-07-02 17:28 +0200, Peter Korsgaard spake thusly: > >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: > > > The git download helper is getting a bit more complex. Fixing it in the > > Makefile when it breaks (like the recent breakage with a non-existing > > sha1-cset) 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 and 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> > > Acked-by: Luca Ceresoli <luca@lucaceresoli.net> > > Cc: Arnout Vandecappelle <arnout@mind.be> > > Reviewed-by: Samuel Martin <s.martin49@gmail.com> > > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > > Thanks. I've verified that the (non-gzip'ed) tarball is identical to > what we had before, but I noticed that we no longer delete the temporary > repo in DL_DIR. > > Did you do that change on purpose? I don't think we want to keep it, do > we? For sure, that's not on-purpose. Now, I fail to see how the remporary repository would linger after a successfull download. Here's what the script does: repodir="${BR2_DL_DIR}/${basename}" if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then printf "Doing shallow clone\n" ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" "${repodir}" else printf "Doing full clone\n" ${GIT} clone --bare "${repo}" "${repodir}" fi pushd "${repodir}" ${GIT} archive --prefix="${basename}/" -o "${output}.tmp" --format=tar "${cset}" gzip -c "${output}.tmp" >"${output}" rm -f "${output}.tmp" popd rm -rf "${repodir}" So, unless there is something I'm missing, the last statement should get rid of the temp repo... Lemme try... Nope, the repository is properly removed here: $ make libllcp-source [--SNIP--] >>> libllcp cf0c4b3c9df98851c6092c130192130c3f5a46bd Downloading Doing full clone Cloning into bare repository '/home/ymorin/dev/buildroot/O/test-dl/libllcp-cf0c4b3c9df98851c6092c130192130c3f5a46bd'... remote: Counting objects: 1202, done. Receiving objects: 100% (1202/1202), 267.40 KiB | 145.00 KiB/s, done. Resolving deltas: 100% (812/812), done. Checking connectivity... done. ~/dev/buildroot/O/test-dl/libllcp-cf0c4b3c9df98851c6092c130192130c3f5a46bd ~/dev/buildroot/buildroot ~/dev/buildroot/buildroot $ ls -l /home/ymorin/dev/buildroot/O/test-dl/ -rw-rw-r-- 1 ymorin ymorin 52K Jul 2 22:23 libllcp-cf0c4b3c9df98851c6092c130192130c3f5a46bd.tar.gz So, no lingering clone... Regards, Yann E. MORIN.
Thomas², Peter, All, On 2014-07-02 19:31 +0200, Thomas De Schampheleire spake thusly: > On Wed, Jul 2, 2014 at 7:22 PM, Thomas Petazzoni > <thomas.petazzoni@free-electrons.com> wrote: > > Dear Thomas De Schampheleire, > > > > On Wed, 2 Jul 2014 19:19:42 +0200, Thomas De Schampheleire wrote: > > > >> >> Thanks. I've verified that the (non-gzip'ed) tarball is identical to > >> >> what we had before, but I noticed that we no longer delete the temporary > >> >> repo in DL_DIR. > >> >> > >> >> Did you do that change on purpose? I don't think we want to keep it, do > >> >> we? > >> > > >> >Hum, I'm confused, didn't we say that we should no longer do any > >> >temporary thing in $(DL_DIR) in order to allow parallel builds of > >> >separate Buildroot instances to not mess up with each other? I think we > >> >said that the process should be: > >> > > >> > 1/ Clone the repo in $(BUILD_DIR) > >> > 2/ Create the tarball of the repo in $(BUILD_DIR) > >> > 3/ Move the tarball from $(BUILD_DIR) to $(DL_DIR) with a temporary > >> > unique file name. > >> > 4/ Rename the tarball in $(DL_DIR) to its final name > >> > > >> >Steps (3) and (4) are separated so that if $(DL_DIR) and $(BUILD_DIR) > >> >are in separate filesystems, the rename to the final name remains an > >> >atomic operation. > >> > > >> >And yes, the git download helper from Yann doesn't seem to implement > >> >this logic (or I got lost with the variable names, which is very > >> >possible). > >> > >> This patch (3/14) only moves the existing logic to a separate script, > >> just like for the other helpers. Later patches in the same series > >> improve the logic. In particular, not using DL_DIR as scratchpad is > >> implemented in patch 11/14 http://patchwork.ozlabs.org/patch/365791/ > > > > Ah, ok, thanks for the clarification. However that doesn't explain why > > the temporary repo in $(DL_DIR) is not being removed. That's a > > regression compared to the original code in the .mk file, no? > > Patch 11 does: > > -rm -rf "${repodir}" > +rm -rf "${repodir}" "${tmp_tar}" "${tmp_output}" > +exit ${ret} > > and > > -repodir="${BR2_DL_DIR}/${basename}" > +repodir="${basename}.tmp-git-checkout" > +tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )" > +tmp_output="$( mktemp "${output}.XXXXXX" )" > > So from the code (haven't verified this now) there is no repo at all > in DL_DIR, so that repo does not need to be removed. The repo $repodir > _is_ removed, including all of the other temporary stuff tmp_tar and > tmp_output. > > Peter, did you test this with all patches applied? Well, even if the full series behaves correctly, the patch 3 should not change the existing behaviour. Any change in behaviour is indeed a regression, and thus a bug. But I could not observe any change in behaviour here. Regards, Yann E. MORIN.
Dear Thomas De Schampheleire, On Wed, 2 Jul 2014 19:31:08 +0200, Thomas De Schampheleire wrote: > > Ah, ok, thanks for the clarification. However that doesn't explain why > > the temporary repo in $(DL_DIR) is not being removed. That's a > > regression compared to the original code in the .mk file, no? > > Patch 11 does: > > -rm -rf "${repodir}" > +rm -rf "${repodir}" "${tmp_tar}" "${tmp_output}" > +exit ${ret} > > and > > -repodir="${BR2_DL_DIR}/${basename}" > +repodir="${basename}.tmp-git-checkout" > +tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )" > +tmp_output="$( mktemp "${output}.XXXXXX" )" > > So from the code (haven't verified this now) there is no repo at all > in DL_DIR, so that repo does not need to be removed. The repo $repodir > _is_ removed, including all of the other temporary stuff tmp_tar and > tmp_output. > > Peter, did you test this with all patches applied? I think what Peter complains about is that with just PATCH 03/14 applied, the Git download method no longer works properly as it leaves the repo in $(DL_DIR). The fact that this problem may or may not get solved by a later patch in the series is more-or-less irrelevant here: Peter is just saying that the series is not perfectly bisectable, and he stopped reviewing when he saw an issue in PATCH 03/14. Thomas
Thomas², All, On 2014-07-02 22:31 +0200, Thomas Petazzoni spake thusly: > On Wed, 2 Jul 2014 19:31:08 +0200, Thomas De Schampheleire wrote: > > > Ah, ok, thanks for the clarification. However that doesn't explain why > > > the temporary repo in $(DL_DIR) is not being removed. That's a > > > regression compared to the original code in the .mk file, no? > > > > Patch 11 does: > > > > -rm -rf "${repodir}" > > +rm -rf "${repodir}" "${tmp_tar}" "${tmp_output}" > > +exit ${ret} > > > > and > > > > -repodir="${BR2_DL_DIR}/${basename}" > > +repodir="${basename}.tmp-git-checkout" > > +tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )" > > +tmp_output="$( mktemp "${output}.XXXXXX" )" > > > > So from the code (haven't verified this now) there is no repo at all > > in DL_DIR, so that repo does not need to be removed. The repo $repodir > > _is_ removed, including all of the other temporary stuff tmp_tar and > > tmp_output. > > > > Peter, did you test this with all patches applied? > > I think what Peter complains about is that with just PATCH 03/14 > applied, the Git download method no longer works properly as it leaves > the repo in $(DL_DIR). > > The fact that this problem may or may not get solved by a later patch > in the series is more-or-less irrelevant here: Agreed. If that patch changes the behaviour, it is incorrect. > Peter is just saying > that the series is not perfectly bisectable, and he stopped reviewing > when he saw an issue in PATCH 03/14. And I do not see this issue... Regards, Yann E. MORIN.
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: Hi, >> I think what Peter complains about is that with just PATCH 03/14 >> applied, the Git download method no longer works properly as it leaves >> the repo in $(DL_DIR). Indeed. >> The fact that this problem may or may not get solved by a later patch >> in the series is more-or-less irrelevant here: > Agreed. If that patch changes the behaviour, it is incorrect. >> Peter is just saying >> that the series is not perfectly bisectable, and he stopped reviewing >> when he saw an issue in PATCH 03/14. > And I do not see this issue... Sorry, I got confused. The problem actually turned out to be the use of pushd/popd with a /bin/sh, on here /bin/sh is dash which doesn't support it. The fix is to use !#/bin/bash
diff --git a/package/pkg-download.mk b/package/pkg-download.mk index e07fd1b..c848f6a 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,19 +84,8 @@ github = https://github.com/$(1)/$(2)/archive/$(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)/ -o $(DL_DIR)/.$($(PKG)_SOURCE).tmp $($(PKG)_DL_VERSION) && \ - gzip -c $(DL_DIR)/.$($(PKG)_SOURCE).tmp > $(DL_DIR)/$($(PKG)_SOURCE) && \ - rm -f $(DL_DIR)/.$($(PKG)_SOURCE).tmp && \ - 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..96db3a9 --- /dev/null +++ b/support/download/git @@ -0,0 +1,37 @@ +#!/bin/sh + +# We want to catch any command failure, and exit immediately +set -e + +# Download helper for git +# Call it with: +# $1: git repo +# $2: git cset +# $3: package's basename (eg. foobar-1.2.3) +# $4: output file +# And this environment: +# BR2_DL_DIR: path to Buildroot's download dir +# GIT : the git command to call + +repo="${1}" +cset="${2}" +basename="${3}" +output="${4}" + +repodir="${BR2_DL_DIR}/${basename}" + +if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then + printf "Doing shallow clone\n" + ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" "${repodir}" +else + printf "Doing full clone\n" + ${GIT} clone --bare "${repo}" "${repodir}" +fi + +pushd "${repodir}" +${GIT} archive --prefix="${basename}/" -o "${output}.tmp" --format=tar "${cset}" +gzip -c "${output}.tmp" >"${output}" +rm -f "${output}.tmp" +popd + +rm -rf "${repodir}"