From nobody Sun Feb 4 17:15:09 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4TSbk25fWTz58Pjg for ; Sun, 4 Feb 2024 17:15:22 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com [209.85.221.51]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4TSbk23tyNz4pmS for ; Sun, 4 Feb 2024 17:15:22 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-wr1-f51.google.com with SMTP id ffacd0b85a97d-33b130f605eso2328946f8f.1 for ; Sun, 04 Feb 2024 09:15:22 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707066921; x=1707671721; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=oIAcfcgTkeiBFtvr8VTJ0O5Jy586/TX+edXsey3JODo=; b=peFeNBNEkxpl6ul+O8ctGYwMThVTI4LOuFpz8isqBXltKPvwGzzAyeWwKRU6eGF9C2 lF14wPlPqFlqhd4OO1tYpOxYaRxtlR0HgMxKBUAuRXgJu2Lg/h8xQpNpU3V9L/zABaSR 9RXKk0l1fqrO7Si3DKY1kUTxbvHYOoS86KRVMPjQ3F40L2MS7wjFLHIs9Hj9hrSi7WAz 41z6xRLVxY3fPfrDcnnIWF+66z9Q25Tr7lAolvD/gkdJGs8Vd1MbGiVfza4byyQlT2lw Jx4p5JdOVmZ+2yvCA9gDnXWfO7R06p9x/slvqEMJOKDOEmvSZxHsav/wFP3b59Cio8yB CUhw== X-Gm-Message-State: AOJu0YyQpZMGdU0EMDRokof3EeCCLcU37lzgYTsbGWnsz3s8i/2wLIs9 BmIhf7hCrPdolgg0qPg8h0ZzwRHdE5n8Q73gDCUVww6BUagUOYM8swr/cxZRxZ8= X-Google-Smtp-Source: AGHT+IHLfICq/w19DW3GnNGhgeAEuqUy9RwjTdtj2tVaWyBmfludadpbp4XCItbucC02YKrDR+bYQA== X-Received: by 2002:a05:6000:184c:b0:33b:3d7b:9df5 with SMTP id c12-20020a056000184c00b0033b3d7b9df5mr147643wri.3.1707066920675; Sun, 04 Feb 2024 09:15:20 -0800 (PST) X-Forwarded-Encrypted: i=0; AJvYcCXR7AS+G/o/SvKSk7UWN+7UAgaaE3/CzZiAL4OAFYiLrJAK4h2GvhNBXz+8dvqW8L/xHr73DJxpGRW0DssPi8PeocqWkK5vqMCklNqV4VbseuDGIJiQg7gU15KEJtq5skv7bJcsUxu+Wpnralv+aD2IRw== Received: from smtpclient.apple ([131.111.5.246]) by smtp.gmail.com with ESMTPSA id u7-20020adff887000000b0033b2221d938sm5555622wrp.76.2024.02.04.09.15.20 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 04 Feb 2024 09:15:20 -0800 (PST) Content-Type: text/plain; charset=utf-8 List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.200.91.1.1\)) Subject: Re: git: 009d3f66cb5f - main - bsdinstall: separate out dist selection in prep for pkgbase support From: Jessica Clarke In-Reply-To: <49467837-dadd-4252-bfa7-169b0630bb41@app.fastmail.com> Date: Sun, 4 Feb 2024 17:15:09 +0000 Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <1BF97C99-2AB2-44C5-B0C7-8FC441735748@freebsd.org> References: <202401312205.40VM5dQS018685@gitrepo.freebsd.org> <1D1F0A7A-C735-4D6F-B333-39920E84CD5D@freebsd.org> <49467837-dadd-4252-bfa7-169b0630bb41@app.fastmail.com> To: Brad Davis X-Mailer: Apple Mail (2.3774.200.91.1.1) X-Rspamd-Queue-Id: 4TSbk23tyNz4pmS X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US] On 4 Feb 2024, at 16:41, Brad Davis wrote: > On Fri, Feb 2, 2024, at 6:27 PM, Jessica Clarke wrote: >> On 31 Jan 2024, at 22:15, Jessica Clarke wrote: >>> On 31 Jan 2024, at 22:05, Brad Davis wrote: >>>>=20 >>>> The branch main has been updated by brd: >>>>=20 >>>> URL: = https://cgit.FreeBSD.org/src/commit/?id=3D009d3f66cb5f0cf3f1d353f311d3a687= 8b2a534e >>>>=20 >>>> commit 009d3f66cb5f0cf3f1d353f311d3a6878b2a534e >>>> Author: Brad Davis >>>> AuthorDate: 2024-01-26 17:46:46 +0000 >>>> Commit: Brad Davis >>>> 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 || = error "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 = 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=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 = 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 >>>>=20 >>>> # 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=3D"/usr/share/bsdconfig" >>>> +. $BSDCFG_SHARE/common.subr || exit 1 >>>> + >>>> +############################################################ = CONFIGURATION >>>> + >>>> +# >>>> +# 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 = 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=3Dyes >>>> +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=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 = the >>> default in selectdists. >>=20 >> Firstly, ping on this. I still object to the approach taken here. >>=20 >> 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. >=20 > I appreciate your feedback, but I explained why I did it that way and = that wasn't good enough for you. That=E2=80=99s 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: >=20 > sudo make -C release clean && sudo make -C release memstick NOPORTS=3Dy = NOPKG=3Dy NOSRC=3Dy WITHOUT_DEBUG=3Dy >=20 > # ls -al /usr/libexec/bsdinstall/selectdists=20 > -r-xr-xr-x 1 root wheel 2882 Jan 31 21:37 = /usr/libexec/bsdinstall/selectdists >=20 > 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=E2=80=99t normally let you run arc diff in that case... Jess