bsdinstall(8): add desktop script

[asiciliano]

Summary

Add a desktop script to install and set up a graphical environment. The current goal is to provide a simple option in bsdinstall to install the GPU driver, SDDM, and KDE. I have received some requests for additional functionality, such as allowing users to choose the display manager instead of SDDM due to potential future issues; it is not yet clear whether the KDE project will continue developing its main display manager without systemd.

Obviously, if there are requests, more options and graphical environments can be added in the future.

Sponsored by: The FreeBSD Foundation

Test Plan

The main testing challenge has been the large number of existing GPUs. I only have access to a recent laptop with an AMD GPU and an older 2011 laptop with NVIDIA Optimus (NVIDIA GPU + Intel GPU).

The call for testing has been very helpful, https://github.com/alfonsosiciliano/kde-installer-dialogs/blob/main/cft.md

[jrtc27]

I can't help but feel a lot of this belongs in packages rather than being big splats of hard-coded config that bsdinstall writes out that risks getting stale when ports gets updated

usr.sbin/bsdinstall/scripts/desktop

21

I did $3 >= 1000 && $3 < 32000 in CheriBSD, since there is an upper limit too above which you're back to system users.

56

Don't we force this non-empty elsewhere?

91

Arm + BIOS?..

[jrtc27]

usr.sbin/bsdinstall/scripts/desktop

93

I don't think this is true? You can get drm-kmod for arm64? (And drm-subtree if you vendor it in your tree)

[david_ixit.cz]

usr.sbin/bsdinstall/scripts/desktop

91

What I recall I saw some aarch64 board with BIOS, it's not extra common, but also not rare.

[arrowd]

usr.sbin/bsdinstall/scripts/desktop

106

IIRC, Optimus is not a correct term anymore. The thing is called "NVIDIA PRIME" now.

307

That's a rather large config file. How much is it different from the .sample one provided by the package?

354

pkg should create config.ini out of config.ini.sample automatically. Does it not?

363

These two knobs are not needed anymore.

424

Pulseaudio has its own way of autostarting. Is this really required?

[ziaee]

usr.sbin/bsdinstall/scripts/desktop

2–5

The hyphen was for a long abandoned parser, and we removed it from our style guides. Currently, for spdx only licenses the style guides suggest putting the copyright line above the spdx tag.

[taosecurity_gmail.com]

usr.sbin/bsdinstall/scripts/desktop

106

This is not correct.

NVIDIA Optimus and NVIDIA PRIME are different.

Optimus remains the official hardware brand name for NVIDIA's laptop graphics-switching technology. Modern versions are often called Advanced Optimus. It enables switching without reboots by using a hardware MUX switch.

PRIME (and the related nvidia-prime tool) is the specific implementation and set of scripts used to make Optimus technology work on Linux systems.

I wanted to make sure the original code remained and was not erroneously changed. Thank you!

[siva]

usr.sbin/bsdinstall/scripts/desktop

332

After a testing report from a user, it looks like this is also needed for pipewire audio on FreeBSD. It's not automatically installed by pipewire, since it's not technically a required dependency for the port itself, but since this installer is to be merged into freebsd base, we do need pipewire-spa-oss here.

[dteske]

@ziaee thank you for bringing this to my attention -- happy to review

[dteske]

See inline.

Also, globally replace:

case ... in
	pattern)
		...
		;;
esac

with

case ... in
pattern)
	...
	;;
esac

To be consistent with FreeBSD style and reduce unnecessary indentation, buying you critical width and maintaining clean code appearance.

usr.sbin/bsdinstall/scripts/desktop

11

I suggest also using UNAME_P and UNAME_R from common.subr, which means this comment should either be updated or removed

15–17

The double-quotes harden against DoS attack without affecting the parameter expansion.

Demonstration of attack:

export BSDDIALOG_OK="/*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/* /*/*/*/*/*/*/*/*/*/*"
bsdinstall desktop

The above, when $BSDDIALOG is expanded un-quoted ramps disk I/O by expanding the glob. While true that the : built-in successfully "eats" the positional arguments, it doesn't escape the glob-expansion. The net effect on systems with few files and a fast filesystem (e.g., NVMe) is a 10-20 second delay, but on systems with either tons of files or a slow filesystem, could be hours-long delay (and affect system performance in the process).

Yes, I am guilty of using unquoted parameter expansions in both bsdinstall and bsdconfig, and I am to fix that. We should harden this script before it hits the tree, for security sake.

19

Alternatively you can quote just $BSDINSTALL_CHROOT but that looks less natural.

21

Protect against NULL or unset value altering positional arguments resulting in pw -R usershow -a

Also, split long line to fit in 80c

23

$BSDCFG_SHARE/common.subr already stored this for you

24

You're already including $BSDCFG_SHARE/common.subr which means you already have the results of uname -r stuffed into UNAME_R global. Suggest using the global with parameter expansion to cut.

46

uname -r can be affected by export UNAME_r= and therefore VERSION could be made to expand to NULL which would cause this comparison to yield [: =: unexpected operator

Try it yourself with:

$ UNAME_r= /bin/sh -c 'v=`uname -r`; [ $v = v ]'
[: =: unexpected operator

54

If, by accident or maliciousness, BSDDIALOG_YES becomes anything but an integer acceptable to ['s -ne operator, then we want the exit 1 to trigger. That means flipping the conditional to utilize || instead of && -- stemming a potential POLA violation wherein the program keeps running with poisonous inherited value

56

In bsdinstall/scripts/hostname:

: ${HOSTNAME=$( hostname )}

Though that will allow NULL (which is common for an "amnesiac" build -- which is perfectly acceptable); though don't copy that exactly, as it would need hardening (with double-quotes added around the parameter expansion).

56

Suggest using global and then adding at the top:

: "${HOSTNAME=$( hostname )}"

58

Continued line should be indented past the parent line.

ASIDE: It shouldn't actually be fatal to have a NULL hostname (historically very well supported, and it is both internally [in code] and externally called "Amnesiac" build). Consider making non-fatal by removing this block unless there is specific reason that a NULL hostname is disallowed in the desktop circumstance.

LORE: Amnesiac builds are common when an integrator pre-images a machine before sending it to customer

76

Less fragile, more "correct"

85

Violates the POSIX "Rule of 4" and quoting

Once you pass 5 or more arguments (which happens the moment you chain more than one -a or -o operator), the POSIX standard states that the behavior is completely implementation-dependent. It explicitly notes that the results cannot be reliably predicted across different shells or operating systems.

Best to use && to make large chains like this.

90

POSIX "Rule of 4" and quoting

91

Quoting and fix incorrect string-comparison operator (= not ==)

94

POSIX "Rule of 4" and also quoting

96

Quoting and fix incorrect string-comparison operator (=, not ==)

98

Quoting

101

Quoting

104

POSIX "Rule of 4"

121

Quoting and fix incorrect string-comparison operator (=, not ==)

126

Quoting (may seem pedantic, but it guards against altering positional arguments to cause the following argument to be taken as the option-value in the event of a future edit introducing an error that results in a NULL value for any reason)

147

Quoting

197

POSIX "Rule of 4" and quoting *and* optimus is not properly initialized.

Find a good place to add optimus= somewhere to counter-balance.

Also, it's good to understand what is going on here. When optimus=1 you are NOT in-fact testing for boolean cast from integer. That-is, it is worth explaining that both of these tests return success:

1. [ 1 ] (returns $?=0)
2. [ 0 ] (also returns $?=0)

What you are in-fact doing with optimus=1 expansion is invoking the implicit -n test of a string, not casting an integer to a boolean.

That's perfectly fine when the dichotomy is either (a) optimus= or (b) optimus=1 because:

1. [ 1 ] returns $?=0 because 1 is a non-NULL string (in the parlance of [ -n 1 ]
2. [ ] for when optimus is NULL returns $?=1 (in the parlance of [ -n "" ]

199–203

Remove unnecessary line-continuation characters at the end of each line

217

Suggest quotes

218

Quotes

219

Remove unnecessary quotes for internal consistency

245

Quotes and POSIX "Rule of 4"

314

Quotes

329

Quotes

332

Quotes

353

Quotes

354

Quotes

355

Quotes

357

Quotes; prevent IFS word-splitting and instead execute code stored in variable as-is

367

Quotes and POSIX "Rule of 4"

371

On line 357 you evaluate gpuconfig which potentially has an append to loader.conf.desktop because of line 246, but here (after line 357) you have a overwrite of loader.conf.desktop.

It's not entirely clear that you intended this, and perhaps you meant to append instead of overwrite. I suspect that you intended to append, given that you explicitly unlink loader.conf.desktop at line 352 before both the potential creation/append on line 357 (influenced by line 246) and here at line 371.

376

Remove unnecessary braces around variable name for internal consistency

385

Quotes and POSIX "Rule of 4"

386

Quotes

389

Quotes

390

Quotes

394–405

Use <<- instead of << to increase readability here

409

Remove unnecessary quotes around var=$(...) as /bin/sh treats this (and var=[backtick]...[backtick]) specially in that IFS word-splitting and other transformations are foregone even without quoting.

Add quoting around $BSDINSTALL_CHROOT

Remove unnecessary braces around ${u}

410

Remove unnecessary braces around ${user_name}

ASIDE: In the C code, it requires more CPU cycles to process ${var} than $var and unless you are either using a parameter expansion feature or immediately abutting the variable name against a character that would be incorrectly taken as part of the variable name, braces should be avoided. It is also an issue with internal consistency.

411

Remove unnecessary braces

415

Remove unnecessary braces

419

Remove unnecessary braces

424

Remove unnecessary braces

425

Quotes and remove unnecessary braces

432

Remove unnecessary braces

434

Remove unnecessary braces

436

Quotes

[asiciliano]

Thanks to everyone for the reviews.
I'll update to fix all the reported issues. To keep things clean, I won't reply to every single comment; I'll only respond to a few to document the rationale for future development.

The Ly configuration file will be removed from this script, as it obviously belongs in the port. Probably the audio-related section of the script will also be temporarily removed and added back later.

I'll retest the script on the current CURRENT and STABLE branches, and update the NVIDIA drivers to the latest version.

[ngie]

I want to echo some of the concerns @jrtc27 had about these files being best suited for packages (or at the very least being distributed as small set of files for a short period of time until they can be incorporated into the installer) instead of being hardcoded in the installer -- otherwise you're chasing whatever the [current] defaults are in the upstream projects. Logic like this makes bsdinstall more difficult to maintain over time.
This level of complexity was part of how sysinstall bitrotted over time, so we should avoid this kind of anti-pattern as much as possible.
Back in the day there used to be a meta-port for X11 (then xorg) drivers. Can something like that be done to avoid a lot of the hardcoded logic? Also.. how about moving the driver selection logic into a separate/standalone script that's distributed with a package (as well). I could see something like that having a large degree of utility outside of bsdinstall for general purpose use.

usr.sbin/bsdinstall/scripts/desktop

121

== is permitted in /bin/sh these days, but I agree that it's best to use = for portability:

$ [ "foo" == "bar" ] || echo 'foo != bar'
foo != bar
$ [ "foo" == "foo" ] && echo 'foo is foo'
foo is foo
$ uname -r
15.1-RELEASE-p1

Imagine my [minor] surprise when I tried doing the above one-liners in ZSH and it didn't work out of the box :).

218

Quotes

Based on another review that @jilles gave me recently, the quotes might not be required here (I'll let them weigh in).

[dteske]

usr.sbin/bsdinstall/scripts/desktop

121

The principal of "don't prevent stupid lest you also prevent clever" applies in the sense that someone may think they are clever by back porting the code into an older release (for whatever reason) and using the tried-and-true portable = would ease their "cleverness" is how I choose to look at it. Also, to the previous point of whether this gets migrated to ports, then the point becomes even stronger as the port could be applied to releases older than CURRENT

218

Quotes

Based on another review that @jilles gave me recently, the quotes might not be required here (I'll let them weigh in).

Quotes still needed as far as I can see in HEAD, demonstrated by an intentional reliance on IFS word-splitting if you wanted to front-load test(1) conditions into the built-in as shown below:

$ sh -c 'foo="a = 1"; [ $foo ]; echo $?'
1
$ sh -c 'foo="a = a"; [ $foo ]; echo $?'
0
$ uname -r
16.0-CURRENT

If @jilles ever made quotes optional, then it would destroy the ability to do what was demonstrated immediately above

[ngie]

usr.sbin/bsdinstall/scripts/desktop

218

I was speaking specifically about case statements:

% cat ~/case_with_space.sh
#!/bin/sh

var1="this variable has spaces in it"

case ${var1} in
*"has spaces in it")
	echo matched as expected
	;;
*)
	echo did not match as expected
	;;
esac
% bash ~/case_with_space.sh
matched as expected
% zsh ~/case_with_space.sh
matched as expected
% sh case_with_space.sh
matched as expected
% uname -r
15.1-RELEASE-p1

[dteske]

usr.sbin/bsdinstall/scripts/desktop

218

This was rather surprising to me:

$ cat unquoted.sh 
var1="this        var has                consecutive spaces and tabs"
case $var1 in
*"this var"*) echo matched on this var ;;
*"has consecutive"*) echo matched on has consecutive ;;
*) echo no match
esac
$ sh unquoted.sh 
no match
$ bash unquoted.sh 
no match
$ zsh unquoted.sh 
no match

I even tested all the way back to 12.0-CURRENT.

But quotes also don't hurt.

They may even make it more readable/consistent.

[ngie]

usr.sbin/bsdinstall/scripts/desktop

218

If it's the existing style in the file, sure... if not, there's no harm in being consistent with whatever's there, e.g., omitting quotes in these statements.
FWIW, I've kind of changed my stance on style issues with python/sh/etc.. if shellcheck doesn't complain about there being an issue, I figure it's ok to not get too pedantic about something [1]:

% shellcheck -o all -x ~/case_with_space.sh
%

There are way more important things to care about. Using automated fixers that maintain a consistent style (black, clang-format, go fmt, ruff, etc) allows us to focus on what the code is doing more than how it's represented [2].

1. shellcheck goes a bit bonkers with some stylistic items like brace use with variables ($foo vs ${foo}), but I understand the advice that it's giving so I tend to agree with it since it helps cut down on noise from the tool.
2. I wish shellcheck had an automated fixer mode like black, ruff, etc, have.

[dteske]

usr.sbin/bsdinstall/scripts/desktop

218

@ngie — thank you for the clarification, and for bringing shellcheck / the auto-formatters into the discussion; that framing is useful to pin down.

Agreed that case is special: the subject after case does not undergo field splitting or pathname expansion, so $var and "$var" are equivalent for that construct. Where I want to push back gently is treating “shellcheck is quiet” / “be consistent with either quoted or unquoted” as a sufficient bar for what we land in bsdinstall.

Tools like shellcheck (and, in other languages, black / ruff / similar) are excellent at “is this syntactically / locally sound?” They are much less interested in what I think of as project-level correctness over time: future edits, multiple maintainers, backports, and keeping later diffs small. An unquoted expansion that is safe today (static one-word value, or a context like case that never splits) often becomes a trap the moment someone copies the pattern next to a test/[ arm, an external command, or a value that later grows whitespace or globs. Re-quoting then is pure review churn. Quoting by default — including in case, even though case itself is lenient — is the fail-safe that avoids that revisit. Consistency between quoting and non-quoting only “works” if you treat them as equal; they are not, outside that one builtin.

So: omit quotes only when you can guarantee the expansion is free of IFS/glob hazard and stay aware that case is the outlier that makes the hazard disappear. Prefer "$var" as the default habit so we do not spend velocity undoing edge cases later. Quotes here do not hurt readability, and they keep the script’s style aligned with the hardening we’re already asking for on the other expansions in this file.

On braces: I take the opposite of shellcheck’s usual preference. $foo is the baseline; ${foo} only when the grammar needs it (${foo#pat}, ${foo:-word}, abutment against a following word character, etc.). Inside braces the parser also has to consider the parameter-expansion operators; without them it does not. Always-bracing buys consistency with a linter’s noise floor, not efficiency or clarity in our tree. When the tool and the long-term shell style diverge on that point, I am fine declining the tool.

Happy for @jilles to correct any of the above if I have misstated /bin/sh’s rules — but on style for this script I would rather default to quotes-on / braces-only-when-needed than to “whatever shellcheck allows.”

[asiciliano]

Hi @ngie

You made some interesting points that probably deserve to be explored in more detail and in a different context.

In D56167#1335583, @ngie wrote:

I want to echo some of the concerns @jrtc27 had about these files being best suited for packages (or at the very least being distributed as small set of files for a short period of time until they can be incorporated into the installer) instead of being hardcoded in the installer -- otherwise you're chasing whatever the [current] defaults are in the upstream projects. Logic like this makes bsdinstall more difficult to maintain over time.
This level of complexity was part of how sysinstall bitrotted over time, so we should avoid this kind of anti-pattern as much as possible.

bsdinstall is probably already in a less-than-ideal state. I read the original 2011 version written by its author, and it was a simple, logical, and well-designed project.
Today we have hardcode, anti-patterns, and much more... :(
Currently, bsdinstall has over 130 open PRs, and the lists of feature requests from various user groups keep growing without solutions or fixes.
I think a positive and constructive discussion at a DevSummit would be very useful, not to find someone to blame or criticize past choices, but to understand how we got to this point from a very simple and logical installer, and to avoid repeating the same mistakes in the future with other projects.

Back in the day there used to be a meta-port for X11 (then xorg) drivers. Can something like that be done to avoid a lot of the hardcoded logic?

Ok, I can try to move the hardcoded logic into a utility within a new port, deskutils/bsdinstall-desktop. This way, the utility can write configuration files outside of /usr/local, such as in the users' home directories, /boot, etc. This could certainly be a good short-term solution.
However, I might not have fully understood your suggestion. Are you suggesting creating a bsdinstall-desktop meta-port? Can 'pkg install bsdinstall-desktop' write files outside of /usr/local? I'll ask around to see if something has changed with PkgBase.

Also.. how about moving the driver selection logic into a separate/standalone script that's distributed with a package (as well). I could see something like that having a large degree of utility outside of bsdinstall for general purpose use.

Ok, I actually mentioned this in the review description. If this script gets good feedback from users, it would be a good idea to decouple the GPU detection part and add a menu to select other desktop environments and graphical tools. People have asked me similar questions on social media, and I've posted some previews/mockups.

In the meantime, I'll add deskutils/bsdinstall-gpu to my TODO list.

Off-Topic

The Lua installer has already separated the GPU component from the desktop component. Architecturally, it is completely different from bsdinstall; among other features, it supports multiple modes. In Simple and Auto modes, the GPU screen is hidden from the user; the installer automatically handles the detection, installation, and configuration of the GPU. In Expert mode, the screen is displayed with the detected GPU pre-selected as a recommendation before the installation and configuration are applied.

The Lua installer will not rely on an external pkg/port for desktop configuration. In the long run, it's easier to maintain the little bit of configuration code inside the installer. Hardcoded values are kept in the configuration file, while hardware-dependent code will be isolated under the /machine/ directory.

Some time ago, I read a comment of yours on a PR where you mentioned it would be nice to have a Reset button/feature in the installer. This is an excellent idea, which is why I implemented it in the Lua installer, with [Prev] and [Next] buttons to navigate backward and forward through the screens.

Feel free to share any other interesting ideas; I would gladly try to implement them.

[jilles]

usr.sbin/bsdinstall/scripts/desktop

218

I don't see any threat to correctness or consistency. If the maintainer of these scripts wants to quote the word between case and in along with other locations where splitting is not expected, power to them.

The other review had something like load_rc_config $name elsewhere in the file, which was inconsistent.