Date: Sun, 04 Feb 2024 09:41:34 -0700 From: "Brad Davis" <brd@FreeBSD.org> To: "Jessica Clarke" <jrtc27@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 009d3f66cb5f - main - bsdinstall: separate out dist selection in prep for pkgbase support Message-ID: <49467837-dadd-4252-bfa7-169b0630bb41@app.fastmail.com> In-Reply-To: <B45A70E6-820C-46C4-968D-67916CFC0B14@freebsd.org> References: <202401312205.40VM5dQS018685@gitrepo.freebsd.org> <1D1F0A7A-C735-4D6F-B333-39920E84CD5D@freebsd.org> <B45A70E6-820C-46C4-968D-67916CFC0B14@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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: >>>=20 >>> The branch main has been updated by brd: >>>=20 >>> URL: https://cgit.FreeBSD.org/src/commit/?id=3D009d3f66cb5f0cf3f1d35= 3f311d3a6878b2a534e >>>=20 >>> 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 >>>=20 >>> bsdinstall: separate out dist selection in prep for pkgbase support >>>=20 >>> No functional change intended. >>>=20 >>> 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(-) >>>=20 >>> 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 || e= rror "Set hostname failed"; fi >>>=20 >>> -export DISTRIBUTIONS=3D"${DISTRIBUTIONS:-base.txz kernel.txz}" >>> -if [ -f $BSDINSTALL_DISTDIR/MANIFEST ]; then >>> - DISTMENU=3D`awk -F'\t' '!/^(kernel\.txz|base\.txz)/{print $1,$5,$6= }' $BSDINSTALL_DISTDIR/MANIFEST` >>> - DISTMENU=3D"$(echo ${DISTMENU} | sed -E 's/\.txz//g')" >>> - >>> - if [ -n "$DISTMENU" ]; then >>> - exec 5>&1 >>> - EXTRA_DISTS=3D$( 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=3D"$DISTRIBUTIONS $dist.txz" >>> - done >>> - fi >>> -fi >>> - >>> -FETCH_DISTRIBUTIONS=3D"" >>> -for dist in $DISTRIBUTIONS; do >>> - if [ ! -f $BSDINSTALL_DISTDIR/$dist ]; then >>> - FETCH_DISTRIBUTIONS=3D"$FETCH_DISTRIBUTIONS $dist" >>> - fi >>> -done >>> - >>> -if [ -n "$FETCH_DISTRIBUTIONS" -a -n "$BSDINSTALL_CONFIGCURRENT" ];= then >>> - bsddialog --backtitle "$OSNAME Installer" --title "Network Install= ation" --msgbox "Some installation files were not found on the boot volu= me. 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=3Dyes >>> +if [ -n "$BSDINSTALL_USE_DISTRIBUTIONS" ]; then >>> + exec 5>&1 >>> + export DISTRIBUTIONS=3D$( `dirname $0`/selectdists 2>&1 1>&5 ) >>> + exec 5>&- >>> fi >>>=20 >>> rm -f $PATH_FSTAB >>> @@ -347,8 +321,10 @@ if [ -n "$FETCH_DISTRIBUTIONS" ]; then >>>=20 >>> [ $FETCH_RESULT -ne 0 ] && error "Could not fetch remote distributio= ns" >>> 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 >>>=20 >>> # Set up boot loader >>> bsdinstall bootconfig || error "Failed to configure bootloader" >>> diff --git a/usr.sbin/bsdinstall/scripts/selectdists b/usr.sbin/bsdi= nstall/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 copyri= ght >>> +# notice, this list of conditions and the following disclaimer i= n the >>> +# documentation and/or other materials provided with the distrib= ution. >>> +# >>> +# 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 PARTICULA= R PURPOSE >>> +# ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE = LIABLE >>> +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONS= EQUENTIAL >>> +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE= GOODS >>> +# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPT= ION) >>> +# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRAC= T, STRICT >>> +# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN= ANY WAY >>> +# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILI= TY OF >>> +# SUCH DAMAGE. >>> +# >>> +# >>> +############################################################ INCLUD= ES >>> + >>> +BSDCFG_SHARE=3D"/usr/share/bsdconfig" >>> +. $BSDCFG_SHARE/common.subr || exit 1 >>> + >>> +############################################################ CONFIG= URATION >>> + >>> +# >>> +# Default distributions >>> +# >>> +DISTRIBUTIONS=3D"${DISTRIBUTIONS:-base.txz kernel.txz}" >>> + >>> +############################################################ MAIN >>> + >>> +if [ -f $BSDINSTALL_DISTDIR/MANIFEST ]; then >>> + DISTMENU=3D`awk -F'\t' '!/^(kernel\.txz|base\.txz)/{print $1,$5,$6= }' $BSDINSTALL_DISTDIR/MANIFEST` >>> + DISTMENU=3D"$(echo ${DISTMENU} | sed -E 's/\.txz//g')" >>> + >>> + if [ -n "$DISTMENU" ]; then >>> + EXTRA_DISTS=3D$( 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=3D"$DISTRIBUTIONS $dist.txz" >>> + done >>> + fi >>> +fi >>> + >>> +FETCH_DISTRIBUTIONS=3D"" >>> +for dist in $DISTRIBUTIONS; do >>> + if [ ! -f $BSDINSTALL_DISTDIR/$dist ]; then >>> + FETCH_DISTRIBUTIONS=3D"$FETCH_DISTRIBUTIONS $dist" >>> + fi >>> +done >>> + >>> +if [ -n "$FETCH_DISTRIBUTIONS" -a -n "$BSDINSTALL_CONFIGCURRENT" ];= then >>> + bsddialog --backtitle "$OSNAME Installer" --title "Network Install= ation" --msgbox "Some installation files were not found on the boot volu= me. 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=3Dyes >>> +fi >>> + >>> +echo $DISTRIBUTIONS >&2 >>> diff --git a/usr.sbin/bsdinstall/startbsdinstall b/usr.sbin/bsdinsta= ll/startbsdinstall >>> index 63239c969ac6..8d9fb981c11d 100644 >>> --- a/usr.sbin/bsdinstall/startbsdinstall >>> +++ b/usr.sbin/bsdinstall/startbsdinstall >>> @@ -6,6 +6,7 @@ >>> : ${BSDDIALOG_EXTRA=3D3} >>> : ${BSDDIALOG_ESC=3D5} >>> : ${BSDDIALOG_ERROR=3D255} >>> +export BSDINSTALL_USE_DISTRIBUTIONS=3Dy >>=20 >> I said it in the review and I=E2=80=99ll say it again here since you = decided to >> just ignore me: this does not belong here. Please remove it and fix t= he >> 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=E2=80=99s 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 th= at 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=3Dy = NOPKG=3Dy NOSRC=3Dy WITHOUT_DEBUG=3Dy # ls -al /usr/libexec/bsdinstall/selectdists=20 -r-xr-xr-x 1 root wheel 2882 Jan 31 21:37 /usr/libexec/bsdinstall/selec= tdists So I am not sure how it worked for me. Regards, Brad Davis
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?49467837-dadd-4252-bfa7-169b0630bb41>