Re: git: 009d3f66cb5f - main - bsdinstall: separate out dist selection in prep for pkgbase support

On 31 Jan 2024, at 22:15, Jessica Clarke <jrtc27@FreeBSD.org> wrote:
> On 31 Jan 2024, at 22:05, Brad Davis <brd@FreeBSD.org> wrote:
>> 
>> The branch main has been updated by brd:
>> 
>> URL: https://cgit.FreeBSD.org/src/commit/?id=009d3f66cb5f0cf3f1d353f311d3a6878b2a534e
>> 
>> commit 009d3f66cb5f0cf3f1d353f311d3a6878b2a534e
>> Author:     Brad Davis <brd@FreeBSD.org>
>> AuthorDate: 2024-01-26 17:46:46 +0000
>> Commit:     Brad Davis <brd@FreeBSD.org>
>> CommitDate: 2024-01-31 22:05:27 +0000
>> 
>>   bsdinstall: separate out dist selection in prep for pkgbase support
>> 
>>   No functional change intended.
>> 
>>   Approved by:    asiciliano
>>   Sponsored by:   Rubicon Communications, LLC ("Netgate")
>>   Differential Revision:  https://reviews.freebsd.org/D43621
>> ---
>> usr.sbin/bsdinstall/scripts/auto        | 40 ++++--------------
>> usr.sbin/bsdinstall/scripts/selectdists | 73 +++++++++++++++++++++++++++++++++
>> usr.sbin/bsdinstall/startbsdinstall     |  1 +
>> 3 files changed, 82 insertions(+), 32 deletions(-)
>> 
>> diff --git a/usr.sbin/bsdinstall/scripts/auto b/usr.sbin/bsdinstall/scripts/auto
>> index 9f4b5b52fe5d..c651d654d62e 100755
>> --- a/usr.sbin/bsdinstall/scripts/auto
>> +++ b/usr.sbin/bsdinstall/scripts/auto
>> @@ -153,36 +153,10 @@ trap true SIGINT # This section is optional
>> trap error SIGINT # Catch cntrl-C here
>> if [ -z "$BSDINSTALL_SKIP_HOSTNAME" ]; then bsdinstall hostname || error "Set hostname failed"; fi
>> 
>> -export DISTRIBUTIONS="${DISTRIBUTIONS:-base.txz kernel.txz}"
>> -if [ -f $BSDINSTALL_DISTDIR/MANIFEST ]; then
>> - DISTMENU=`awk -F'\t' '!/^(kernel\.txz|base\.txz)/{print $1,$5,$6}' $BSDINSTALL_DISTDIR/MANIFEST`
>> - DISTMENU="$(echo ${DISTMENU} | sed -E 's/\.txz//g')"
>> -
>> - if [ -n "$DISTMENU" ]; then
>> - exec 5>&1
>> - EXTRA_DISTS=$( eval bsddialog \
>> -    --backtitle \"$OSNAME Installer\" \
>> -    --title \"Distribution Select\" --nocancel --separate-output \
>> -    --checklist \"Choose optional system components to install:\" \
>> -    0 0 0 $DISTMENU \
>> - 2>&1 1>&5 )
>> - for dist in $EXTRA_DISTS; do
>> - export DISTRIBUTIONS="$DISTRIBUTIONS $dist.txz"
>> - done
>> - fi
>> -fi
>> -
>> -FETCH_DISTRIBUTIONS=""
>> -for dist in $DISTRIBUTIONS; do
>> - if [ ! -f $BSDINSTALL_DISTDIR/$dist ]; then
>> - FETCH_DISTRIBUTIONS="$FETCH_DISTRIBUTIONS $dist"
>> - fi
>> -done
>> -
>> -if [ -n "$FETCH_DISTRIBUTIONS" -a -n "$BSDINSTALL_CONFIGCURRENT" ]; then
>> - bsddialog --backtitle "$OSNAME Installer" --title "Network Installation" --msgbox "Some installation files were not found on the boot volume. The next few screens will allow you to configure networking so that they can be downloaded from the Internet." 0 0
>> - bsdinstall netconfig || error
>> - NETCONFIG_DONE=yes
>> +if [ -n "$BSDINSTALL_USE_DISTRIBUTIONS" ]; then
>> + exec 5>&1
>> + export DISTRIBUTIONS=$( `dirname $0`/selectdists 2>&1 1>&5 )
>> + exec 5>&-
>> fi
>> 
>> rm -f $PATH_FSTAB
>> @@ -347,8 +321,10 @@ if [ -n "$FETCH_DISTRIBUTIONS" ]; then
>> 
>> [ $FETCH_RESULT -ne 0 ] && error "Could not fetch remote distributions"
>> fi
>> -bsdinstall checksum || error "Distribution checksum failed"
>> -bsdinstall distextract || error "Distribution extract failed"
>> +if [ -n "$BSDINSTALL_USE_DISTRIBUTIONS" ]; then
>> + bsdinstall checksum || error "Distribution checksum failed"
>> + bsdinstall distextract || error "Distribution extract failed"
>> +fi
>> 
>> # Set up boot loader
>> bsdinstall bootconfig || error "Failed to configure bootloader"
>> diff --git a/usr.sbin/bsdinstall/scripts/selectdists b/usr.sbin/bsdinstall/scripts/selectdists
>> new file mode 100644
>> index 000000000000..b548e82a95f8
>> --- /dev/null
>> +++ b/usr.sbin/bsdinstall/scripts/selectdists
>> @@ -0,0 +1,73 @@
>> +#!/bin/sh
>> +#-
>> +# Copyright (c) 2011 Nathan Whitehorn
>> +# Copyright (c) 2013-2018 Devin Teske
>> +# All rights reserved.
>> +#
>> +# Redistribution and use in source and binary forms, with or without
>> +# modification, are permitted provided that the following conditions
>> +# are met:
>> +# 1. Redistributions of source code must retain the above copyright
>> +#    notice, this list of conditions and the following disclaimer.
>> +# 2. Redistributions in binary form must reproduce the above copyright
>> +#    notice, this list of conditions and the following disclaimer in the
>> +#    documentation and/or other materials provided with the distribution.
>> +#
>> +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
>> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>> +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
>> +# ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
>> +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
>> +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
>> +# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
>> +# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
>> +# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
>> +# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>> +# SUCH DAMAGE.
>> +#
>> +#
>> +############################################################ INCLUDES
>> +
>> +BSDCFG_SHARE="/usr/share/bsdconfig"
>> +. $BSDCFG_SHARE/common.subr || exit 1
>> +
>> +############################################################ CONFIGURATION
>> +
>> +#
>> +# Default distributions
>> +#
>> +DISTRIBUTIONS="${DISTRIBUTIONS:-base.txz kernel.txz}"
>> +
>> +############################################################ MAIN
>> +
>> +if [ -f $BSDINSTALL_DISTDIR/MANIFEST ]; then
>> + DISTMENU=`awk -F'\t' '!/^(kernel\.txz|base\.txz)/{print $1,$5,$6}' $BSDINSTALL_DISTDIR/MANIFEST`
>> + DISTMENU="$(echo ${DISTMENU} | sed -E 's/\.txz//g')"
>> +
>> + if [ -n "$DISTMENU" ]; then
>> + EXTRA_DISTS=$( eval bsddialog \
>> + --backtitle \"$OSNAME Installer\" \
>> + --title \"Distribution Select\" --nocancel --separate-output \
>> + --checklist \"Choose optional system components to install:\" \
>> + 0 0 0 $DISTMENU \
>> + 2>&1 >&3 )
>> + for dist in $EXTRA_DISTS; do
>> + DISTRIBUTIONS="$DISTRIBUTIONS $dist.txz"
>> + done
>> + fi
>> +fi
>> +
>> +FETCH_DISTRIBUTIONS=""
>> +for dist in $DISTRIBUTIONS; do
>> + if [ ! -f $BSDINSTALL_DISTDIR/$dist ]; then
>> + FETCH_DISTRIBUTIONS="$FETCH_DISTRIBUTIONS $dist"
>> + fi
>> +done
>> +
>> +if [ -n "$FETCH_DISTRIBUTIONS" -a -n "$BSDINSTALL_CONFIGCURRENT" ]; then
>> + bsddialog --backtitle "$OSNAME Installer" --title "Network Installation" --msgbox "Some installation files were not found on the boot volume. The next few screens will allow you to configure networking so that they can be downloaded from the Internet." 0 0
>> + bsdinstall netconfig || error
>> + NETCONFIG_DONE=yes
>> +fi
>> +
>> +echo $DISTRIBUTIONS >&2
>> diff --git a/usr.sbin/bsdinstall/startbsdinstall b/usr.sbin/bsdinstall/startbsdinstall
>> index 63239c969ac6..8d9fb981c11d 100644
>> --- a/usr.sbin/bsdinstall/startbsdinstall
>> +++ b/usr.sbin/bsdinstall/startbsdinstall
>> @@ -6,6 +6,7 @@
>> : ${BSDDIALOG_EXTRA=3}
>> : ${BSDDIALOG_ESC=5}
>> : ${BSDDIALOG_ERROR=255}
>> +export BSDINSTALL_USE_DISTRIBUTIONS=y
> 
> I said it in the review and I’ll say it again here since you decided to
> just ignore me: this does not belong here. Please remove it and fix the
> default in selectdists.

Firstly, ping on this. I still object to the approach taken here.

Secondly, this commit was not at all tested. You do not install the new
selectdists script, and so the installer falls over (in a rather
cryptic way*). I am extremely unimpressed at ignoring reviewer comments
and completely breaking the installer, and thus am immediately
reverting this commit. It probably works if you install the script, but
it’s your job to test that, not mine, so when you have a patch that
actually works please re-seek review and actually engage with the
comments.

Jess

* The checksums script is fed garbage for DISTRIBUTIONS and gives:

    Error while extracting /usr/libexec/bsdinstall/auto:: Failed to open
    '/usr/freebsd-dist//usr/libexec/bsdinstall/auto:'

  since it’s interpreting the shell error message from auto of:

    /usr/libexec/bsdinstall/auto: /usr/libexec/bsdinstall/selectdists: not
    found

  as a space-separated list of distributions.