Cleanups in download helpers (branch yem/check-downloads)

bzr uses the name of the extension of the output file to known what output format to use: tar, tgz, tar.bz2... If no extension is recognised, bzr will output to a directory.

Since we use 'mktemp .XXXXXX' to generate temporary files, it obviously never ends with a recognised extension. Thus, bzr expects the output to be a directory, and fails since it is a file.

Fix that by forcing the output format.

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 support/download/bzr | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

support/download/bzr +1 -1

index 3f52ee9..19d837d 100755
@@ -26,7 +26,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 # - finally, we atomically rename to the final file
 
 ret=1
-if ${BZR} export "${tmp_dl}" "${repo}" -r "${rev}"; then
+if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
     if mv "${tmp_dl}" "${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0

When using 'mv' to move files to temp files, the existing temp file is forcibly removed by 'mv', and a new file is created.

Although this should have little impact to us, there is still a race conditions in case two parallel downloads would step on each other's toes, and it goes like that:

Process 1 Process 2
mktemp tmp_output
mv tmp_dl tmp_output
removes tmp_output
mktemp tmp_ouput
writes to tmp_output
mv tmp_dl tmp_output
removes tmp_output writes to tmp_output
mv tmp_output output
mv tmp_output output

In this case, mktemp has the opportunity to create the same tmp_output temporary file, and we trash the content from one with the content from the other, and the last to do the final 'mv' breaks horibly.

Instead, use 'cat', which guarantees that tmp_output is not removed before writing to it.

This complexifies a bit the local-files (cp) helper, but better safe than sorry.

(Note: this is a purely theoretical issue, I did not observe it yet, but it is slated to happen one day or the other, and will be very hard to debug then.)

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 support/download/bzr  | 2 +-
 support/download/cp   | 9 ++++++---
 support/download/cvs  | 2 +-
 support/download/git  | 4 ++--
 support/download/hg   | 2 +-
 support/download/scp  | 2 +-
 support/download/svn  | 2 +-
 support/download/wget | 2 +-
 8 files changed, 14 insertions(+), 11 deletions(-)

support/download/bzr +0 -0

index 19d837d..f86fa82 100755

support/download/cp +0 -0

index 8f6bc06..e73159b 100755
@@ -13,12 +13,15 @@ set -e
 source="${1}"
 output="${2}"

support/download/cp +3 -0

+        mv "${tmp_output}" "${output}"
+        ret=0
+    fi
 fi
 
 # Cleanup

support/download/cvs +1 -1

index 9aeed79..a8ab080 100755
@@ -36,7 +36,7 @@ rm -rf "${repodir}"
 ret=1
 if ${CVS} -z3 -d":pserver:anonymous@${repo}" \
            co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then
-    if tar czf "${tmp_output}" "${repodir}"; then
+    if tar czf - "${repodir}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi

support/download/git +0 -0

index badb491..b0031e5 100755

support/download/hg +0 -0

         mv "${tmp_output}" "${output}"
         ret=0
     fi

support/download/scp +1 -1

index d3aad43..e16ece5 100755
@@ -17,7 +17,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
 if ${SCP} "${url}" "${tmp_dl}"; then
-    if mv "${tmp_dl}" "${tmp_output}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi

support/download/svn +0 -0

index 39cbbcb..259d3ed 100755

support/download/svn +0 -0

         mv "${tmp_output}" "${output}"
         ret=0
     fi

support/download/wget +1 -1

index cbeca3b..0f71108 100755
@@ -25,7 +25,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
 if ${WGET} -O "${tmp_dl}" "${url}"; then
-    if mv "${tmp_dl}" "${tmp_output}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi

Quoted 23 lines

When using 'mv' to move files to temp files, the existing temp file is forcibly removed by 'mv', and a new file is created.

Although this should have little impact to us, there is still a race conditions in case two parallel downloads would step on each other's toes, and it goes like that:

Process 1 Process 2
mktemp tmp_output
mv tmp_dl tmp_output
removes tmp_output
mktemp tmp_ouput
writes to tmp_output
mv tmp_dl tmp_output
removes tmp_output writes to tmp_output
mv tmp_output output
mv tmp_output output

In this case, mktemp has the opportunity to create the same tmp_output temporary file, and we trash the content from one with the content from the other, and the last to do the final 'mv' breaks horibly.

Quoted 2 lines

Instead, use 'cat', which guarantees that tmp_output is not removed before writing to it.

Not that it makes a real difference, but I think that 'cp' is a more natural
way to do this.

Quoted 6 lines

This complexifies a bit the local-files (cp) helper, but better safe than sorry.

(Note: this is a purely theoretical issue, I did not observe it yet, but it is slated to happen one day or the other, and will be very hard to debug then.)

Actually, since the mktemp string is based on time and PID, the chance of this
happening is really vanishingly small. That said, better safe than sorry.

Quoted 18 lines

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 support/download/bzr  | 2 +-
 support/download/cp   | 9 ++++++---
 support/download/cvs  | 2 +-
 support/download/git  | 4 ++--
 support/download/hg   | 2 +-
 support/download/scp  | 2 +-
 support/download/svn  | 2 +-
 support/download/wget | 2 +-
 8 files changed, 14 insertions(+), 11 deletions(-)

support/download/bzr +0 -0

index 19d837d..f86fa82 100755
@@ -27,7 +27,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 

support/download/bzr tmp_output="$( mktemp "${output}.XXXXXX" )" · line 33

Quoted 7 lines

support/download/bzr +1 -1

 ret=1
 if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
-    if mv "${tmp_dl}" "${tmp_output}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi

Not really related to this patch, but why do we need this ${tmp_dl} to begin
with? Especially since we're already "occupying" a tempfile in DL_DIR anyway.

Also, with this added complexity, the download helper scripts are becoming
quite similar. So wouldn't it be a good idea to refactor them?

I'm thinking of
something like this:

support/download/helper:

#!/bin/bash

# We want to catch any command failure, and exit immediately set -e

# Generic download helper
# Call it with:

#   $1: specific download helper
#   $2: output file
#   ...: arguments for the specific download helper

download="${1}"
output="${2}"
shift 2

tmp_output="$( mktemp ... )"
if "${download}" "${tmp_output}" "$@"; then

	# Blah blah need things to be atomic blah blah
	mv "${tmp_output}" "${output}"

fi

I expect there will be even more common stuff between the download helpers in
the future, so it makes sense to have this.

Quoted 11 lines

support/download/cp +1 -0

index 8f6bc06..e73159b 100755
@@ -13,12 +13,15 @@ set -e
 source="${1}"
 output="${2}"
 
+tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
 tmp_output="$( mktemp "${output}.XXXXXX" )"
 

support/download/cp set -e · line 21

Quoted 6 lines

support/download/cp +2 -3

 ret=1
-if ${LOCALFILES} "${source}" "${tmp_output}"; then
-    mv "${tmp_output}" "${output}"
-    ret=0
+if ${LOCALFILES} "${source}" "${tmp_dl}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then

Same here, what's the point of ${tmp_dl}?

Quoted 25 lines

support/download/cp +3 -0

+        mv "${tmp_output}" "${output}"
+        ret=0
+    fi
 fi
 
 # Cleanup

support/download/cvs +1 -1

index 9aeed79..a8ab080 100755
@@ -36,7 +36,7 @@ rm -rf "${repodir}"
 ret=1
 if ${CVS} -z3 -d":pserver:anonymous@${repo}" \
            co -d "${repodir}" -r ":${rev}" -P "${rawname}"; then
-    if tar czf "${tmp_output}" "${repodir}"; then
+    if tar czf - "${repodir}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi

support/download/git +0 -0

index badb491..b0031e5 100755
@@ -43,8 +43,8 @@ fi
 

support/download/git fi · line 47

Quoted 6 lines

support/download/git +2 -2

 ret=1
 pushd "${repodir}" >/dev/null
-if ${GIT} archive --prefix="${basename}/" -o "${tmp_tar}" \
-                  --format=tar "${cset}"; then
+if ${GIT} archive --prefix="${basename}/" --format=tar "${cset}" \
+                  >"${tmp_tar}"; then

What's the reason for this change? I've checked with strace, and git archive
doesn't seem to unlink, it just does open(O_TRUNC) like cp.

Quoted 8 lines

support/download/git +0 -0

     if gzip -c "${tmp_tar}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0

support/download/hg +0 -0

index 6e9e26b..8b36db9 100755
@@ -35,7 +35,7 @@ ret=1

support/download/hg ret=1 · line 38

Quoted 5 lines

support/download/hg +1 -1

 if ${HG} clone --noupdate --rev "${cset}" "${repo}" "${repodir}"; then
     if ${HG} archive --repository "${repodir}" --type tgz \
                      --prefix "${basename}" --rev "${cset}" \
-                     "${tmp_output}"; then
+                     - >"${tmp_output}"; then

I didn't check for hg, but I also don't expect it to unlink() first. In fact,
it's mv's behaviour of unlinking first that is surprising.

Quoted 22 lines

support/download/hg +0 -0

         mv "${tmp_output}" "${output}"
         ret=0
     fi

support/download/scp +1 -1

index d3aad43..e16ece5 100755
@@ -17,7 +17,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
 if ${SCP} "${url}" "${tmp_dl}"; then
-    if mv "${tmp_dl}" "${tmp_output}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi

support/download/svn +0 -0

index 39cbbcb..259d3ed 100755
@@ -33,7 +33,7 @@ rm -rf "${repodir}"
 

support/download/svn rm -rf "${repodir}" · line 36

Quoted 4 lines

support/download/svn +1 -1

 ret=1
 if ${SVN} export "${repo}@${rev}" "${repodir}"; then
-    if tar czf "${tmp_output}" "${repodir}"; then
+    if tar czf - "${repodir}" >"${tmp_output}"; then

Again, tar doesn't unlink so this isn't needed.

Arnout, All,

On 2014-07-07 08:11 +0200, Arnout Vandecappelle spake thusly:

Quoted 24 lines · Yann E. MORIN

Quoted 23 lines

When using 'mv' to move files to temp files, the existing temp file is forcibly removed by 'mv', and a new file is created.

Although this should have little impact to us, there is still a race conditions in case two parallel downloads would step on each other's toes, and it goes like that:

Process 1 Process 2
mktemp tmp_output
mv tmp_dl tmp_output
removes tmp_output
mktemp tmp_ouput
writes to tmp_output
mv tmp_dl tmp_output
removes tmp_output writes to tmp_output
mv tmp_output output
mv tmp_output output

In this case, mktemp has the opportunity to create the same tmp_output temporary file, and we trash the content from one with the content from the other, and the last to do the final 'mv' breaks horibly.

Quoted 5 lines

Quoted 2 lines

Instead, use 'cat', which guarantees that tmp_output is not removed before writing to it.

Not that it makes a real difference, but I think that 'cp' is a more natural
way to do this.

I am not sure how cp handles copying over an existing file. I'll check...

Quoted 8 lines

Quoted 7 lines

This complexifies a bit the local-files (cp) helper, but better safe than sorry.

(Note: this is a purely theoretical issue, I did not observe it yet, but it is slated to happen one day or the other, and will be very hard to debug then.)

Quoted 2 lines

Actually, since the mktemp string is based on time and PID, the chance of this
happening is really vanishingly small. That said, better safe than sorry.

Yes, the opportunity for a collision is very low, but it's not impossible. After all, that's the problem with race conditions: given sufficient time and trials, you'll hit it, and it is very hard to debug, especially in this case, since the condition is not reproducible (random names.)

Quoted 19 lines

Quoted 19 lines

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 support/download/bzr  | 2 +-
 support/download/cp   | 9 ++++++---
 support/download/cvs  | 2 +-
 support/download/git  | 4 ++--
 support/download/hg   | 2 +-
 support/download/scp  | 2 +-
 support/download/svn  | 2 +-
 support/download/wget | 2 +-
 8 files changed, 14 insertions(+), 11 deletions(-)

support/download/bzr +0 -0

index 19d837d..f86fa82 100755
@@ -27,7 +27,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 

Quoted 10 lines

support/download/bzr tmp_output="$( mktemp "${output}.XXXXXX" )" · line 33

Quoted 7 lines

support/download/bzr +1 -1

 ret=1
 if ${BZR} export --format=tgz "${tmp_dl}" "${repo}" -r "${rev}"; then
-    if mv "${tmp_dl}" "${tmp_output}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi

Not really related to this patch, but why do we need this ${tmp_dl} to begin
with? Especially since we're already "occupying" a tempfile in DL_DIR anyway.

The idea was not to pollute BR2_DL_DIR, in case the download fails. Hence this dance:
- download to a disposable area (BUILD_DIR);
- move to a temp file in BR2_DL_DIR;
- atomically rename to the final file.

But granted, since we remove failed downloads, we can skip the intermediary temp file in BUILD_DIR (although I still do not like much the idea of polluting BR2_DL_DIR.)

I think there is still a case for which we can't remove the temp file... Ah yes, if the user cancels a build with Ctrl-C, the script is aborted, so we leave a .XXXXXX file in BR2_DL_DIR.

But even with the three-step dance above, there is still a small window for leaving out cruft in BR2_DL_DIR; but that window is much, much smaller than the download window.

Quoted 3 lines

Also, with this added complexity, the download helper scripts are becoming
quite similar. So wouldn't it be a good idea to refactor them?

I think we already had this discussion back when I introduced the series:

    http://lists.busybox.net/pipermail/buildroot/2014-January/086826.html

But for now, there are a few important fixes I'd like be applied before going further.

Quoted 21 lines

I'm thinking of
something like this:

support/download/helper:

#!/bin/bash

# We want to catch any command failure, and exit immediately set -e

# Generic download helper
# Call it with:

#   $1: specific download helper
#   $2: output file
#   ...: arguments for the specific download helper

download="${1}"
output="${2}"
shift 2

Quoted 5 lines

tmp_output="$( mktemp ... )"
if "${download}" "${tmp_output}" "$@"; then

	# Blah blah need things to be atomic blah blah
	mv "${tmp_output}" "${output}"

fi

Oh, you meant having a wrapper script above all the other helpers, and delegate to the helpers only the download, and handle the atomicity dance to the wrapper. Hmmm... Might be worth looking at, probably, yes.

Quoted 3 lines

I expect there will be even more common stuff between the download helpers in
the future, so it makes sense to have this.

Well, for one, moving the hash check in that wrapper, for example...

Quoted 6 lines

Quoted 6 lines

support/download/git +0 -0

index badb491..b0031e5 100755
@@ -43,8 +43,8 @@ fi
 

Quoted 9 lines

support/download/git fi · line 47

Quoted 6 lines

support/download/git +2 -2

 ret=1
 pushd "${repodir}" >/dev/null
-if ${GIT} archive --prefix="${basename}/" -o "${tmp_tar}" \
-                  --format=tar "${cset}"; then
+if ${GIT} archive --prefix="${basename}/" --format=tar "${cset}" \
+                  >"${tmp_tar}"; then

What's the reason for this change? I've checked with strace, and git archive
doesn't seem to unlink, it just does open(O_TRUNC) like cp.

Ah, so you did strace both? I straced scp, and it does unlink. Are we sure all cp and all tar we can encounter will all use open(O_TRUNC) ?

It'd rather go on the safe side, and not assume much about this, especially since that behaviour may change in the future, or may not show in older versions, or different versions.

Quoted 17 lines

Quoted 9 lines

support/download/git +0 -0

     if gzip -c "${tmp_tar}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0

support/download/hg +0 -0

index 6e9e26b..8b36db9 100755

support/download/hg ret=1 · line 38

Quoted 5 lines

support/download/hg +1 -1

@@ -35,7 +35,7 @@ ret=1
 if ${HG} clone --noupdate --rev "${cset}" "${repo}" "${repodir}"; then
     if ${HG} archive --repository "${repodir}" --type tgz \
                      --prefix "${basename}" --rev "${cset}" \
-                     "${tmp_output}"; then
+                     - >"${tmp_output}"; then

I didn't check for hg, but I also don't expect it to unlink() first. In fact,
it's mv's behaviour of unlinking first that is surprising.

Well, scp also unlinks first.

And same answer, I don;t want to rely on such a behaviour. Redirecting works the same everywhere, though.

Quoted 22 lines

Quoted 22 lines

support/download/hg +0 -0

         mv "${tmp_output}" "${output}"
         ret=0
     fi

support/download/scp +1 -1

index d3aad43..e16ece5 100755
@@ -17,7 +17,7 @@ tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1
 if ${SCP} "${url}" "${tmp_dl}"; then
-    if mv "${tmp_dl}" "${tmp_output}"; then
+    if cat "${tmp_dl}" >"${tmp_output}"; then
         mv "${tmp_output}" "${output}"
         ret=0
     fi

support/download/svn +0 -0

index 39cbbcb..259d3ed 100755
@@ -33,7 +33,7 @@ rm -rf "${repodir}"
 

Quoted 6 lines

support/download/svn rm -rf "${repodir}" · line 36

Quoted 4 lines

support/download/svn +1 -1

 ret=1
 if ${SVN} export "${repo}@${rev}" "${repodir}"; then
-    if tar czf "${tmp_output}" "${repodir}"; then
+    if tar czf - "${repodir}" >"${tmp_output}"; then

Again, tar doesn't unlink so this isn't needed.

Ditto.

Regards,
Yann E. MORIN.

Quoted 1 line

On 2014-07-07 08:11 +0200, Arnout Vandecappelle spake thusly:

Quoted 5 lines

Quoted 2 lines

Not that it makes a real difference, but I think that 'cp' is a more natural
way to do this.

I am not sure how cp handles copying over an existing file. I'll check...

It does. It only unlinks if open(O_TRUNC) fails. (Checked with strace for
coreutils and my reading the source for busybox.)

Quoted 2 lines

Not really related to this patch, but why do we need this ${tmp_dl} to begin
with? Especially since we're already "occupying" a tempfile in DL_DIR anyway.

The idea was not to pollute BR2_DL_DIR, in case the download fails. Hence this dance:
- download to a disposable area (BUILD_DIR);
- move to a temp file in BR2_DL_DIR;
- atomically rename to the final file.

Quoted 9 lines

Quoted 3 lines

Not really related to this patch, but why do we need this ${tmp_dl} to begin
with? Especially since we're already "occupying" a tempfile in DL_DIR anyway.

The idea was not to pollute BR2_DL_DIR, in case the download fails. Hence this dance:
- download to a disposable area (BUILD_DIR);
- move to a temp file in BR2_DL_DIR;
- atomically rename to the final file.

And also because $(DL_DIR) and $(BUILD_DIR) might be on different filesystems, so the rename/move of the file from $(BUILD_DIR) (where it was downloaded) to $(DL_DIR) may not be atomic. Hence the idea is to:

- Download in $(BUILD_DIR)

- Move to a temporary file in $(DL_DIR). This operation may not be atomic if $(DL_DIR) is not on the same filesystem as $(BUILD_DIR)

- Finally rename the temporary file to the expected file name in $(DL_DIR). Since we're in the same directory, it's guaranteed to be atomic.

This allows to ensure that when the final file appears in $(DL_DIR), we're sure the download is finished, and that therefore concurrently executing Buildroot instances will either not see the downloaded file, or see a fully completed downloaded file.

Or am I missing the point of the discussion here?

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

Currently, the local-files download helper behaves like all the other download helpers, by first copying into the BUILD_DIR, then to BR2_DL_DIR, and then rename the final file.

This does two copies, for the sake of using the LOCALFILES command.

Just get rid of the intermediate copy to BUILD_DIR. Instead, directly copy to the final temp file and rename that.

This does a single copy, but we lose the file access mode, so we just reinstate them (in case it's a self-extracting executable used in a br2-external package, for example.)

Signed-off-by "Yann E. MORIN" <[email protected]>

+++ b/support/download/cp

support/download/cp +1 -2

@@ -8,20 +8,19 @@ set -e
 #   $1: source file
 #   $2: output file
 # And this environment:
-#   LOCALFILES: the cp command to call
+#   (nothing special)
 
 source="${1}"
 output="${2}"
 
-tmp_dl="$( mktemp "${BUILD_DIR}/.XXXXXX" )"
 tmp_output="$( mktemp "${output}.XXXXXX" )"
 
 ret=1

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 <[email protected]>

Signed-off-by "Yann E. MORIN" <[email protected]>

---
 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(-)

support/download/bzr +1 -1

index f86fa82..a2cb440 100755
@@ -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

support/download/cvs +1 -1

index a8ab080..22863d8 100755
@@ -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

support/download/git +0 -0

index b0031e5..d3fcdaf 100755

support/download/git +2 -0

 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

Quoted 4 lines

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 :-)

Quoted 1 line

Add a comment on why we don;t clean-up after git.

Typo in don;t

support/download/git basename="${3}" · line 21

Quoted 8 lines

support/download/git +0 -0

index b0031e5..d3fcdaf 100755
@@ -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.

Quoted 9 lines

support/download/git +0 -2

-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}"
 

support/download/git cd "${BUILD_DIR}" · line 35

Quoted 2 lines

support/download/git +2 -0

+# 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').

Arnout, All,

On 2014-07-07 18:08 +0200, Arnout Vandecappelle spake thusly:

Quoted 1 line · Yann E. MORIN

Quoted 5 lines

support/download/git cd "${BUILD_DIR}" · line 35

Quoted 2 lines

support/download/git +2 -0

+# 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. ;-)

Quoted 2 lines

(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.

Currently, we have temporary files to store 'downloaded' files or repositories, and they all are created using 'mktemp .XXXXXX'. Also, temporary repositories are named in a inconsistent manner.

This poses a problem in case something goes wrong: it is not possible to easily see what temp file corresponds to what attempted download.

Make all this a bit more homogeneous:
- name the temporary files and directories after the final file,

    with this mktemp pattern:   ${BUILD_DIR}/.${output##*/}.XXXXXX

This ensures it is easy to correlate a temp file or dir to the associated download attempt.

support/download/cvs +8 -7

@@ -20,28 +20,31 @@ rawname="${3}"
 basename="${4}"
 output="${5}"
 
-repodir="${basename}.tmp-cvs-checkout"
-
-cd "${BUILD_DIR}"
-# Remove leftovers from a previous failed run
-rm -rf "${repodir}"
-
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
 # - then we create a temporary tarball in the final location, so it is
 #   on the same filesystem as the final file
 # - finally, we atomically rename to the final file
 
+# Since cvs by itself is not capable of generating archives, we use
+# a little trick of tar, to transform the filenames as they are being
+# added to the archive, by replacing the leadin '.' with the basename
+# of the package. Note: basename is just that, a basename, so it does
+# not contain any '/', so we need not escape them for the transform
+# expression.
+
 ret=1
+repodir="$( mktemp -d "${BUILD_DIR}/.${output##*/}.XXXXXX" )"
 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

support/download/cvs +0 -0

 exit ${ret}

support/download/git +0 -2

index d3fcdaf..011c055 100755
@@ -18,8 +18,6 @@ cset="${2}"
 basename="${3}"
 output="${4}"
 
-repodir="${basename}.tmp-git-checkout"
-
 # Play tic-tac-toe with temp files
 # - first, we download to a trashable location (the build-dir)
 # - then we create the uncomporessed tarball in tht same trashable location

@@ -27,12 +25,11 @@ repodir="${basename}.tmp-git-checkout"
 #   it is on the same filesystem as the final file
 # - finally, we atomically rename to the final file

support/download/svn +0 -0

index 232d887..db98143 100755