[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 09/16] osstest: introduce a FreeBSD build script
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |