On Fri, Feb 2, 2024, at 6:27 PM, Jessica Clarke wrote: > On 31 Jan 2024, at 22:15, Jessica Clarke <jrt...@freebsd.org> wrote: >> On 31 Jan 2024, at 22:05, Brad Davis <b...@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 <b...@freebsd.org> >>> AuthorDate: 2024-01-26 17:46:46 +0000 >>> Commit: Brad Davis <b...@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
