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

On Fri, Feb 2, 2024, at 6:27 PM, Jessica Clarke wrote:
> 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.

I appreciate your feedback, but I explained why I did it that way and that wasn't good enough for you.

Sorry for the breakage.  I tested by rebuilding the memstick image over and over and installing a bhyve VM:

sudo make -C release clean && sudo make -C release memstick NOPORTS=y NOPKG=y NOSRC=y WITHOUT_DEBUG=y

# ls -al /usr/libexec/bsdinstall/selectdists 
-r-xr-xr-x  1 root wheel 2882 Jan 31 21:37 /usr/libexec/bsdinstall/selectdists

So I am not sure how it worked for me.


Regards,
Brad Davis