Details

Reviewers

emaste
ifreund_freebsdfoundation.org

Group Reviewers

releng

Commits

rGe554d44c3a7f: bsdinstall: Use package sets for pkgbase install
rG561dc357c2f5: bsdinstall: Use package sets for pkgbase install

Summary

Update the pkgbase component selection dialogue to match the meta sets
for the base system. Require either "base" or "minimal" be selected,
to produce a functional system. The other components are optional.

The "minimal" set is listed in the dialogue even though it's required,
to make it clear to users that they don't need to select "base", even
though this was required in previous releases.

By default, only "minimal" is selected. This is a departure from
previous FreeBSD releases, but allowing a minimal install is one of
the main benefits of pkgbase, so let's expose this in the installer.

Replace the various "dbg" options with a single "debug" component that
installs the debug symbols for all the components the user selected,
except for kernel since we handle that specifically, and it's common
to want kernel debugs symbols without userland debug symbols.

MFC after: 3 days

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 67123
Build 64006: arc lint + arc unit

Event Timeline

ivy created this revision.Mon, Sep 15, 8:08 PM

Herald added subscribers: jrtc27, imp. · View Herald TranscriptMon, Sep 15, 8:09 PM

ivy requested review of this revision.Mon, Sep 15, 8:09 PM

Harbormaster completed remote builds in B67058: Diff 162107.Mon, Sep 15, 8:09 PM

Please make base the default. bsdinstall defaults should be such that you can mash enter without thinking and get a sensible system. I suspect minimal is not what most want.

In D52558#1200053, @jrtc27 wrote:

Please make base the default. bsdinstall defaults should be such that you can mash enter without thinking and get a sensible system. I suspect minimal is not what most want.

i don't think that's actually true: if you install Debian, do you expect mashing enter to install every package they ship? should everyone get the NVMoF target by default? who even uses that?

i don't feel that strongly about this though, so if releng prefers base to be selected by default, i will change that.

emaste added inline comments.Mon, Sep 15, 8:32 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
80–86	Note to self, this form is needed because `kernel-dbg` is not a legal identifier.

It's very nice to reduce the amount of fragile string matching here!

The "minimal" set is listed in the dialogue even though it's required, to make it clear to users that they don't need to select "base", even though this was required in previous releases.

I'm not convinced that making both base and minimal selectable with an error when neither is selected is clearer. The user interface shouldn't make it possible for the user to select an invalid combination of choices from the list IMO. Being able to select both is also strange albeit unproblematic.

Assuming we make base the default again, we could try the following UX:

Don't list base in the component selection dialog.
List minimal in the dialog, unchecked by default.
If minimal is not selected by the user, install base
If minimal is selected by the user, install minimal rather than base

This would make it impossible for the user to select an invalid combination of options.

usr.sbin/bsdinstall/scripts/pkgbase.in
80–86	The motivation for this stylistic change seems to be displaying `kernel-dbg` rather than `kernel_dbg` to the user in the dialog. My original choice of `kernel_dbg` here was simply maintaining the status quo of FreeBSD 14 bsdinstall. I don't mind the style change though lumping it in with these other functional changes could have been avoided I guess.
83	Hmm, it looks like the base set also includes the `src` and `src-sys` packages currently. This doesn't seem ideal to me though, the `src` package is the biggest single pkgbase package and I think it makes sense for that to stay separate. This concern is somewhat orthogonal to your patch however, I'll open a patch adding a new set for the `src` and `src-sys` packages soon. The same goes for the `tests` packages.
92–98	Beyond changing the default from `base` to `minimal`, you've also changed the defaults for `kernel-dbg` and `lib32` compared to the behavior of the FreeBSD 14 installer. I don't have a strong opinion here myself, but perhaps it would be better to keep the defaults as close to FreeBSD 14 as possible in this patch to avoid blocking the technical improvements on bikeshedding defaults?
127	I presume the `#components[component] > 0` check here was removed due to the new behavior of the `debug` option in the dialog? I think that problem needs to be solved differently. Currently with your patch bsdinstall will list e.g. `src` as an option even if there is no `src` package available. This is relevant when performing an offline installation from the limited repo included on installation media for example.
211	I think we should fail an assertion if neither `base` nor `minimal` is present.

ifreund_freebsdfoundation.org added inline comments.Tue, Sep 16, 1:50 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
83	I opened https://reviews.freebsd.org/D52573 and https://reviews.freebsd.org/D52574

ivy added inline comments.Tue, Sep 16, 2:47 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
80–86	in my original version, i had all the debug sets listed (`minimal-dbg`, `base-dbg`, etc.) and wanted the displayed list entry to match the set name, so i changed `kernel_dbg` to `kernel-dbg` for consistency. then i replaced those with the single `debug` option, so this apparently-unrelated change was left over. i don't really mind what we call the options here: `kernel-dbg` isn't the name of a package or a set, so we could go back to `kernel_dbg`, but there is `kernel-generic-dbg`, which makes the `-dbg` variant feel a bit more natural to me.
83	once those are in i can update this diff to use the sets for those too.
92–98	aside from the base/minimal question, i do think we should disable lib32 by default nowadays... but that's is several changes which aren't actually required for this change, so for now i'll go back to matching the 14.x defaults and we can revisit this in a different discussion.

In D52558#1200313, @ifreund_freebsdfoundation.org wrote:

I'm not convinced that making both base and minimal selectable with an error when neither is selected is clearer.

i don't love either this approach or your proposed one, because neither of them really communicate the information we want to the user, which is that they always get minimal and could also choose base. what we really want is a greyed-out 'minimal' entry which is always selected, but i don't think bsddialog supports that.

Don't list base in the component selection dialog.

List minimal in the dialog, unchecked by default.

If minimal is not selected by the user, install base

If minimal is selected by the user, install minimal rather than base

i don't really like the idea that selecting more options installs less software, this feels unintuitive.

what about if we always install minimal and don't list that in the dialogue, then list base and check it by default, and at the same time, i'll add a bit more text to the dialogue to explain what's going on?

In D52558#1200444, @ivy wrote:

i don't really like the idea that selecting more options installs less software, this feels unintuitive.

That's a good point.

what about if we always install minimal and don't list that in the dialogue, then list base and check it by default, and at the same time, i'll add a bit more text to the dialogue to explain what's going on?

That sounds good to me, I think the main challenge is getting the wording concise and easy to understand :)

address review feedback

base, lib32 and kernel-dbg are enabled by default
now we have src and tests sets, build the entire components list dynamically based on which sets are available on the install media, except for "kernel" and "debug". this means if the install media doesn't have (say) the "src" set, it just won't be listed.
adjust the logic for the "debug" set to handle sets that don't have a -dbg subpackage (e.g., src).

i've tried to remove as much knowledge as possible about the available sets
from bsdinstall, e.g., we now only list the default sets we want enabled, not
all of them.

however, we still need to filter the list of sets we want to present to the
user, which i've done by only displaying sets which have a description. in
future we'll want to take the set description from the packages as well, but
that's out of scope for this diff.

Harbormaster completed remote builds in B67093: Diff 162188.Tue, Sep 16, 8:06 PM

minor fixes

fix a typo (no "||" in lua)
remove the explicit creation of the libcompat sets, so the installer doesn't fail if those aren't present on the media. this means we aren't using the value of all_libcompats at all, but i've left it in since it might be useful later.

Harbormaster completed remote builds in B67096: Diff 162191.Tue, Sep 16, 8:13 PM

Is the expectation that we ensure all release media have all supported package sets present in an offline package repo? Because disc1/memstick won't have the real src package, for example, and bootonly/mini-memstick won't have any real packages at all. So I think for this to work even bootonly/mini-memstick needs to have a package repo of the package sets (as a pkgbase analogue to how it still has the MANIFEST file that lists all distribution sets, just the sets themselves aren't present)?

jrtc27 added inline comments.Tue, Sep 16, 8:15 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
217	Maybe go stronger: "but we install minimal explicitly so removing base leaves the user with a minimal system"?

enable all libcompats by default

remove hardcoded "lib32" and instead default to installing all the sets listed
in all_libcompats. since "lib32" is the only libcompat in FreeBSD, this has
no functional impact, but it avoids an instance of hardcoding "lib32" which
we're trying to fix.

Harbormaster completed remote builds in B67101: Diff 162196.Tue, Sep 16, 8:17 PM

In D52558#1200643, @jrtc27 wrote:

Is the expectation that we ensure all release media have all supported package sets present in an offline package repo? Because disc1/memstick won't have the real src package, for example, and bootonly/mini-memstick won't have any real packages at all. So I think for this to work even bootonly/mini-memstick needs to have a package repo of the package sets (as a pkgbase analogue to how it still has the MANIFEST file that lists all distribution sets, just the sets themselves aren't present)?

for the purpose of this script (i.e., /usr/libexec/bsdinstall/pkgbase), the only requirement is that pkg(8) must be configured with a FreeBSD-base repository, and that repository must contain at least FreeBSD-set-minimal and FreeBSD-kernel-generic (or the equivalent name on powerpc64) which are the two packages required to install the system. it's fine if it doesn't contain src, tests, or even set-base; those options simply won't be listed in the component selection dialogue if they're missing.

i don't know how the actual media works here, but my assumption is that bootonly would include a pkg repository that pulls packages from pkg.freebsd.org; since that has all the sets, it should work fine. if there's media that has all of base but not src and src-sys, that's fine, as long as it also doesn't contain set-src (because that would create a broken dependency).

reword the comment about installing the minimal set to be more clear

while here, adjust the components asserts: we now assert that both minimal and
base are available. since it's impossible to build the system without the
base set, this would indicate something is wrong with the install media.

Harbormaster completed remote builds in B67105: Diff 162201.Tue, Sep 16, 8:46 PM

Looks reasonable to me

usr.sbin/bsdinstall/scripts/pkgbase.in
217	that sounds like removing base is a normal thing but most users won't do that, so maybe phrase it like "even if base is removed a minimal system will remain installed"?

ivy added inline comments.Tue, Sep 16, 8:56 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
217	since we select base by default now (for consistency with 14.x / dist sets) i suspect a lot of users will install FreeBSD, select the default options, learn more about pkgbase, and then remove the base set. so this might be more common than you'd think... (as an example: you have to remove the base set if you want to remove sendmail.)

emaste added inline comments.Tue, Sep 16, 9:01 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
217	Fair point and your updated text captures well what we want to convey.

In D52558#1200684, @ivy wrote:

i don't know how the actual media works here, but my assumption is that bootonly would include a pkg repository that pulls packages from pkg.freebsd.org; since that has all the sets, it should work fine. if there's media that has all of base but not src and src-sys, that's fine, as long as it also doesn't contain set-src (because that would create a broken dependency).

Currently, building installation media using release/Makefile builds packages for the target and copies select packages to a repository on the installation media.

The script used for this needs to be adjusted in line with your changes here, I can take care of that shortly.

We definitely need to test these changes in the case of an offline install from installation media before commiting them, I'm pretty sure that this will break things if it is landed before the logic in release/Makefile is updated.

The good news is that using package sets rather than selecting individual packages based on naming patterns significantly reduces the amount of duplicated logic :)

usr.sbin/bsdinstall/scripts/pkgbase.in
105	This doesn't make sense to me why is this done? It will also render the attempted support for libcompats other than lib32 pointless since they do not have descriptions hardcoded in this script and will therefore not be listed for selection.
127	This comment hasn't been addressed as far as I can tell.
144–146	There's a typo here, `s/selected/select/` I might also change the wording of the second sentence to "Select additional packages you wish to install:"
191	This code can be further simplified by making components a table mapping set name to package rather than set name to list of packages. With your current changes, no list will ever have more than one package.
220	This is redundant with the `local selected = {"minimal"}` line in `select_components()`.
222–229	This loop could be refactored away if `select_components()` were changed to return a table mapping component name to true (the lua idiom for a set) rather than a list. Then the debug variable would be pointless and could be replaced with a simple `selected["debug"]` in the condition below. I'd be happy to do this cleanup myself though after this patch is landed if that's easier.

ivy added inline comments.Wed, Sep 17, 12:46 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
105	because we now build the list of components dynamically from the available sets, but we don't want to display every set in the installer. for example, D52591 will add a "minimal-jail" set which isn't suitable for installing a host. libcompat support for compats other than lib32 is broken in pkgbase anyway (e.g., generate-ucl.lua always assumes lib32), so it's okay it still doesn't work here, i just wanted to avoid making it worse since i promised jrtc i would fix this properly at some point in the future.
127	i missed the comment, but the problem is fixed: if a set isn't on the media, it won't be listed in `components` at all, so we can never have an empty component. the only exception is the "kernel" component, but installing from media that doesn't contain a kernel is not going to work, and we assert later that a kernel is available.
191	i think it's useful to keep the ability for a component to have multiple packages in case we need it later for something, but i can redo that if you prefer.

ifreund_freebsdfoundation.org added inline comments.Wed, Sep 17, 12:47 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
210–211	The assertions no longer match the comment, and I don't think we should fail an assert if minimal is available but not base. I think the correct thing to do here is to simply drop the assertion for base and always assert that minimal is present.

ifreund_freebsdfoundation.org added inline comments.Wed, Sep 17, 1:01 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
105	Well, bsdinstall also supports installing to a jail using this script. In this case, the `no_kernel` option is set. It would make sense to allow or even default to `minimal_jail` in that case. Currently with your patch we have an ad-hoc whitelist (the hardcoded descriptions) and an ad-hoc blacklist (the no_kernel option). I think it would make sense to have only a blacklist, excluding the kernel packages if `no_kernel` is set and excluding minimal_jail if not.
127	I see, it would be good to have an assertion that the list cannot be empty here then.

ivy marked 8 inline comments as done.Wed, Sep 17, 1:11 PM

ivy added inline comments.

usr.sbin/bsdinstall/scripts/pkgbase.in
127	unfortunately this will fail for `debug` (the original reason for this change) since we don't populate that until we know what sets the user wants to install, so it's still empty at this point.

address minor review feedback

avoid adding minimal twice, and move the comment to where it's now set
fix a typo in the components dialogue
don't assert that the base set is available, since it's not required (although it is impossible to build the system without it, perhaps we might allow that in the future).

Harbormaster completed remote builds in B67120: Diff 162258.Wed, Sep 17, 1:14 PM

ifreund_freebsdfoundation.org added inline comments.Wed, Sep 17, 1:15 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
127	Ah OK, I guess the situation with debug would also make the refactor to eliminate extra list nesting a bit awkward.

ivy marked 2 inline comments as done.Wed, Sep 17, 1:29 PM

ivy added inline comments.

usr.sbin/bsdinstall/scripts/pkgbase.in
105	i think there are two related issues here: we should be taking the description from the package comment, not hardcoding it in the installer we need some sort of annotation on the set packages that indicates their target environment. i'm not sure what to call this, but perhaps "target=host" and "target=jail". minimal would have target=host, and minimal-jail would have target=jail. then, we can rework this to build the list of available sets completely dynamically (maybe except for kernel), and we'll just choose the appropriate target based on the no_kernel option. does that seem like a reasonable approach? however, for now, we might want to just hardcode logic for `-jail$` and `-dbg$`, since i'm not even sure you can get annotations with rquery.

ifreund_freebsdfoundation.org added inline comments.Wed, Sep 17, 1:59 PM

usr.sbin/bsdinstall/scripts/pkgbase.in
105	It's possible to get annotations with rquery using `%A`. With regards to the descriptions, I totally agree that that is a separate concern and that they should be obtained with `rquery %c` eventually. Since that is a separate problem, I think it would be good to stop using the hardcoded descriptions to also define a whitelist of components to display for selection. I have no problem with hardcoded logic for hiding `*-dbg` components in favor of the single global `debug` switch, I think that is the clearest possible way to implement that UX choice. For the jail/no_kernel case I also think hardcoding is fine. I'm not really sure if a generic solution using annotations is worthwhile or not.

only use a blacklist for the package selection dialogue

filter out packages we never want (e.g. -dbg$ which are handled separately),
and filter out -jail packages unless --no-kernel was specified.

this doesn't properly support the minimal-jail set since it hasn't landed yet,
so i'll address that in a separate review.

while here, change the empty component description from '' to an empty string,
since the quotes are passed verbatim and it looks a bit odd.

Harbormaster completed remote builds in B67123: Diff 162264.Wed, Sep 17, 2:25 PM

Thanks for the patience with my reviews, the code LGTM!

I haven't had a chance to test this out myself locally yet, will try to get to that tomorrow.

In D52558#1201065, @ifreund_freebsdfoundation.org wrote:

I haven't had a chance to test this out myself locally yet, will try to get to that tomorrow.

i was planning to build a release medium and test this properly before landing, but given D52592 i assume that won't actually work. do you mind testing both of them together?

ivy mentioned this in D52592: release: use sets to select base packages.Wed, Sep 17, 3:57 PM

I can't visualise the current edition of the dialogue, can someone provide a screenshot?

Thanks