From nobody Mon Feb 5 22:11:22 2024 X-Original-To: dev-commits-src-main@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 4TTLFN14tSz597Jx for ; Mon, 5 Feb 2024 22:11:36 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) (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 4TTLFM35tFz57kQ for ; Mon, 5 Feb 2024 22:11:35 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-wm1-f46.google.com with SMTP id 5b1f17b1804b1-40fd72f7125so16061865e9.1 for ; Mon, 05 Feb 2024 14:11:35 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707171094; x=1707775894; 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=8DxFZY3xaI11khOlLv6yfzGVCow0lFEkITY97+WiUjs=; b=kBKlVlTkrnoTdVpCLJ83+Va9suQjLMzIr61XN2WQU8zCBWe/NPMErSMSdboBS3pICN Mw/gCzKZo9yxFFMGgL/I4+idMhAjvFWzDa//iDm1jMJ9sLl3FMmOdDS1R7stG7fBz7RX 0lI9cW5AUv0Ao5FJyvzG9xO/mVyDtICmC/rfHmjNnuDIfJBI/XVeUrcYgQbmeoIqoTjX SxbTmTii2g+ZCoEyJoaA/bfsg6HVSRpoxohUyQ2F/JocIXPtv2LnnsS29ZVCjiof6hYb YDRWx8FlEWLilYzFdajtqe22Inwwk1jcoCn23d009c1f53xnQyjd7djRQXdEoYdeE3XL F/4Q== X-Gm-Message-State: AOJu0Yz8/ABOlJgWlo1K9Vb+llhEhMMfNTPZKabQLlHYyaFU8++vxTWp fqMyXbSPaZQ10WkO5dEDJmd+plxhiWzyHzL2bKqSZqBDI8ZX65pNdK9iskIaGfWYH8Ap0b43HAX R X-Google-Smtp-Source: AGHT+IHRPXxGWEmEW//XI22uMX3wPqN/6GLRA6k+HeFJMX/2bnPuuln6qNSQFWADnrP2q5QkiPw64A== X-Received: by 2002:a05:600c:5491:b0:40f:c5cc:e756 with SMTP id iv17-20020a05600c549100b0040fc5cce756mr705237wmb.32.1707171093871; Mon, 05 Feb 2024 14:11:33 -0800 (PST) X-Forwarded-Encrypted: i=0; AJvYcCVJx/pBKYeSVm43rWg0BkmWbcyZbKCnUw2WqU8gzg3V4+YVNTEO142LbNXaZHNqRljPCFxLG3owtegnjSP0zMmhHUAzDiHJikqHdsFfw9T3g3LHXZ/hAFwTbVoDqavivIUhPWFwCcLFEbEmUDXqwC5LLw== Received: from smtpclient.apple ([131.111.5.246]) by smtp.gmail.com with ESMTPSA id o15-20020a05600c510f00b0040d5ae2906esm9947364wms.30.2024.02.05.14.11.33 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Feb 2024 14:11:33 -0800 (PST) Content-Type: text/plain; charset=utf-8 List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@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: Date: Mon, 5 Feb 2024 22:11:22 +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: References: <202401312205.40VM5dQS018685@gitrepo.freebsd.org> <1D1F0A7A-C735-4D6F-B333-39920E84CD5D@freebsd.org> <49467837-dadd-4252-bfa7-169b0630bb41@app.fastmail.com> <1BF97C99-2AB2-44C5-B0C7-8FC441735748@freebsd.org> To: Brad Davis X-Mailer: Apple Mail (2.3774.200.91.1.1) X-Rspamd-Queue-Id: 4TTLFM35tFz57kQ 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 5 Feb 2024, at 21:59, Brad Davis wrote: > On Sun, Feb 4, 2024, at 10:15 AM, Jessica Clarke wrote: >> 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. >>=20 >> That=E2=80=99s a rather misleading characterisation of what was a = discussion: >>=20 >> 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. >>=20 >> That was the limit of your explanation, with no response to the more >> detailed follow-ups refuting that explanation. >>=20 >> 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. >=20 > But you also didn't provide any constructive suggestions of a better = location, Because I provided the constructive suggestion that it should not exist and instead be the default in bsdinstall. > just went off on a tangent unrelated to this change. It was not unrelated. You said it wouldn=E2=80=99t be the default = behaviour in future. So I explained why even in that future where it=E2=80=99s no = longer the default, startbsdinstall still shouldn=E2=80=99t be overriding whatever bsdinstall=E2=80=99s default is, and that in a future where there is = more than one option in bsdinstall, that should be a user option, not hard-coded up in startbsdinstall. That is, in every possible future, the variable in question should never be set in startbsdinstall. That seems pretty related to me as a justification for my review comments? > 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. See above. > I will endeavor to ask more questions in the future. >=20 > I will submit a new review taking this suggestion into account and fix = the Makefile issue. Thank you. Jess >>> 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. >>=20 >> 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... >=20 > I missed it teasing out this part of the bigger change. >=20 >=20 > Regards, > Brad Davis