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

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v4 09/16] osstest: introduce a FreeBSD build 
script"):
> In order to generate the FreeBSD installer image and the install
> media.

> +sub install_deps () {
> +    target_install_packages($ho, 'git');

Please say

  +    target_install_packages($ho, qw(git));

> +    logm("Checkout the FreeBSD source tree");
> +    build_clone($ho, 'freebsd', $builddir, 'freebsd', );

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

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.

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

> +    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 ?

Your current approach means that:
 * if nothing matchines, you do not detect an error
 * you are unable to check that the putative version number
   has a plausible syntax
 * if something goes wrong, the param.h that you're parsing does
   not end up conveniently in a log (although I guess it's probably
   in build/ somewhere).

> +    store_runvar("freebsd_buildversion", "$srcversion");
> +
> +    # Set path_freebsddist to point to the build output folder

Seems to be a unicode nonbreaking space after the # !

Ian.

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