| Message ID | 399c6d6d31d5b3dbe337ce97bb18c3e19f224422.1404681878.git.yann.morin.1998@free.fr |
|---|---|
| State | Superseded |
| Headers | show |
On 06/07/14 23:27, Yann E. MORIN wrote: > Create the temp file in the final location only when it is needed. > > This avoids the nasty experience of seeing lots of temp files in > BR2_DL_DIR, that would linger around in case the downloads fails. It would also help to add a trap "rm -f ${tmp_dl}" EXIT HUP INT QUIT TERM which also removes the need of doing the rm at the end. Of course, that trap is still racy (interrupt between mktemp and trap). But with the two temporary variables, adding a trap becomes even more difficult. Unless that part is refactored into a separate helper script of course :-) > > Add a comment on why we don;t clean-up after git. Typo in don;t > > Reported-by: Peter Korsgaard <jacmet@uclibc.org> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > --- > support/download/bzr | 2 +- > support/download/cvs | 2 +- > support/download/git | 6 ++++-- > support/download/hg | 2 +- > support/download/scp | 2 +- > support/download/svn | 2 +- > support/download/wget | 2 +- > 7 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/support/download/bzr b/support/download/bzr > index f86fa82..a2cb440 100755 > --- a/support/download/bzr > +++ b/support/download/bzr > @@ -17,7 +17,6 @@ rev="${2}" > output="${3}" > > tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )" > -tmp_output="$( mktemp "${output}.XXXXXX" )" > > # Play tic-tac-toe with temp files > # - first, we download to a trashable location (the build-dir) > @@ -27,6 +26,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )" > > ret=1 > if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then > + tmp_output="$( mktemp "${output}.XXXXXX" )" > if cat "${tmp_dl}" >"${tmp_output}"; then > mv "${tmp_output}" "${output}" > ret=0 > diff --git a/support/download/cvs b/support/download/cvs > index a8ab080..22863d8 100755 > --- a/support/download/cvs > +++ b/support/download/cvs > @@ -21,7 +21,6 @@ basename="${4}" > output="${5}" > > repodir="${basename}.tmp-cvs-checkout" > -tmp_output="$( mktemp "${output}.XXXXXX" )" > > cd "${BUILD_DIR}" > # Remove leftovers from a previous failed run > @@ -36,6 +35,7 @@ rm -rf "${repodir}" > ret=1 > if ${CVS} -z3 -d":pserver:anonymous@${repo}" \ > co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then > + tmp_output="$( mktemp "${output}.XXXXXX" )" > if tar czf - "${repodir}" >"${tmp_output}"; then > mv "${tmp_output}" "${output}" > ret=0 > diff --git a/support/download/git b/support/download/git > index b0031e5..d3fcdaf 100755 > --- a/support/download/git > +++ b/support/download/git > @@ -19,8 +19,6 @@ basename="${3}" > output="${4}" > > repodir="${basename}.tmp-git-checkout" (not related to this patch) It's actually not a checkout, it's a bare clone. > -tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )" > -tmp_output="$( mktemp "${output}.XXXXXX" )" > > # Play tic-tac-toe with temp files > # - first, we download to a trashable location (the build-dir) > @@ -33,6 +31,8 @@ cd "${BUILD_DIR}" > # Remove leftovers from a previous failed run > rm -rf "${repodir}" > > +# Upon failure, git cleans behind itself, so no need to catch failures here. > +# The only case when git would not clean up, is if it gets killed with SIGKILL. I think the SIGKILL is reason enough to do the rm explicitly in the script. Of course, this is only valid if you use the trap, otherwise you never reach the rm (due to 'set -e'). Regards, Arnout > if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then > printf "Doing shallow clone\n" > ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" "${repodir}" > @@ -43,8 +43,10 @@ fi > > ret=1 > pushd "${repodir}" >/dev/null > +tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )" > if ${GIT} archive --prefix="${basename}/" --format=tar "${cset}" \ > >"${tmp_tar}"; then > + tmp_output="$( mktemp "${output}.XXXXXX" )" > if gzip -c "${tmp_tar}" >"${tmp_output}"; then > mv "${tmp_output}" "${output}" > ret=0 [snip]
Arnout, All, On 2014-07-07 18:08 +0200, Arnout Vandecappelle spake thusly: > On 06/07/14 23:27, Yann E. MORIN wrote: > > Create the temp file in the final location only when it is needed. > > > > This avoids the nasty experience of seeing lots of temp files in > > BR2_DL_DIR, that would linger around in case the downloads fails. > > It would also help to add a > > trap "rm -f ${tmp_dl}" EXIT HUP INT QUIT TERM > > which also removes the need of doing the rm at the end. Of course, that trap is > still racy (interrupt between mktemp and trap). > > But with the two temporary variables, adding a trap becomes even more > difficult. Unless that part is refactored into a separate helper script of > course :-) Yep, added to the list of TODO. ;-) > > Add a comment on why we don;t clean-up after git. > > Typo in don;t Yep, already fixed. [--SNIP--] > > diff --git a/support/download/git b/support/download/git > > index b0031e5..d3fcdaf 100755 > > --- a/support/download/git > > +++ b/support/download/git > > @@ -19,8 +19,6 @@ basename="${3}" > > output="${4}" > > > > repodir="${basename}.tmp-git-checkout" > > (not related to this patch) It's actually not a checkout, it's a bare clone. And it's being renamed in a later patch. > > -tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )" > > -tmp_output="$( mktemp "${output}.XXXXXX" )" > > > > # Play tic-tac-toe with temp files > > # - first, we download to a trashable location (the build-dir) > > @@ -33,6 +31,8 @@ cd "${BUILD_DIR}" > > # Remove leftovers from a previous failed run > > rm -rf "${repodir}" > > > > +# Upon failure, git cleans behind itself, so no need to catch failures here. > > +# The only case when git would not clean up, is if it gets killed with SIGKILL. > > I think the SIGKILL is reason enough to do the rm explicitly in the script. Of > course, this is only valid if you use the trap, otherwise you never reach the rm Well, SIGKILL is not catchable. That's the main selling point of SIGKILL. ;-) > (due to 'set -e'). Well, 'set -e' only kicks in for the final 'mv', since all other commands and conditions in an if statement. Also, I'm not a big fan of traps, although I can be abusing them from time to time. But OK, I'll add this to the TODO... Regards, Yann E. MORIN.
On 07/07/14 23:53, Yann E. MORIN wrote: > Arnout, All, > > On 2014-07-07 18:08 +0200, Arnout Vandecappelle spake thusly: >> On 06/07/14 23:27, Yann E. MORIN wrote: [snip] >>> +# Upon failure, git cleans behind itself, so no need to catch failures here. >>> +# The only case when git would not clean up, is if it gets killed with SIGKILL. >> >> I think the SIGKILL is reason enough to do the rm explicitly in the script. Of >> course, this is only valid if you use the trap, otherwise you never reach the rm > > Well, SIGKILL is not catchable. That's the main selling point of SIGKILL. > ;-) If git gets SIGKILLed, the shell script is still running. > >> (due to 'set -e'). > > Well, 'set -e' only kicks in for the final 'mv', since all other > commands and conditions in an if statement. The call to git clone isn't, though. > > Also, I'm not a big fan of traps, although I can be abusing them from > time to time. I do agree that trap is mightily confusing: it has obscure syntax, and you can easily accidentally replace an earlier trap. So in fact, it would be better not to use 'set -e' but to put everything in conditions. But that just moves the problem elsewhere... Regards, Arnout > > But OK, I'll add this to the TODO... > > Regards, > Yann E. MORIN. >
diff --git a/support/download/bzr b/support/download/bzr index f86fa82..a2cb440 100755 --- a/support/download/bzr +++ b/support/download/bzr @@ -17,7 +17,6 @@ rev="${2}" output="${3}" tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )" -tmp_output="$( mktemp "${output}.XXXXXX" )" # Play tic-tac-toe with temp files # - first, we download to a trashable location (the build-dir) @@ -27,6 +26,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )" ret=1 if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then + tmp_output="$( mktemp "${output}.XXXXXX" )" if cat "${tmp_dl}" >"${tmp_output}"; then mv "${tmp_output}" "${output}" ret=0 diff --git a/support/download/cvs b/support/download/cvs index a8ab080..22863d8 100755 --- a/support/download/cvs +++ b/support/download/cvs @@ -21,7 +21,6 @@ basename="${4}" output="${5}" repodir="${basename}.tmp-cvs-checkout" -tmp_output="$( mktemp "${output}.XXXXXX" )" cd "${BUILD_DIR}" # Remove leftovers from a previous failed run @@ -36,6 +35,7 @@ rm -rf "${repodir}" ret=1 if ${CVS} -z3 -d":pserver:anonymous@${repo}" \ co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then + tmp_output="$( mktemp "${output}.XXXXXX" )" if tar czf - "${repodir}" >"${tmp_output}"; then mv "${tmp_output}" "${output}" ret=0 diff --git a/support/download/git b/support/download/git index b0031e5..d3fcdaf 100755 --- a/support/download/git +++ b/support/download/git @@ -19,8 +19,6 @@ basename="${3}" output="${4}" repodir="${basename}.tmp-git-checkout" -tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )" -tmp_output="$( mktemp "${output}.XXXXXX" )" # Play tic-tac-toe with temp files # - first, we download to a trashable location (the build-dir) @@ -33,6 +31,8 @@ cd "${BUILD_DIR}" # Remove leftovers from a previous failed run rm -rf "${repodir}" +# Upon failure, git cleans behind itself, so no need to catch failures here. +# The only case when git would not clean up, is if it gets killed with SIGKILL. if [ -n "$(${GIT} ls-remote "${repo}" "${cset}" 2>&1)" ]; then printf "Doing shallow clone\n" ${GIT} clone --depth 1 -b "${cset}" --bare "${repo}" "${repodir}" @@ -43,8 +43,10 @@ fi ret=1 pushd "${repodir}" >/dev/null +tmp_tar="$( mktemp "${BUILD_DIR}/.XXXXXX" )" if ${GIT} archive --prefix="${basename}/" --format=tar "${cset}" \ >"${tmp_tar}"; then + tmp_output="$( mktemp "${output}.XXXXXX" )" if gzip -c "${tmp_tar}" >"${tmp_output}"; then mv "${tmp_output}" "${output}" ret=0 diff --git a/support/download/hg b/support/download/hg index 8b36db9..7e5c9eb 100755 --- a/support/download/hg +++ b/support/download/hg @@ -19,7 +19,6 @@ basename="${3}" output="${4}" repodir="${basename}.tmp-hg-checkout" -tmp_output="$( mktemp "${output}.XXXXXX" )" cd "${BUILD_DIR}" # Remove leftovers from a previous failed run @@ -33,6 +32,7 @@ rm -rf "${repodir}" ret=1 if ${HG} clone --noupdate --rev "${cset}" "${repo}" "${repodir}"; then + tmp_output="$( mktemp "${output}.XXXXXX" )" if ${HG} archive --repository "${repodir}" --type tgz \ --prefix "${basename}" --rev "${cset}" \ - >"${tmp_output}"; then diff --git a/support/download/scp b/support/download/scp index e16ece5..1cc18de 100755 --- a/support/download/scp +++ b/support/download/scp @@ -13,10 +13,10 @@ set -e url="${1}" output="${2}" tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )" -tmp_output="$( mktemp "${output}.XXXXXX" )" ret=1 if ${SCP} "${url}" "${tmp_dl}"; then + tmp_output="$( mktemp "${output}.XXXXXX" )" if cat "${tmp_dl}" >"${tmp_output}"; then mv "${tmp_output}" "${output}" ret=0 diff --git a/support/download/svn b/support/download/svn index 259d3ed..232d887 100755 --- a/support/download/svn +++ b/support/download/svn @@ -19,7 +19,6 @@ basename="${3}" output="${4}" repodir="${basename}.tmp-svn-checkout" -tmp_output="$( mktemp "${output}.XXXXXX" )" cd "${BUILD_DIR}" # Remove leftovers from a previous failed run @@ -33,6 +32,7 @@ rm -rf "${repodir}" ret=1 if ${SVN} export "${repo}@${rev}" "${repodir}"; then + tmp_output="$( mktemp "${output}.XXXXXX" )" if tar czf - "${repodir}" >"${tmp_output}"; then mv "${tmp_output}" "${output}" ret=0 diff --git a/support/download/wget b/support/download/wget index 0f71108..e961d71 100755 --- a/support/download/wget +++ b/support/download/wget @@ -15,7 +15,6 @@ url="${1}" output="${2}" tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )" -tmp_output="$( mktemp "${output}.XXXXXX" )" # Play tic-tac-toe with temp files # - first, we download to a trashable location (the build-dir) @@ -25,6 +24,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )" ret=1 if ${WGET} -O "${tmp_dl}" "${url}"; then + tmp_output="$( mktemp "${output}.XXXXXX" )" if cat "${tmp_dl}" >"${tmp_output}"; then mv "${tmp_output}" "${output}" ret=0
Create the temp file in the final location only when it is needed. This avoids the nasty experience of seeing lots of temp files in BR2_DL_DIR, that would linger around in case the downloads fails. Add a comment on why we don;t clean-up after git. Reported-by: Peter Korsgaard <jacmet@uclibc.org> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> --- support/download/bzr | 2 +- support/download/cvs | 2 +- support/download/git | 6 ++++-- support/download/hg | 2 +- support/download/scp | 2 +- support/download/svn | 2 +- support/download/wget | 2 +- 7 files changed, 10 insertions(+), 8 deletions(-)