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

On 4 Feb 2024, at 16:41, Brad Davis <brd@FreeBSD.org> wrote:
> 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.

That’s a rather misleading characterisation of what was a discussion:

jrtc27:
This seems pretty strange to do here. Why isn't it the default?
brd:
Because in the near future it won't be the default
jrtc27:
Then change the default when the default should change? This doesn't
belong here. Besides, I'd expect a transitional period where there's a
menu asking which you'd like; pkgbase is coming along but it still
seems like it isn't quite battle-tested enough to be the only way to
install FreeBSD.
emaste:
What I'd like to do is have a menu in the boot loader that sets a
variable for experimental features, so that it can be available in
snapshots but still somewhat hidden
jrtc27:
That seems reasonable. But that still doesn't make this line belong
here.

That was the limit of your explanation, with no response to the more
detailed follow-ups refuting that explanation.

If you want more of a technical justification that just what is in my
view an ugly design, startbsdinstall is meant to just be a wrapper
around bsdinstall that provides the install media-specific behaviour,
with bsdinstall itself being usable standalone as a program you can
just run. Therefore any default settings like this one need to be set
inside bsdinstall, not startbsdinstall.

> 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.

Who knows. Maybe you forgot to stage one hunk (though you likely still
missed OptionalObsoleteFiles.inc, which is often forgotten). But arc
doesn’t normally let you run arc diff in that case...

Jess