From nobody Mon Feb 5 21:59:12 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 4TTKzW6XVZz596GT; Mon, 5 Feb 2024 21:59:35 +0000 (UTC) (envelope-from brd@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4TTKzW601fz56nL; Mon, 5 Feb 2024 21:59:35 +0000 (UTC) (envelope-from brd@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1707170375; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8RAPmQO73QqH4gjcFViWR6vlwNPSXckqacN2+F9Xs+c=; b=Ku71oUoi1dU7TPmHO3XmLU90r2i9EBeX/dlf/CkA8ixPls8vdWvXMRbxWQkpfVoGN1Agz5 hZRZPY+Vvvfc8PSGIkFh634YHdC2kxeuDK6dOUn4EHeXqryYzvQPKLJsoHMkrr6URaAyhS Arr6YdSkQ7OyrTKRSot7TJEOFtMCVpa3NI95kXfZhcuDHquIAcBZ+fCtUijjNHDA7pp+0e a6//m+JfW9wdNmtUwc0aUtC8K0jrnOME0ezTF0muQkNQtMJz4Can1w5c9geTn0Z3/w/Ciz XqEM1Xdgn9qdQyH/TY1r2yD5Dw8VS2XHtGTu0lI1yIeeWFGbbrY/0kMr91vGNw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1707170375; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8RAPmQO73QqH4gjcFViWR6vlwNPSXckqacN2+F9Xs+c=; b=lwaSf7s/V3aKiTzqXq6aW6+JQTPqfMgApHSVkunAzAmCMN5wBPQHdB12R13EXZmhesXnAy Q+wqL++B0BHtzUcOiAJvHL5OlnC/B7/9QnNwQJ1bmdfl7ktpwwB2TN5GYrT+C5iSgolktO vVk/y5UnuvtweUFmKDopIoDJ4U/jVxE5MaqECOzxD2dPgHTtV0yXO8VQV83eJGhEPgVfWp /8CAmacOYvG3cPCIN4+Xeyo45XKXMbbRTRFP2WA3qNPAcjJJsuTy2Qg6GLgr0Itlh4oPU0 dbojC+HVijZ/fcDe5e+mkBPtDgusOLRHwFGl8tDbdxapue2NwKXR5okcqI3Y5w== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1707170375; a=rsa-sha256; cv=none; b=s6kxT3KM0L2OjxpK1Q26CCOE8RqfpXc64UMGt/3CPBSYC2m0ydn+Hl+3wec7LNyl027/RE 3frxCR1DXeL2UqojpzZFWlO/dC7X5nmArbXvDo5Z2gwFY0kO2O9mrMR8yI2Al8Qk5i0zVK sZvVrHb1UFlj9S1xnBWpEXlGzqM0Ey4Y5XWk3fgAcUB2flnvIKIUmnXCSNNGO5g0cg9oy8 j5981zpWoRCahzhlvbNAP8YqWMUUGl83YJ7Wjc6NWOysLHgudAlrBASTmjWKmSlvUCesE0 NGVz08loW7KR/c6Du4wMAtriw7BGJHvQ7yHhg2k4gV++vmBDD7gGYTGW0+NxIg== Received: from fauth2-smtp.messagingengine.com (fauth2-smtp.messagingengine.com [103.168.172.201]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: brd/mail) by smtp.freebsd.org (Postfix) with ESMTPSA id 4TTKzW4N7Zz1QFK; Mon, 5 Feb 2024 21:59:35 +0000 (UTC) (envelope-from brd@FreeBSD.org) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailfauth.nyi.internal (Postfix) with ESMTP id 9E4A01200066; Mon, 5 Feb 2024 16:59:34 -0500 (EST) Received: from imap48 ([10.202.2.98]) by compute1.internal (MEProxy); Mon, 05 Feb 2024 16:59:34 -0500 X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvkedrfedvuddgieefucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpefofgggkfgjfhffhffvvefutgfgse htqhertderreejnecuhfhrohhmpedfuehrrgguucffrghvihhsfdcuoegsrhgusefhrhgv vgeuufffrdhorhhgqeenucggtffrrghtthgvrhhnpeegjeeikeehgfffteelveefgfdtke ehleffieduffevheevveegjedtkedtleetveenucffohhmrghinhepfhhrvggvsghsugdr ohhrghenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpe gsrhgrugdomhgvshhmthhprghuthhhphgvrhhsohhnrghlihhthidqjedtjeeifedvfedv qddukedtieelieekkedqsghrugeppefhrhgvvgeuufffrdhorhhgsegsrhgruggurghvih hsrdhioh X-ME-Proxy: Feedback-ID: if7394599:Fastmail Received: by mailuser.nyi.internal (Postfix, from userid 501) id 1F6B831A0065; Mon, 5 Feb 2024 16:59:34 -0500 (EST) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.11.0-alpha0-144-ge5821d614e-fm-20240125.002-ge5821d61 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 Message-Id: In-Reply-To: <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> <1BF97C99-2AB2-44C5-B0C7-8FC441735748@freebsd.org> Date: Mon, 05 Feb 2024 14:59:12 -0700 From: "Brad Davis" To: "Jessica Clarke" Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Subject: Re: git: 009d3f66cb5f - main - bsdinstall: separate out dist selection in prep for pkgbase support Content-Type: text/plain;charset=utf-8 Content-Transfer-Encoding: quoted-printable 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=3D009d3f66cb5f0cf3f1d= 353f311d3a6878b2a534e >>>>>=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 suppo= rt >>>>>=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/bsdinstal= l/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 Insta= llation" --msgbox "Some installation files were not found on the boot vo= lume. The next few screens will allow you to configure networking so tha= t 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 distribut= ions" >>>>> 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/bs= dinstall/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 with= out >>>>> +# modification, are permitted provided that the following conditi= ons >>>>> +# are met: >>>>> +# 1. Redistributions of source code must retain the above copyrig= ht >>>>> +# notice, this list of conditions and the following disclaimer. >>>>> +# 2. Redistributions in binary form must reproduce the above copy= right >>>>> +# notice, this list of conditions and the following disclaimer= in the >>>>> +# documentation and/or other materials provided with the distr= ibution. >>>>> +# >>>>> +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS I= S'' AND >>>>> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED T= O, THE >>>>> +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICU= LAR PURPOSE >>>>> +# ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS B= E LIABLE >>>>> +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CO= NSEQUENTIAL >>>>> +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITU= TE GOODS >>>>> +# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRU= PTION) >>>>> +# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTR= ACT, STRICT >>>>> +# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING = IN ANY WAY >>>>> +# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBI= LITY OF >>>>> +# SUCH DAMAGE. >>>>> +# >>>>> +# >>>>> +############################################################ INCL= UDES >>>>> + >>>>> +BSDCFG_SHARE=3D"/usr/share/bsdconfig" >>>>> +. $BSDCFG_SHARE/common.subr || exit 1 >>>>> + >>>>> +############################################################ CONF= IGURATION >>>>> + >>>>> +# >>>>> +# 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 Insta= llation" --msgbox "Some installation files were not found on the boot vo= lume. The next few screens will allow you to configure networking so tha= t 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/bsdins= tall/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 yo= u 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 comme= nts >>> 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 pat= ch 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 disc= ussion: > > 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 loc= ation, just went off on a tangent unrelated to this change. I have no i= dea 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 add= ing pkgbase support in the future. I will endeavor to ask more question= s in the future. I will submit a new review taking this suggestion into account and fix t= he Makefile issue. >> Sorry for the breakage. I tested by rebuilding the memstick image ov= er and over and installing a bhyve VM: >>=20 >> sudo make -C release clean && sudo make -C release memstick NOPORTS=3D= y 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/se= lectdists >>=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... I missed it teasing out this part of the bigger change. Regards, Brad Davis