From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v5] OSSTEST: introduce a raisin build test Date: Wed, 13 May 2015 10:02:10 +0100 Message-ID: <1431507730.8263.214.camel@citrix.com> References: <1431450790-28686-1-git-send-email-stefano.stabellini@eu.citrix.com> <21842.14869.478809.933823@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: wei.liu2@citrix.com, Ian Jackson , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Tue, 2015-05-12 at 19:04 +0100, Stefano Stabellini wrote: > On Tue, 12 May 2015, Ian Jackson wrote: > > Stefano Stabellini writes ("[PATCH v5] OSSTEST: introduce a raisin build test"): > > > Signed-off-by: Stefano Stabellini > > ... > > > + echo >>config XEN_URL=\\"$r{tree_xen}\\" > > > + echo >>config XEN_REVISION=\\"$r{revision_xen}\\" > > > > This is very repetitive. In ts-xen-build, the names of the variables > > are irregular, but here they are regular. I think you should refactor > > this accordingly. > > I think that the cure here would be worse than the disease. In bash > would be fragile and difficult to read, but you probably would do it in > perl, right? In that case I don't know how it would look like; in fact I > wouldn't know how to write it. However if you are keen on it, feel free > to provide a snippet of code and I'll try to include it in the patch. Something like: foreach (qw(xen foo bar)) { (nonempty($r{tree_$_}) && nonempty($r{revision_$_}) ? <>config ".uc($_)."_URL=\\"$r{tree_$_}\\" echo >>config ".uc($_)."_REVISION=\\"$r{revision_$_}\\" +END } I think. > > > > I don't understand what the \\ are doing here. Perhaps you should use > > '' like in ts-xen-build ? > > I need to retain the " in the output If you use ' as your quote character then you do not need to escape uses of ". However I think that would stop $r{tree_$_} being evaluated too, so you would need to do like I did with uc above and use . to paste strings together, which may not be an overall improvement. > > > +sub divide () { > > > + # Only move hv to xeninstall, so that we can have > > > + # xenpolicy in tools tarball. > > > + # > > > + # The files inside boot/ after `make dist' are > > > + # xen-$XEN_VERSION: Xen binary > > > + # xen.gz/xen: symlink to xen-$XEN_VERSION > > > + # xen-$MAJOR: symlink to xen-$XEN_VERSION > > > + # xen-$MAJOR.$MINOR: symlink to xen-$XEN_VERSION > > > + # xen-sym-$XEN_VERSION: Xen symbol > > > + # xenpolicy-$XEN_VERSION: flask policy binary if xsm is enabled > > > + # > > > + # So the following snippet will leave xenpolicy* in > > > + # install/boot and get packaged to tools tarball. > > > + target_cmd_build($ho, 100, $builddir, < > > + cd raisin > > > + mkdir xendist > > > + for f in *dist; do > > > + mkdir -p \$f/lib > > > + done > > > + if test -d dist/boot; then > > > + if test -f dist/boot/xen.gz || test -f dist/boot/xen; then > > > + mkdir xendist/boot > > > + mvfiles=`find dist/boot -name 'xen[a-z]*' -prune -o -name 'xen*' -print` > > > + mv \$mvfiles xendist/boot/. > > > > This, and much of stash(), is a clone-and-hack of ts-xen-build. > > > > > +our @probs; > > > + > > > +sub trapping ($) { > > > + my ($sub) = @_; > > > + my $tok= eval { $sub->(); 1; }; > > > + if (!$tok) { push @probs, $@; print STDERR "failure (trapped): $@\n"; } > > > +} > > > > Again, this is copied from ts-xen-build. > > Yes, it is. If this is not a descriptive comment, what would you have me > do? I could move trapping somewhere else common, but I don't think that > generalizing divide and stash is a good idea. Osstest/Testsupport.pm is a suitable home for any random shared code I think, even if only shared between two ts-*. Or Osstest/Buildsupport.pm might be a better choice in this case. Thinking back to the issue about dividing the raisin build into per components though, perhaps use_raisin should become a runvar which is used in ts-*-build to either do the existing thing vs the new raisin thing? Then stash+divide etc would only be used in the one place, just consuming a potentially different dist dir output from the build phase. Ian.