Re: [Xen-devel] [PATCH v4 09/16] osstest: introduce a FreeBSD build script

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Thu, Jul 06, 2017 at 04:25:24PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v4 09/16] osstest: introduce a FreeBSD build 
> script"):
> You have a spurious ", " at the end there.
> 
> In general, I notice that you sometimes add comments like this:
> 
>     # Reverse the neutron polarity
>     neutron_polarity_op(--reverse);

Right, it's too verbose maybe... I've added the logm to have some kind
of references to what's going on, but really it's a mess to find them
between so much output (the size of the build log file is ~100MB
IIRC).

> I won't insist on you removing any but in general I thought I'd say
> they aren't IMO particularly useful.
> 
> > +sub build_release($$$) {
> > +    my ($target, $prefix, $time) = @_;
> > +
> > +    buildcmd_stamped_logged_root($time, 'freebsd', "release-$target",
> > +                                 $prefix, <<END, '');
> > +make -C release $target
> > +END
> 
> Does this not want $makeflags ?  Mostly, this would be a -j.

No, those targets are (like) install targets, and will fail if called
with -j.

> > +    # Build process as documented in the handbook:
> > +    # https://www.freebsd.org/doc/handbook/updating-src.html
> 
> _This_ is a really good comment :-).
> 
> > +# Create a temporary fstab with the root dir
> > +echo '/dev/ufs/FreeBSD_Install / ufs rw 1 1' > etc/fstab
> 
> It's quite noticeable that there is a lot of code here that perhaps
> ought to be in some FreeBSD component.  (This is not a criticism of
> your osstest submission.)

Nods, I agree. There's a GSoC project currently ongoing to integrate
this functionality into the FreeBSD build process itself.

> > +    my $srcversion = target_cmd_output_root($ho, <<END, 30);
> > +awk '/^\\\#define[[:space:]]*__FreeBSD_version/ { print \$3 }' \\
> > +    $builddir/freebsd/sys/sys/param.h | cut -c1-2
> > +END
> 
> Cor.  Might it be better to use target_getfile and get_filecontents,
> and use a perl regexp ?

This line is basically the same used by the FreeBSD Makefile to get
the version number, that's why I've used it, but I don't like it, I
think it's fragile to regexp like that. I've changed this to:

    my $srcversion = target_cmd_output_root($ho, <<END, 30);
set -e
cd $builddir/freebsd
eval `make buildenvvars`
test -n "\$SRCRELDATE"
echo "\$SRCRELDATE" | cut -c1-2
END

Which I think it's more bullet-proof.

> > +    store_runvar("freebsd_buildversion", "$srcversion");
> > +
> > +    # Set path_freebsddist to point to the build output folder
> 
> Seems to be a unicode nonbreaking space after the # !

Ups.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel