Re: [Xen-devel] [PATCH v4 08/16] osstest: add support for the FreeBSD package manager

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

On Thu, Jul 06, 2017 at 04:12:08PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v4 08/16] osstest: add support for the 
> FreeBSD package manager"):
> > FreeBSD support is added to target_install_packages and
> > target_install_packages_norec, although there's no equivalent to the
> > --no-install-recommends in the FreeBSD package manager.
> 
> LGTM, just a couple of quibbles:
> 
> > +sub target_run_pkg {
> > +    my ($ho, @aptopts) = @_;
> > +    target_cmd_root($ho,"lockf /var/run/osstest-pkg-lock pkg-static 
> > @aptopts",
> 
> This line is rather long.
> 
> > +}
> >  sub target_install_packages {
> >      my ($ho, @packages) = @_;
> > -    target_run_apt($ho, qw(-y install), @packages);
> > +    my @hostflags = get_hostflags('host');
> > +
> > +    if (grep /^freebsd/i, @hostflags) {
> 
> I would prefer
> 
>   +    if (grep /^freebsd\b/i, @hostflags) {
>                          ^^
> 
> > +        target_run_pkg($ho, qw(install), @packages);
> > +    } else {
> > +        target_run_apt($ho, qw(-y install), @packages);
> > +    }
> 
> Also, target_install_packages and target_install_packages_norec are
> rather too similar for my taste.  If you can think of a better way of
> doing this please suggest one.  If not, then it's tolerable as it is.

I've changed it to:

sub package_install_cmd {
    my ($norec) = @_;
    my @hostflags = get_hostflags('host');
    my @cmd;

    if (grep /^freebsd\b/i, @hostflags) {
        push @cmd, qw(lockf /var/run/osstest-pkg-lock pkg-static install));
    } else {
        push @cmd, qw(DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y
                      with-lock-ex -w /var/lock/osstest-apt apt-get);
        if ($norec)
            push @cmd, qw(--no-install-recommends);
        push @cmd, qw(-y install);
    }

    return @cmd;
}
sub target_install_packages {
    my ($ho, @packages) = @_;
    my @cmd = package_install_cmd();

    target_cmd_root($ho,"@cmd @packages", 3000);
}
sub target_install_packages_norec {
    my ($ho, @packages) = @_;
    my @cmd = package_install_cmd(1);

    target_cmd_root($ho,"@cmd @packages", 3000);
}

(will test shortly). Let me know if that looks better.

Thanks, Roger.

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