From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 5/5] make: Make "src-tarball" target actually make a source tarball Date: Thu, 17 Jul 2014 17:46:56 +0100 Message-ID: <53C7FE00.20109@eu.citrix.com> References: <1405354526-20929-1-git-send-email-george.dunlap@eu.citrix.com> <1405354526-20929-5-git-send-email-george.dunlap@eu.citrix.com> <21446.38146.96203.437720@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21446.38146.96203.437720@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: Anthony Perard , Ian Campbell , Jan Beulich , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 07/16/2014 04:06 PM, Ian Jackson wrote: > George Dunlap writes ("[PATCH 5/5] make: Make "src-tarball" target actually make a source tarball"): >> At the moment, making a release tarball is an annoyingly manual >> process that involves running "git archive" into a temporary directory. > ... >> +src-tarball: subtree-update >> + bash ./tools/misc/mktarball $(XEN_ROOT) $$(git describe) > Does this absolutely have to depend on subtree-update ? Perhaps we > need a new "make the subtrees exist" target ? I'll add a "subtree-find" command (perhaps renaming the other to "subtree-force-update"; then as per IanC's suggestion, I'll make this depend on subtree-force-update unless some sensible parameter is set. > >> diff --git a/tools/misc/mktarball b/tools/misc/mktarball > ... >> +function finish { >> + [[ -n "$tdir" ]] && rm -rf $tdir >> +} > The use of the POSIX syntax > finish() { > seems much more prevalent in-tree, than the use of `function'. Ack. > >> +trap finish EXIT > The EXIT trap handler should probably `set +e'. Ack. > >> +function git_archive_into { >> + mkdir "$2" >> + >> + git --git-dir="$1"/.git \ >> + archive --format=tar HEAD | \ >> + tar Cxf "$2" - >> +} >> + >> +if [[ -z "$1" || -z "$2" ]] ; then >> + echo "usage: $0 path-to-XEN_ROOT xen-version" >> + exit 1 >> +fi >> + >> +xen_root="$1" >> +desc="$2" >> + >> +mkdir -p $xen_root/dist/ >> + >> +tdir="$(mktemp -d $xen_root/dist/xen.XXXXXXXX)" > Why not use a fixed filename ? This script isn't safe for concurrent > invocation anyway, because the output filename is (mostly) fixed. I'm not so much worried about concurrent invocation per se, but about dealing with stuff possibly left over from a previous run. If we use a fixed path, we have to always rm -rf the path before starting, which seems unnecessarily risky to me. (Or we could not rm -rf the path, which is even worse from a consistency point of view.) Creating a new temporary directory just seems like the cleanest, safest thing to do. It's not like it's that expensive. (Re concurrent invocation: The output filename may collide, but at least you'll get a consistent snapshot of that one particular tree. What I'd be more worried about is racing with make subdir-force-update and getting inconsisent subtree archives.) > > If you do that you can do away with the trap handler entirely. You > should probably make git_archive_into delete the destination > directory. I'm not sure what the fixed filename has to do with cleanup -- I would want to remove the temporary directory on a failure regardless. > >> +GZIP=-9v tar cz -f $xen_root/dist/xen-$desc.tar.gz -C $tdir xen-$desc >> + >> +echo "Source tarball in $xen_root/dist/xen-$desc.tar.gz" >> \ No newline at end of file > No newline at end of file. Ack. -George