Details

Reviewers

imp
manu
emaste

Commits

rG03c07bdc8b31: bsdinstall: add menu to install firmware
rGcce4df8b3cbc: bsdinstall: wlanconfig: fix interface UP on (re-)starting wpa_supplicant
rG15f6edf20e4e: bsdinstall: add menu to install firmware
rG7d0c12e1f907: bsdinstall: wlanconfig: fix interface UP on (re-)starting wpa_supplicant
rGbbe2a1da2df6: bsdinstall: add menu to install firmware
rG5399052c63a7: bsdinstall: wlanconfig: fix interface UP on (re-)starting wpa_supplicant

Summary

Add a menu to the installer to run fwget(8) inside the newly installed
system to install firmware known to be needed.
This requires working netowrking.

This is needed ate least for wireless currently for when we entirely
stop shipping new firmware in src.git to have working networking on
the installed system (we already do need this for at least rtw89).

Sponsored by: The FreeBSD Foundation
Tested with: four different iwlwifi chipsets in a system
MFC after: 3 days

Test Plan

rebuilding, rebuilding install media, firing up bhyve, ...
manual testing is expensive...

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bz created this revision.Nov 9 2024, 1:27 AM

Herald added a subscriber: jrtc27. · View Herald TranscriptNov 9 2024, 1:27 AM

bz requested review of this revision.Nov 9 2024, 1:27 AM

Harbormaster completed remote builds in B60452: Diff 146188.Nov 9 2024, 1:27 AM

I have no idea where to report all the bugs or unexpected behavior I found during testing but it wasn't pleasant to use the installer again after a decade or so.

In case you know anyone else who could review this please add them!

Unless there'll be any comments I'll commit this around Tue 19.11. afternoon UTC

emaste added inline comments.Nov 18 2024, 2:00 PM

usr.sbin/bsdinstall/scripts/wlanconfig
58 ↗	(On Diff #146188)	this is an unrelated change?

bz added inline comments.Nov 18 2024, 5:59 PM

usr.sbin/bsdinstall/scripts/wlanconfig
58 ↗	(On Diff #146188)	Says so in the commit message; I can split it out any time.

emaste added inline comments.Nov 18 2024, 6:02 PM

usr.sbin/bsdinstall/scripts/wlanconfig
58 ↗	(On Diff #146188)	Says so in the commit message; I can split it out any time. Oh, so it does - I missed that. I'd suggest committing this part separately now; it LGTM.

jrtc27 added inline comments.Nov 18 2024, 6:07 PM

usr.sbin/bsdinstall/scripts/firmware
39	This seems like an annoying waste of time, at least for when it's called by scripts/auto?

bz added inline comments.Nov 18 2024, 6:37 PM

usr.sbin/bsdinstall/scripts/firmware
39	It stays for a blip and at least when doing interactive you get some kind of feedback and not just silently to the next menu wondering what happened (as can happen a lot in our installer and is rather annoying if you come to it as a not-used-to-it-user). Do we have "versions" to not call this for "auto" as in no using ${DIALOG} but something which skips this automatically for non-interactive versions?
usr.sbin/bsdinstall/scripts/wlanconfig
58 ↗	(On Diff #146188)	Will do. Thanks.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 19 2024, 12:08 AM

Closed by commit rG5399052c63a7: bsdinstall: wlanconfig: fix interface UP on (re-)starting wpa_supplicant (authored by bz). · Explain Why

This revision was automatically updated to reflect the committed changes.

bz marked 2 inline comments as done.

bz added a commit: rG5399052c63a7: bsdinstall: wlanconfig: fix interface UP on (re-)starting wpa_supplicant.

bz reopened this revision.Nov 19 2024, 12:11 AM

The wlanconfig change got committed as 5399052c63a7a3d2f54615d31bfd092ab887a600
and the description was locally removed from the commit message.

Harbormaster completed remote builds in B60675: Diff 146663.Nov 19 2024, 12:11 AM

bz edited the summary of this revision. (Show Details)Nov 19 2024, 12:11 AM

jrtc27 added inline comments.Nov 19 2024, 4:53 PM

usr.sbin/bsdinstall/scripts/firmware
39	I mean for the interactive version. I don't want to have to say ok to something that is expected to be the case a lot of the time. If someone explicitly selects it from finalconfig then sure, pop up a message saying there's nothing to do, but if all I want to do is install FreeBSD I don't want to be bombarded by things I don't need to know.

bz added inline comments.Nov 20 2024, 12:38 AM

usr.sbin/bsdinstall/scripts/firmware
39	It's an INFOBOX which doesn't have buttons. Ever tried to run that command by hand and see what happens? `dialog --backtitle "FOO" --infobox "BAR" 0 0`

bz marked 2 inline comments as done.Nov 20 2024, 12:39 AM

jrtc27 added inline comments.Nov 20 2024, 1:11 AM

usr.sbin/bsdinstall/scripts/firmware
39	I don't keep the details of all the various (bsd)dialog boxes in my head. But fine, it doesn't require user input. Doesn't that then mean it flashes and disappears when the next step (Add User Accounts) pops up a dialog? That is, it's almost entirely useless, unless you have a very slow serial connection?

bz marked an inline comment as done.Nov 20 2024, 1:21 AM

bz added inline comments.

usr.sbin/bsdinstall/scripts/firmware
39	I managed to see and read it when running the installer in bhyve at least on the serial console. Indeed. But any feedback is still better than no feedback as a user will wonder "what the bleep did happen" and our installer is already as userfriendly as the jokes there. As I asked, if there's a way to skip it in auto/scripted installs I'll do that to save you the 27ms but otherwise you keep changing your point as you go and need it seems and I keep repeating myself.

jrtc27 added inline comments.Nov 20 2024, 1:26 AM

usr.sbin/bsdinstall/scripts/firmware
39	I changed my point because you explained I had misunderstood what the code was doing. That doesn't mean I can't have a problem with both scenarios... I don't see why you have to respond to my technical objections by attacking me personally.

jrtc27 added inline comments.Nov 20 2024, 1:41 AM

usr.sbin/bsdinstall/scripts/firmware
39	Regardless, my point is, if you want this dialog to exist so the user actually knows what's happening, I would suggest using --pause to make the time it's displayed on screen independent of how long it takes the system to exit this script and display the next dialog.

bz marked an inline comment as done.Nov 20 2024, 1:46 AM

bz added inline comments.

usr.sbin/bsdinstall/scripts/firmware
39	How many seconds do you suggest to wait for someone to read the message who doesn't know what's coming?

jrtc27 added inline comments.Nov 20 2024, 1:55 AM

usr.sbin/bsdinstall/scripts/firmware
39	I'd suggest maybe 5-10s, with a more complete dialog: ${DIALOG} --title "${title}" --backtitle "${btitle}" \ --nocancel --pause "No firmware to install, continuing..." 0 0 5 That should be more than enough time to read those few words without being unnecessarily long, at least to me.
99	Missing --title "$DIALOG_TITLE"

Implement --pause with a 5 second delay as suggested by @jrtc27.

In addition we needed to calculate the size as otherwise the pause
bar was displayed over the text or partially over the text.
While doing that also added the hline with "Press OK to continue".

Add the missing --title "$DIALOG_TITLE".

Harbormaster completed remote builds in B60696: Diff 146732.Nov 20 2024, 2:22 AM

bz marked 3 inline comments as done.Nov 20 2024, 2:23 AM

bz added inline comments.