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

On Sun, Feb 4, 2024, at 10:15 AM, Jessica Clarke wrote:
> 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.

But you also didn't provide any constructive suggestions of a better location, just went off on a tangent unrelated to this change.  I have no idea why you brought up pkgbase as being the only way to install, as this in no way made pkgbase the default, but just starts making room for adding pkgbase support in the future.  I will endeavor to ask more questions in the future.

I will submit a new review taking this suggestion into account and fix the Makefile issue.

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

I missed it teasing out this part of the bigger change.


Regards,
Brad Davis