Updated Plasma packages #253
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideSwitches Plasma desktop configuration from SDDM/X11 to LightDM/Wayland, aligns system time and NTP settings, adjusts kernel/sysctl and devfs permissions for user mounting, and sets up Polkit rules and X init defaults for the live user, root, and skeleton. Sequence diagram for LightDM login and Plasma Wayland session startupsequenceDiagram
actor User
participant LightDM
participant lightdm_conf as LightDM_config
participant xinitrc as Xinitrc
participant dbus_daemon as Dbus
participant ck_session as ConsoleKit
participant startplasma_wayland as Plasma
User->>LightDM: Enter credentials
LightDM->>lightdm_conf: Read greeter-session and user-session
lightdm_conf-->>LightDM: greeter-session=slick-greeter
lightdm_conf-->>LightDM: user-session=plasma
LightDM->>User: Start Plasma session
User->>xinitrc: Run startx (live_user/root/skel)
xinitrc->>xinitrc: setxkbmap us
xinitrc->>xinitrc: setxkbmap -option ctrl:swapcaps
xinitrc->>dbus_daemon: exec dbus-launch --exit-with-session
dbus_daemon->>ck_session: ck-launch-session
ck_session->>startplasma_wayland: startplasma-wayland
startplasma_wayland-->>User: Plasma Wayland desktop ready
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Several helpers (
update_rcconf_dm,configure_devfs,plasma_settings,set_localtime_from_bios) always append settings torc.conf,devfs.rules, andsysctl.confwithout checking for existing entries, which will lead to duplicate/conflicting lines when the script is rerun; consider normalizing/removing any existing keys before appending or usingsedto update in place as you did for some other options. lightdm_setupassumes${release}/usr/local/etc/lightdm/lightdm.confalready exists, sosed -iwill fail on a fresh install; add a guard to create a minimal config or skip updates if the file is missing.set_localtime_from_biosunconditionally forceslocaltimeto UTC and wipes any existingntpd_enable,ntpd_sync_on_start, andlocal_unbound_enablesettings, which can override user/system-specific configuration; consider making this behavior conditional or updating only the specific keys you need to change without deleting unrelated settings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several helpers (`update_rcconf_dm`, `configure_devfs`, `plasma_settings`, `set_localtime_from_bios`) always append settings to `rc.conf`, `devfs.rules`, and `sysctl.conf` without checking for existing entries, which will lead to duplicate/conflicting lines when the script is rerun; consider normalizing/removing any existing keys before appending or using `sed` to update in place as you did for some other options.
- `lightdm_setup` assumes `${release}/usr/local/etc/lightdm/lightdm.conf` already exists, so `sed -i` will fail on a fresh install; add a guard to create a minimal config or skip updates if the file is missing.
- `set_localtime_from_bios` unconditionally forces `localtime` to UTC and wipes any existing `ntpd_enable`, `ntpd_sync_on_start`, and `local_unbound_enable` settings, which can override user/system-specific configuration; consider making this behavior conditional or updating only the specific keys you need to change without deleting unrelated settings.
## Individual Comments
### Comment 1
<location> `desktop_config/plasma.sh:11-14` </location>
<code_context>
-
-[General]
-Numlock=on
+update_rcconf_dm() {
+ rc_conf="${release}/etc/rc.conf"
+ echo 'ntpdate_hosts="asia.pool.ntp.org"' >> "${rc_conf}"
+}
+
</code_context>
<issue_to_address>
**suggestion:** Make rc.conf updates idempotent to avoid accumulating duplicate ntpdate_hosts entries.
Because this script may run multiple times on an existing rc.conf, always appending the line can create duplicate ntpdate_hosts entries. Either delete any existing `^ntpdate_hosts=` lines (e.g. `sed -i '' '/^ntpdate_hosts=.*/d'`) before appending, or only append if `ntpdate_hosts="asia.pool.ntp.org"` is not already present. This keeps rc.conf idempotent and easier to maintain.
```suggestion
update_rcconf_dm() {
rc_conf="${release}/etc/rc.conf"
# Ensure rc.conf exists before we try to edit it
mkdir -p "$(dirname "${rc_conf}")"
touch "${rc_conf}"
# Remove any existing ntpdate_hosts lines to keep the setting idempotent
sed -i '' '/^ntpdate_hosts=.*/d' "${rc_conf}"
# Append the desired ntpdate_hosts configuration
echo 'ntpdate_hosts="asia.pool.ntp.org"' >> "${rc_conf}"
}
```
</issue_to_address>
### Comment 2
<location> `desktop_config/plasma.sh:31-40` </location>
<code_context>
+set_localtime_from_bios() {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align rc.conf ntpd/ntpdate writes with the deletion logic to avoid duplicate entries.
In `set_localtime_from_bios`, `ntpdate_enable` is only appended, so an existing `ntpdate_enable` line in rc.conf would remain, leading to conflicting/duplicate settings. Please remove any existing `^ntpdate_enable=` line before appending, matching the handling of the other keys. A shared helper for rc.conf mutations could also keep this logic consistently idempotent.
Suggested implementation:
```
rm -f "${tz_target}"
ln -s /usr/share/zoneinfo/UTC "${tz_target}"
# Ensure ntpdate_enable in rc.conf is set idempotently and without duplicates
if [ -f "${release}/etc/rc.conf" ]; then
# Remove any existing ntpdate_enable line before appending our desired setting
sed -i '' -e '/^ntpdate_enable=/d' "${release}/etc/rc.conf"
printf '%s\n' 'ntpdate_enable="YES"' >> "${release}/etc/rc.conf"
fi
```
If other time-sync related keys (e.g. `ntpd_enable`, `ntpd_sync_on_start`) are being set elsewhere in this script, consider factoring the repeated `sed`/append logic into a small helper, e.g.:
- A function like `set_rc_conf_kv key value` that:
1. `sed -i '' -e "/^${key}=*/d"` on `${release}/etc/rc.conf`
2. Appends `key="value"`.
Then call that helper from `set_localtime_from_bios` and any other places where `rc.conf` is mutated, to keep behavior consistent and idempotent.
</issue_to_address>
### Comment 3
<location> `desktop_config/plasma.sh:85-92` </location>
<code_context>
+
+}
+
+configure_devfs() {
+ devfs_rules="${release}/etc/devfs.rules"
+ rc_conf="${release}/etc/rc.conf"
+
+ echo '[localrules=10]' >> "${devfs_rules}"
+ echo "add path 'da*' mode 0666 group operator" >> "${devfs_rules}"
+ echo 'devfs_system_ruleset="localrules"' >> "${rc_conf}"
+}
+
</code_context>
<issue_to_address>
**suggestion:** Make devfs and rc.conf rule additions safe for multiple invocations.
Right now this always appends `[localrules=10]` and `devfs_system_ruleset="localrules"`. If the script runs more than once or the base image already contains these entries, you’ll get duplicate sections/lines. Please guard the writes (e.g. `grep -q '\[localrules=10\]' "${devfs_rules}" || echo '[localrules=10]' >> ...` and similarly for `devfs_system_ruleset`) so the config remains deterministic and free of duplicates.
```suggestion
configure_devfs() {
devfs_rules="${release}/etc/devfs.rules"
rc_conf="${release}/etc/rc.conf"
# Add localrules section only if it doesn't already exist
if ! grep -q '^\[localrules=10\]' "${devfs_rules}" 2>/dev/null; then
echo '[localrules=10]' >> "${devfs_rules}"
fi
# Add the da* rule only if it isn't already present
if ! grep -q "^[[:space:]]*add path 'da\*' mode 0666 group operator" "${devfs_rules}" 2>/dev/null; then
echo "add path 'da*' mode 0666 group operator" >> "${devfs_rules}"
fi
# Ensure devfs_system_ruleset is configured only once
if ! grep -q '^[[:space:]]*devfs_system_ruleset="localrules"' "${rc_conf}" 2>/dev/null; then
echo 'devfs_system_ruleset="localrules"' >> "${rc_conf}"
fi
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
desktop_config/plasma.sh
Outdated
Comment on lines
11
to
14
| update_rcconf_dm() { | ||
| rc_conf="${release}/etc/rc.conf" | ||
| echo 'ntpdate_hosts="asia.pool.ntp.org"' >> "${rc_conf}" | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion: Make rc.conf updates idempotent to avoid accumulating duplicate ntpdate_hosts entries.
Because this script may run multiple times on an existing rc.conf, always appending the line can create duplicate ntpdate_hosts entries. Either delete any existing ^ntpdate_hosts= lines (e.g. sed -i '' '/^ntpdate_hosts=.*/d') before appending, or only append if ntpdate_hosts="asia.pool.ntp.org" is not already present. This keeps rc.conf idempotent and easier to maintain.
Suggested change
| update_rcconf_dm() { | |
| rc_conf="${release}/etc/rc.conf" | |
| echo 'ntpdate_hosts="asia.pool.ntp.org"' >> "${rc_conf}" | |
| } | |
| update_rcconf_dm() { | |
| rc_conf="${release}/etc/rc.conf" | |
| # Ensure rc.conf exists before we try to edit it | |
| mkdir -p "$(dirname "${rc_conf}")" | |
| touch "${rc_conf}" | |
| # Remove any existing ntpdate_hosts lines to keep the setting idempotent | |
| sed -i '' '/^ntpdate_hosts=.*/d' "${rc_conf}" | |
| # Append the desired ntpdate_hosts configuration | |
| echo 'ntpdate_hosts="asia.pool.ntp.org"' >> "${rc_conf}" | |
| } |
desktop_config/plasma.sh
Outdated
Comment on lines
31
to
40
| set_localtime_from_bios() { | ||
| tz_target="${release}/etc/localtime" | ||
|
|
||
| grep -q "^\[Theme\]" "${sddm_conf}" || echo "[Theme]" >> "${sddm_conf}" | ||
| sed -i '' "s@^Current=.*@Current=breeze@" "${sddm_conf}" || echo "Current=breeze" >> "${sddm_conf}" | ||
| rm -f "${tz_target}" | ||
| ln -s /usr/share/zoneinfo/UTC "${tz_target}" | ||
|
|
||
| grep -q "^\[General\]" "${sddm_conf}" || echo "[General]" >> "${sddm_conf}" | ||
| sed -i '' "s@^Numlock=.*@Numlock=on@" "${sddm_conf}" || echo "Numlock=on" >> "${sddm_conf}" | ||
| fi | ||
| rc_conf="${release}/etc/rc.conf" | ||
| sed -i '' '/^ntpd_enable=.*/d' "${rc_conf}" 2>/dev/null || true | ||
| sed -i '' '/^ntpd_sync_on_start=.*/d' "${rc_conf}" 2>/dev/null || true | ||
| sed -i '' '/^local_unbound_enable=.*/d' "${rc_conf}" 2>/dev/null || true |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Align rc.conf ntpd/ntpdate writes with the deletion logic to avoid duplicate entries.
In set_localtime_from_bios, ntpdate_enable is only appended, so an existing ntpdate_enable line in rc.conf would remain, leading to conflicting/duplicate settings. Please remove any existing ^ntpdate_enable= line before appending, matching the handling of the other keys. A shared helper for rc.conf mutations could also keep this logic consistently idempotent.
Suggested implementation:
rm -f "${tz_target}"
ln -s /usr/share/zoneinfo/UTC "${tz_target}"
# Ensure ntpdate_enable in rc.conf is set idempotently and without duplicates
if [ -f "${release}/etc/rc.conf" ]; then
# Remove any existing ntpdate_enable line before appending our desired setting
sed -i '' -e '/^ntpdate_enable=/d' "${release}/etc/rc.conf"
printf '%s\n' 'ntpdate_enable="YES"' >> "${release}/etc/rc.conf"
fi
If other time-sync related keys (e.g. ntpd_enable, ntpd_sync_on_start) are being set elsewhere in this script, consider factoring the repeated sed/append logic into a small helper, e.g.:
- A function like
set_rc_conf_kv key valuethat:sed -i '' -e "/^${key}=*/d"on${release}/etc/rc.conf- Appends
key="value".
Then call that helper from set_localtime_from_bios and any other places where rc.conf is mutated, to keep behavior consistent and idempotent.
SunilSherkhane
force-pushed
the
master
branch
from
043db64 to
2b8bb45
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Switch the Plasma desktop configuration to use LightDM with a Wayland-based Plasma session while updating system time, device access, and core desktop settings.
New Features:
Bug Fixes:
Enhancements: