ti: Add more firmware, GPU etc packages #8375
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughBoard configs for SK‑AM62B and SK‑AM64B were updated to declare SOC_ID and GPU/firmware/video-related settings; the K3 family source config ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/bsp/ti/weston/weston.socket (1)
8-9: Consider uncommenting user/group settings for security.Running Weston as root (implied by the commented user/group settings) may not be ideal from a security perspective. Consider uncommenting and using dedicated users if they exist on the system.
extensions/ti-debpkgs.sh (1)
45-45: Consider improving error handling for service enablement.The current error handling only logs an alert if
systemctl enablefails. Consider whether this should be a fatal error or if the alert provides sufficient information for debugging.packages/bsp/ti/weston/weston.service (1)
40-40: Consider making Weston backend configurableThe service hard-codes the DRM backend with specific options. Consider making the backend and options configurable through the environment file for better flexibility across different hardware configurations.
-ExecStart=/usr/bin/weston --backend=drm-backend.so --continue-without-input --idle-time=0 +ExecStart=/usr/bin/weston $WESTON_OPTIONSThen define
WESTON_OPTIONSin/etc/default/weston.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
config/boards/sk-am62b.conf(1 hunks)config/boards/sk-am64b.conf(1 hunks)config/sources/families/k3.conf(1 hunks)extensions/ti-debpkgs.sh(1 hunks)packages/bsp/ti/weston/weston(1 hunks)packages/bsp/ti/weston/weston.service(1 hunks)packages/bsp/ti/weston/weston.socket(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: leggewie
PR: armbian/build#8357
File: config/boards/qcom-robotics-rb5.conf:88-88
Timestamp: 2025-07-04T18:33:19.873Z
Learning: In the Armbian build system, package transitions in Debian/Ubuntu don't happen uniformly across all releases. For example, the transition from plasma-workspace-wayland to plasma-workspace occurred in newer releases like trixie (Debian testing) around July 2024, but older releases like jammy (Ubuntu 22.04) still maintain the original plasma-workspace-wayland package. Release-specific package availability must be considered when updating package references.
Learnt from: libiunc
PR: armbian/build#8033
File: config/kernel/linux-starfive2-vendor.config:43-43
Timestamp: 2025-03-31T12:57:13.880Z
Learning: For StarFive2 platform kernel configurations, maintain alignment with vendor-provided configurations rather than modifying security settings like SECCOMP. This ensures hardware compatibility as intended by the manufacturer.
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-06-16T03:24:00.458Z
Learning: In Armbian board configuration files (.csc), the standard pattern is to have only one line as a comment describing the board hardware specifications. This single-line description typically includes the SoC model, core count, RAM options, and key features like connectivity options, storage interfaces, and special features.
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Learnt from: pyavitz
PR: armbian/build#8361
File: config/kernel/linux-sunxi64-edge.config:594-597
Timestamp: 2025-07-06T16:27:45.349Z
Learning: In Armbian sunxi64-edge kernel configurations, PCI/PCIe support may be enabled for future-proofing purposes to support upcoming Allwinner SoCs like the T536 that include PCIe Gen2 interfaces, even if current primary targets like H616 lack PCIe controllers. This forward-looking approach prepares the configuration for next-generation hardware in the same SoC family.
Learnt from: Grippy98
PR: armbian/build#8152
File: lib/functions/configuration/interactive.sh:209-266
Timestamp: 2025-05-05T12:35:07.143Z
Learning: For the interactive kernel selection in Armbian, KERNEL_MAJOR_MINOR and KERNEL_DESCRIPTION are parsed from family.conf but deliberately not set as environment variables to avoid potential interference with other parts of the build system.
Learnt from: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:148-161
Timestamp: 2025-06-12T21:03:39.686Z
Learning: In Armbian U-Boot bootscripts, the `${cpu}` environment variable is always pre-set by U-Boot (or defaults to a valid value), so additional guards against it being unset are unnecessary.
packages/bsp/ti/weston/weston (1)
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
config/boards/sk-am62b.conf (2)
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-06-16T03:24:00.458Z
Learning: In Armbian board configuration files (.csc), the standard pattern is to have only one line as a comment describing the board hardware specifications. This single-line description typically includes the SoC model, core count, RAM options, and key features like connectivity options, storage interfaces, and special features.
Learnt from: pyavitz
PR: armbian/build#8361
File: config/kernel/linux-sunxi64-edge.config:594-597
Timestamp: 2025-07-06T16:27:45.349Z
Learning: In Armbian sunxi64-edge kernel configurations, PCI/PCIe support may be enabled for future-proofing purposes to support upcoming Allwinner SoCs like the T536 that include PCIe Gen2 interfaces, even if current primary targets like H616 lack PCIe controllers. This forward-looking approach prepares the configuration for next-generation hardware in the same SoC family.
config/boards/sk-am64b.conf (8)
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-06-16T03:24:00.458Z
Learning: In Armbian board configuration files (.csc), the standard pattern is to have only one line as a comment describing the board hardware specifications. This single-line description typically includes the SoC model, core count, RAM options, and key features like connectivity options, storage interfaces, and special features.
Learnt from: libiunc
PR: armbian/build#8033
File: config/kernel/linux-starfive2-vendor.config:43-43
Timestamp: 2025-03-31T12:57:13.880Z
Learning: For StarFive2 platform kernel configurations, maintain alignment with vendor-provided configurations rather than modifying security settings like SECCOMP. This ensures hardware compatibility as intended by the manufacturer.
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:42:09.086Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, KERNELSOURCE is explicitly declared when using unofficial or 3rd party kernel repositories (like the "dev" branch using https://github.com/apritzel/linux), but can be omitted when using the standard mainline kernel (like the "edge" branch) since it will fall back to the default mainline source.
Learnt from: pyavitz
PR: armbian/build#8361
File: config/kernel/linux-sunxi64-edge.config:594-597
Timestamp: 2025-07-06T16:27:45.349Z
Learning: In Armbian sunxi64-edge kernel configurations, PCI/PCIe support may be enabled for future-proofing purposes to support upcoming Allwinner SoCs like the T536 that include PCIe Gen2 interfaces, even if current primary targets like H616 lack PCIe controllers. This forward-looking approach prepares the configuration for next-generation hardware in the same SoC family.
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:40:52.109Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, when KERNELSOURCE is not explicitly declared in a case branch, the system falls back to a default KERNELSOURCE value, so missing KERNELSOURCE declarations are not critical issues.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.
Learnt from: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:148-161
Timestamp: 2025-06-12T21:03:39.686Z
Learning: In Armbian U-Boot bootscripts, the `${cpu}` environment variable is always pre-set by U-Boot (or defaults to a valid value), so additional guards against it being unset are unnecessary.
Learnt from: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:13-23
Timestamp: 2025-06-12T21:08:11.318Z
Learning: In config/bootscripts/boot-sunxi.cmd the variable name `align_overlap_oboe_avoidance` intentionally uses the acronym “OBOE” (Off-By-One Error); it is not a typo and should not be renamed.
extensions/ti-debpkgs.sh (5)
Learnt from: leggewie
PR: armbian/build#8133
File: extensions/apa.sh:1-2
Timestamp: 2025-04-28T08:27:26.890Z
Learning: In the Armbian build system, extension scripts in the `extensions/` directory contain hook functions and are meant to be sourced, not executed directly. These scripts don't require a shebang or `set -euo pipefail`.
Learnt from: EvilOlaf
PR: armbian/build#8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: In Armbian build framework, extensions are loaded and executed via lib/functions/general/extensions.sh using call_extension_method. The framework sets set -o errexit and set -o errtrace globally, and individual commands run with pipefail protection through the command execution functions.
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Learnt from: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
Learnt from: EvilOlaf
PR: armbian/build#8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: Armbian build framework extensions are NOT designed to run standalone but are called by the supervisor. The framework provides environment variables like USERPATCHES_PATH and SDCARD, and commands are executed with protection via run_host_command_logged and chroot_sdcard functions that use bash -e -o pipefail.
config/sources/families/k3.conf (6)
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:40:52.109Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, when KERNELSOURCE is not explicitly declared in a case branch, the system falls back to a default KERNELSOURCE value, so missing KERNELSOURCE declarations are not critical issues.
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:42:09.086Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, KERNELSOURCE is explicitly declared when using unofficial or 3rd party kernel repositories (like the "dev" branch using https://github.com/apritzel/linux), but can be omitted when using the standard mainline kernel (like the "edge" branch) since it will fall back to the default mainline source.
Learnt from: Grippy98
PR: armbian/build#8152
File: lib/functions/configuration/interactive.sh:209-266
Timestamp: 2025-05-05T12:35:07.143Z
Learning: For the interactive kernel selection in Armbian, KERNEL_MAJOR_MINOR and KERNEL_DESCRIPTION are parsed from family.conf but deliberately not set as environment variables to avoid potential interference with other parts of the build system.
Learnt from: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Learnt from: leggewie
PR: armbian/build#8357
File: config/boards/qcom-robotics-rb5.conf:88-88
Timestamp: 2025-07-04T18:33:19.873Z
Learning: In the Armbian build system, package transitions in Debian/Ubuntu don't happen uniformly across all releases. For example, the transition from plasma-workspace-wayland to plasma-workspace occurred in newer releases like trixie (Debian testing) around July 2024, but older releases like jammy (Ubuntu 22.04) still maintain the original plasma-workspace-wayland package. Release-specific package availability must be considered when updating package references.
🧬 Code Graph Analysis (1)
extensions/ti-debpkgs.sh (3)
lib/functions/logging/runners.sh (2)
run_host_command_logged(204-206)chroot_sdcard(133-136).github/generate_CODEOWNERS.sh (1)
display_alert(6-6)lib/functions/compilation/utils-compilation.sh (1)
grab_version(23-32)
🔇 Additional comments (10)
packages/bsp/ti/weston/weston (1)
1-2: Verify socket path consistency with weston.socket.The environment variables look correct for a system-level Weston service. However, there's a potential mismatch:
WAYLAND_DISPLAY=wayland-1suggests the socket should be at/run/wayland-1, but the correspondingweston.socketfile creates a socket at/run/wayland-0.Please verify that the socket path and display name are consistent between this file and
weston.socket.config/boards/sk-am62b.conf (1)
19-22: LGTM! Feature flags are well-structured.The addition of GPU, display, and Chromium support flags follows Armbian conventions and integrates well with the broader TI package management changes in the k3 family configuration.
config/boards/sk-am64b.conf (1)
19-20: LGTM! Board-specific configuration is appropriate.The SOC_ID and firmware package additions are consistent with the AM62B board configuration approach and properly specify AM64-specific PRU firmware packages.
extensions/ti-debpkgs.sh (1)
48-54: LGTM! DKMS activation logic is sound.The function correctly:
- Checks the GPU_SUPPORT flag
- Uses the standard
grab_versionfunction- Constructs the kernel version string following Armbian conventions
- Runs DKMS autoinstall with appropriate verbosity
packages/bsp/ti/weston/weston.service (1)
34-34:.not_logged_in_yetLifecycle Is Fully Handled
- Creation
- lib/functions/rootfs/distro-agnostic.sh:
touch "${SDCARD}"/root/.not_logged_in_yet- extensions/preset-firstrun.sh: writes presets into
${SDCARD}/root/.not_logged_in_yetfor automated first-run configurations- Consumption
- etc/profile.d/armbian-check-first-login.sh and armbian-firstlogin: detect
/root/.not_logged_in_yetto drive the first-login flow- Removal
- armbian-firstlogin (multiple places):
rm -f /root/.not_logged_in_yetonce setup completes- config/templates/customize-image.sh.template & extensions/cloud-init/cloud-init.sh: clean up leftover
.not_logged_in_yetNo further changes required.
config/sources/families/k3.conf (5)
23-23: LGTM: Clean package array initializationThe TI_PACKAGES array is properly initialized with base packages. This provides a solid foundation for conditional package additions.
31-31: SOC_ID definitions verifiedSOC_ID is defined in all relevant board configuration files, ensuring
${SOC_ID}interpolates correctly ink3.conf.• config/boards/sk-am64b.conf: SOC_ID="am64"
• config/boards/sk-am62b.conf: SOC_ID="am62"No further action required.
30-34: Ensure GPU support packages exist on each target distributionThe TI GPU packages (
ti-img-rogue-driver-${SOC_ID}-dkms,ti-img-rogue-umlibs-${SOC_ID},ti-img-rogue-tools-${SOC_ID},ti-img-rogue-firmware-${SOC_ID}, plusmesa-vulkan-drivers,libgl1-mesa-dri,gstreamer1.0-*) may be named differently or missing in some distros. Add distribution-specific checks before appending them toTI_PACKAGES. For example:• Detect the distro via
/etc/os-releaseand dispatch accordingly
• Debian/Ubuntu:apt-cache policy "$pkg" &>/dev/null || missing+=("$pkg")• RHEL/CentOS/Fedora:
dnf repoquery --quiet --exactmatch "$pkg" || missing+=("$pkg")• Report any missing packages and skip or substitute as needed
Example snippet:
. /etc/os-release if [[ "${GPU_SUPPORT}" == "yes" ]] ; then + missing=() + add_pkg(){ + local pkg=$1 + case "$ID" in + debian|ubuntu) apt-cache policy "$pkg" &>/dev/null ;; + centos|rhel|fedora) dnf repoquery --quiet --exactmatch "$pkg" ;; + *) return 1 ;; + esac || missing+=("$pkg") + TI_PACKAGES+=("$pkg") + } + # TI image drivers - TI_PACKAGES+=("ti-img-rogue-driver-${SOC_ID}-dkms" "ti-img-rogue-umlibs-${SOC_ID}" "ti-img-rogue-tools-${SOC_ID}" "ti-img-rogue-firmware-${SOC_ID}") + for pkg in \ + "ti-img-rogue-driver-${SOC_ID}-dkms" \ + "ti-img-rogue-umlibs-${SOC_ID}" \ + "ti-img-rogue-tools-${SOC_ID}" \ + "ti-img-rogue-firmware-${SOC_ID}"; do + add_pkg "$pkg" + done + # Mesa / Vulkan - TI_PACKAGES+=("mesa-vulkan-drivers" "libgl1-mesa-dri") + for pkg in "mesa-vulkan-drivers" "libgl1-mesa-dri"; do + add_pkg "$pkg" + done + # GStreamer - TI_PACKAGES+=("gstreamer1.0-tools" "gstreamer1.0-plugins-good" "gstreamer1.0-plugins-bad" "gstreamer1.0-plugins-base") + for pkg in \ + "gstreamer1.0-tools" \ + "gstreamer1.0-plugins-good" \ + "gstreamer1.0-plugins-bad" \ + "gstreamer1.0-plugins-base"; do + add_pkg "$pkg" + done + + if [[ ${#missing[@]} -gt 0 ]]; then + echo "Warning: the following GPU packages are unavailable on $ID: ${missing[*]}" + fi fiPlease verify these package names against your TI repositories on each supported distribution.
46-48: Improve array handling for EXTRA_BOARD_PACKAGESThe current syntax
"${EXTRA_BOARD_PACKAGES[@]}"within the TI_PACKAGES+=() assignment may not work correctly if EXTRA_BOARD_PACKAGES is not an array. Consider using a more robust approach.-if [[ ! -z ${EXTRA_BOARD_PACKAGES} ]] ; then - TI_PACKAGES+=("${EXTRA_BOARD_PACKAGES[@]}") -fi +if [[ -n "${EXTRA_BOARD_PACKAGES}" ]] ; then + if [[ "$(declare -p EXTRA_BOARD_PACKAGES 2>/dev/null)" =~ "declare -a" ]] ; then + TI_PACKAGES+=("${EXTRA_BOARD_PACKAGES[@]}") + else + TI_PACKAGES+=("${EXTRA_BOARD_PACKAGES}") + fi +fiThis handles both string and array cases properly.
⛔ Skipped due to learnings
Learnt from: djurny PR: armbian/build#8166 File: config/bootscripts/boot-mvebu.cmd:38-50 Timestamp: 2025-05-07T20:49:40.969Z Learning: In U-Boot scripts, `itest.s` should be used for explicit string comparisons rather than plain `itest` with string operators like `==`.Learnt from: djurny PR: armbian/build#8315 File: lib/functions/rootfs/distro-agnostic.sh:54-68 Timestamp: 2025-06-22T00:45:26.182Z Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
44-44: Action Required: Verify k3 family inclusion and firmware package namesEnsure
config/sources/families/k3.confis actually applied to each k3-based board and that the SOC_ID interpolation produces valid firmware package names. Based on the current board configs:
- config/boards/sk-am64b.conf → SOC_ID="am64" → would generate
firmware-ti-ipc-am64- config/boards/sk-am62b.conf → SOC_ID="am62" → would generate
firmware-ti-ipc-am62Please confirm:
- That
k3.confis sourced (directly or via common include) in each of these board files- The interpolated packages (
firmware-ti-ipc-am64,firmware-ti-ipc-am62) actually exist and match your firmware repository naming
packages/bsp/ti/weston/weston.socket
Outdated
| RequiresMountsFor=/run | ||
|
|
||
| [Socket] | ||
| ListenStream=/run/wayland-0 |
There was a problem hiding this comment.
Fix socket path mismatch with environment configuration.
The socket path /run/wayland-0 doesn't match the WAYLAND_DISPLAY=wayland-1 setting in the weston environment file. This will cause connection failures.
Choose one of these solutions:
- Change the socket path to match the display name:
-ListenStream=/run/wayland-0
+ListenStream=/run/wayland-1- Or change the environment variable to match the socket:
-WAYLAND_DISPLAY=wayland-1
+WAYLAND_DISPLAY=wayland-0Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/bsp/ti/weston/weston.socket at line 6, the socket path
ListenStream=/run/wayland-0 does not match the WAYLAND_DISPLAY=wayland-1
environment setting, causing connection failures. Fix this by either updating
the socket path to /run/wayland-1 to match the environment variable or change
the WAYLAND_DISPLAY environment variable to wayland-0 to match the socket path,
ensuring both are consistent.
| User=root | ||
| Group=root |
There was a problem hiding this comment.
Security concern: Running Weston as root user
Running the Weston compositor as root presents significant security risks. Display servers typically run with lower privileges for security isolation. Consider creating a dedicated user account for Weston or using a login manager that can handle privilege separation.
Consider this approach instead:
-User=root
-Group=root
+User=weston
+Group=westonAnd ensure the weston user is created with appropriate permissions for accessing graphics hardware.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| User=root | |
| Group=root | |
| User=weston | |
| Group=weston |
🤖 Prompt for AI Agents
In packages/bsp/ti/weston/weston.service at lines 47 to 48, the service is
configured to run Weston as the root user, which poses security risks. Modify
the User and Group directives to run Weston under a dedicated, non-root user
account (e.g., 'weston'). Ensure this user is created beforehand with the
necessary permissions to access graphics hardware, and update the service file
accordingly to enhance security by limiting privileges.
| WorkingDirectory=/root/ | ||
|
|
||
| # Set up a full user session for the user, required by Weston. | ||
| PAMName=weston-autologin |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify PAM configuration for weston-autologin
The service references a PAM configuration named "weston-autologin" but this PAM configuration file is not provided in the PR. Without proper PAM configuration, the service may fail to start.
Please ensure the corresponding PAM configuration file exists:
🏁 Script executed:
#!/bin/bash
# Description: Check if PAM configuration for weston-autologin exists
# Expected: PAM config file should exist and be properly configured
fd -t f "weston-autologin" /etc/pam.d/Length of output: 137
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for pam.d directories in the repository:"
fd -t d pam.d .
echo
echo "Searching for files named 'weston-autologin' in the repository:"
fd -t f weston-autologin .
echo
echo "Searching for occurrences of 'weston-autologin' in file contents:"
rg -n "weston-autologin"Length of output: 563
Add missing PAM configuration for “weston-autologin”
The service unit at packages/bsp/ti/weston/weston.service (line 54) specifies
PAMName=weston-autologinbut no corresponding PAM config file (pam.d/weston-autologin) is present in this PR. Without it, the service will fail to start.
• Please add the PAM configuration file under your package’s pam.d/ directory (e.g. files/etc/pam.d/weston-autologin) with the required rules.
• Verify that it’s installed to /etc/pam.d/weston-autologin on target systems.
🤖 Prompt for AI Agents
In packages/bsp/ti/weston/weston.service at line 54, the PAMName is set to
"weston-autologin" but the corresponding PAM configuration file
pam.d/weston-autologin is missing. To fix this, create the PAM config file with
the necessary authentication rules under your package's pam.d/ directory (e.g.,
files/etc/pam.d/weston-autologin) and ensure it is installed to
/etc/pam.d/weston-autologin on the target system.
| [Service] | ||
| # Requires systemd-notify.so Weston plugin. | ||
| Type=simple | ||
| EnvironmentFile=/etc/default/weston |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify weston environment file
The service references /etc/default/weston but this environment file is not provided in the PR. Missing environment configuration could cause the service to fail or behave unexpectedly.
Please ensure the weston environment file exists:
🏁 Script executed:
#!/bin/bash
# Description: Check for weston environment file
# Expected: Environment file should exist with proper variables
fd -t f "weston" /etc/default/Length of output: 131
🏁 Script executed:
#!/bin/bash
# Search for the weston environment file within the repository
find . -type f | grep '/etc/default/weston$'Length of output: 46
Provide Weston environment file or remove unused reference
The weston.service unit at packages/bsp/ti/weston/weston.service still references an external file:
39 │ EnvironmentFile=/etc/default/weston
However, no /etc/default/weston is present in the repo or packaging. This can cause the service to fail or skip needed settings.
Please address one of the following:
-
Include the default environment file
Add a file (e.g.packages/bsp/ti/weston/weston.env) and install it to/etc/default/westonwith the necessary variables for Weston. -
Remove or adjust the
EnvironmentFile=directive
If Weston doesn’t require custom environment variables, drop or comment out the line to avoid loading a non-existent file.
🤖 Prompt for AI Agents
In packages/bsp/ti/weston/weston.service at line 39, the EnvironmentFile
directive points to /etc/default/weston, which does not exist in the repo or
packaging. To fix this, either create a default environment file with necessary
Weston variables (e.g., packages/bsp/ti/weston/weston.env) and ensure it is
installed to /etc/default/weston, or remove/comment out the EnvironmentFile line
if no custom environment variables are needed to prevent service failure.
|
Starting Weston as a root systemd service was a hack we did for our Yocto SDK distro, it was not correct and we are switching to using a proper weston user and launching with a login-manager (Emptty). For Armbian you should do the same or re-use GDM or similar default manager to launch a weston session. |
|
@glneo Understood. I am going with emptty, and got it working using the upstream emptty package. Do we want auto-login into weston like in our Yocto-based SDK, or do we want to show the username/password screen? I am wondering since the first boot takes you through the armbian-firstlogin script setup anyway. For now, I have gone with auto-login. If you think the alternative is better, I will update the PR. |
jsuhaas22
force-pushed
the
add-configure-more-packages
branch
from
781005f to
e0e9e5d
Compare
github-actions
bot
removed
the
Ready to merge
config/sources/families/k3.conf
Outdated
| TI_PACKAGES+=("weston" "emptty") | ||
| fi | ||
|
|
||
| if [[ "${CHROMIUM_SUPPORT}" == "yes" ]] ; then |
There was a problem hiding this comment.
For my own knowledge here - when would we have GPU support but no Chromium support currently?
There was a problem hiding this comment.
Currently, this is the case with AM62SIP and AM62LP. This is due to memory constraints (at least in case of SIP).
jsuhaas22
force-pushed
the
add-configure-more-packages
branch
from
e0e9e5d to
8399056
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
extensions/ti-debpkgs.sh (1)
44-50: Quote variable expansions and verify destination paths
$SRCand$SDCARDare unquoted inside thecp/mkdircommand strings.
A pathname with spaces (rare, but possible in$SRC) would break the command passed to the host shell.The Weston/Emptty configuration copies can be compacted, but more importantly, should be fully quoted:
- run_host_command_logged "cp -v $SRC/packages/bsp/ti/emptty/weston.ini $SDCARD/etc/xdg/weston/" + run_host_command_logged "cp -v \"${SRC}/packages/bsp/ti/emptty/weston.ini\" \"${SDCARD}/etc/xdg/weston/\""(repeat for the other two
cplines).
- Double-check that Emptty actually expects its main config at
/etc/emptty/conf.
Upstream recently switched the default to/etc/emptty/emptty.conf; if that’s the case the copy target should be updated accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
config/boards/sk-am62b.conf(1 hunks)config/boards/sk-am64b.conf(1 hunks)config/cli/common/main/packages(1 hunks)config/sources/families/k3.conf(1 hunks)extensions/ti-debpkgs.sh(1 hunks)packages/bsp/ti/emptty/emptty.conf(1 hunks)packages/bsp/ti/emptty/pamconf(1 hunks)packages/bsp/ti/emptty/weston.ini(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- config/cli/common/main/packages
- packages/bsp/ti/emptty/emptty.conf
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/bsp/ti/emptty/weston.ini
- config/boards/sk-am62b.conf
- packages/bsp/ti/emptty/pamconf
- config/boards/sk-am64b.conf
- config/sources/families/k3.conf
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: EvilOlaf
PR: armbian/build#8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: EvilOlaf
PR: armbian/build#8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Learnt from: leggewie
PR: armbian/build#8357
File: config/boards/qcom-robotics-rb5.conf:88-88
Timestamp: 2025-07-04T18:33:19.873Z
Learning: In the Armbian build system, package transitions in Debian/Ubuntu don't happen uniformly across all releases. For example, the transition from plasma-workspace-wayland to plasma-workspace occurred in newer releases like trixie (Debian testing) around July 2024, but older releases like jammy (Ubuntu 22.04) still maintain the original plasma-workspace-wayland package. Release-specific package availability must be considered when updating package references.
Learnt from: libiunc
PR: armbian/build#8033
File: config/kernel/linux-starfive2-vendor.config:43-43
Timestamp: 2025-03-31T12:57:13.880Z
Learning: For StarFive2 platform kernel configurations, maintain alignment with vendor-provided configurations rather than modifying security settings like SECCOMP. This ensures hardware compatibility as intended by the manufacturer.
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-06-16T03:24:00.458Z
Learning: In Armbian board configuration files (.csc), the standard pattern is to have only one line as a comment describing the board hardware specifications. This single-line description typically includes the SoC model, core count, RAM options, and key features like connectivity options, storage interfaces, and special features.
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:1-14
Timestamp: 2025-07-17T04:12:33.125Z
Learning: In the Armbian build system, board configuration files (*.conf) follow a project standard of not including shebang lines, even though they contain bash code and are sourced by the build system. This is an established Armbian convention that individual contributors are expected to follow, and changes to this standard would require broader project maintainer approval.
Learnt from: pyavitz
PR: armbian/build#8421
File: config/kernel/linux-sunxi64-edge.config:805-825
Timestamp: 2025-07-27T15:53:30.629Z
Learning: In the Armbian build system, kernel configurations prioritize broad hardware compatibility over optimization concerns. Even when including numerous legacy/obsolete drivers causes technical issues like increased initramfs size and slower enumeration, the project philosophy is to "appease the masses" by ensuring maximum device compatibility rather than optimizing for specific use cases.
Learnt from: EvilOlaf
PR: armbian/build#8417
File: config/boards/orangepi5pro.csc:57-58
Timestamp: 2025-07-23T07:30:52.265Z
Learning: In the Armbian build system, BOOTPATCHDIR can contain board-specific subdirectories (e.g., board_orangepi5pro) for applying patches to specific boards only. The framework automatically checks if such board-specific subdirectories exist for the board being built and applies those patches accordingly.
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-07-21T04:12:02.439Z
Learning: In the Armbian build system, for recurring maintenance tasks like kernel version bumping, TODO comments should use generic version formats (e.g., "MAJOR.MINOR-rc1") rather than specific version numbers (e.g., "6.17-rc1") to avoid the need for frequent comment updates that would create unnecessary maintenance overhead.
Learnt from: Grippy98
PR: armbian/build#8152
File: lib/functions/configuration/interactive.sh:209-266
Timestamp: 2025-05-05T12:35:07.143Z
Learning: For the interactive kernel selection in Armbian, KERNEL_MAJOR_MINOR and KERNEL_DESCRIPTION are parsed from family.conf but deliberately not set as environment variables to avoid potential interference with other parts of the build system.
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:42:09.086Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, KERNELSOURCE is explicitly declared when using unofficial or 3rd party kernel repositories (like the "dev" branch using https://github.com/apritzel/linux), but can be omitted when using the standard mainline kernel (like the "edge" branch) since it will fall back to the default mainline source.
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
extensions/ti-debpkgs.sh (11)
Learnt from: leggewie
PR: #8133
File: extensions/apa.sh:1-2
Timestamp: 2025-04-28T08:27:26.890Z
Learning: In the Armbian build system, extension scripts in the extensions/ directory contain hook functions and are meant to be sourced, not executed directly. These scripts don't require a shebang or set -euo pipefail.
Learnt from: EvilOlaf
PR: #8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: In Armbian build framework, extensions are loaded and executed via lib/functions/general/extensions.sh using call_extension_method. The framework sets set -o errexit and set -o errtrace globally, and individual commands run with pipefail protection through the command execution functions.
Learnt from: Grippy98
PR: #8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Learnt from: EvilOlaf
PR: #8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: Armbian build framework extensions are NOT designed to run standalone but are called by the supervisor. The framework provides environment variables like USERPATCHES_PATH and SDCARD, and commands are executed with protection via run_host_command_logged and chroot_sdcard functions that use bash -e -o pipefail.
Learnt from: djurny
PR: #8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
Learnt from: EvilOlaf
PR: #8417
File: config/boards/orangepi5pro.csc:57-58
Timestamp: 2025-07-23T07:30:52.265Z
Learning: In the Armbian build system, BOOTPATCHDIR can contain board-specific subdirectories (e.g., board_orangepi5pro) for applying patches to specific boards only. The framework automatically checks if such board-specific subdirectories exist for the board being built and applies those patches accordingly.
Learnt from: amazingfate
PR: #8419
File: lib/functions/host/prepare-host.sh:272-275
Timestamp: 2025-07-23T10:01:41.310Z
Learning: In the Armbian build system, the design philosophy is to fail fast when host dependencies are not met rather than gracefully skipping unsupported architectures. This ensures build environment consistency and prevents silent failures. Host dependency checks should be explicit and non-negotiable.
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-07-21T04:12:02.439Z
Learning: In the Armbian build system, for recurring maintenance tasks like kernel version bumping, TODO comments should use generic version formats (e.g., "MAJOR.MINOR-rc1") rather than specific version numbers (e.g., "6.17-rc1") to avoid the need for frequent comment updates that would create unnecessary maintenance overhead.
Learnt from: djurny
PR: #8287
File: config/bootscripts/boot-sunxi.cmd:148-161
Timestamp: 2025-06-12T21:03:39.686Z
Learning: In Armbian U-Boot bootscripts, the ${cpu} environment variable is always pre-set by U-Boot (or defaults to a valid value), so additional guards against it being unset are unnecessary.
Learnt from: djurny
PR: #8235
File: packages/bsp/mvebu/helios4/helios4-wol.service:0-0
Timestamp: 2025-05-29T01:52:21.381Z
Learning: In systemd service ExecStart commands, shell variables like ${INTF} can cause issues because systemd may resolve them before the shell processes them, leading to "no device matches name" errors. Using xargs with -I{} is a more reliable approach for iterating over dynamic lists in systemd services, as it avoids variable substitution problems entirely.
Learnt from: pyavitz
PR: #8388
File: config/boards/kickpik2b.conf:1-14
Timestamp: 2025-07-17T04:12:33.125Z
Learning: In the Armbian build system, board configuration files (*.conf) follow a project standard of not including shebang lines, even though they contain bash code and are sourced by the build system. This is an established Armbian convention that individual contributors are expected to follow, and changes to this standard would require broader project maintainer approval.
🧬 Code Graph Analysis (1)
extensions/ti-debpkgs.sh (3)
lib/functions/rootfs/post-tweaks.sh (1)
post_debootstrap_tweaks(10-34)lib/functions/rootfs/customize.sh (2)
run_hooks_post_armbian_repo_customize_image(59-66)customize_image(10-51)extensions/network/net-systemd-networkd.sh (1)
pre_install_kernel_debs__configure_systemd_networkd(14-42)
extensions/ti-debpkgs.sh
Outdated
| function pre_customize_image__enable_services() { | ||
| chroot_sdcard "groupadd nopasswdlogin" | ||
| chroot_sdcard "useradd -m -s /bin/bash weston" | ||
| chroot_sdcard "usermod -aG video,render,input,nopasswdlogin weston" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make user/group creation idempotent to avoid hard-failures on rebuilds
groupadd and useradd abort with non-zero exit codes when the entity already exists.
Because chroot_sdcard executes with set -e, a rebuild or a base image that already ships the nopasswdlogin group (or a weston user) will terminate the whole build.
- chroot_sdcard "groupadd nopasswdlogin"
- chroot_sdcard "useradd -m -s /bin/bash weston"
+ # Create only if missing – keeps the hook idempotent
+ chroot_sdcard "getent group nopasswdlogin || groupadd --system nopasswdlogin"
+ chroot_sdcard "id -u weston >/dev/null 2>&1 || useradd --system -m -s /bin/bash weston"Also consider guarding the subsequent usermod against missing auxiliary groups (input, render, …) or create them up-front for the same reason.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function pre_customize_image__enable_services() { | |
| chroot_sdcard "groupadd nopasswdlogin" | |
| chroot_sdcard "useradd -m -s /bin/bash weston" | |
| chroot_sdcard "usermod -aG video,render,input,nopasswdlogin weston" | |
| function pre_customize_image__enable_services() { | |
| # Create only if missing – keeps the hook idempotent | |
| chroot_sdcard "getent group nopasswdlogin || groupadd --system nopasswdlogin" | |
| chroot_sdcard "id -u weston >/dev/null 2>&1 || useradd --system -m -s /bin/bash weston" | |
| chroot_sdcard "usermod -aG video,render,input,nopasswdlogin weston" | |
🤖 Prompt for AI Agents
In extensions/ti-debpkgs.sh around lines 39 to 43, the groupadd and useradd
commands are not idempotent and will fail if the group or user already exists,
causing the build to terminate. Modify the script to first check if the group
'nopasswdlogin' exists before attempting to add it, and similarly check if the
user 'weston' exists before adding. Also, ensure that the auxiliary groups
'video', 'render', 'input', and 'nopasswdlogin' exist before running usermod to
add the user to these groups, creating any missing groups as needed to prevent
failures.
8399056 to
5ed3111
Compare
github-actions
bot
removed
the
Ready to merge
jsuhaas22
force-pushed
the
add-configure-more-packages
branch
from
5ed3111 to
e77bc6e
Compare
config/sources/families/k3.conf
Outdated
| if [[ "${GPU_SUPPORT}" == "yes" ]] ; then | ||
| TI_PACKAGES+=("ti-img-rogue-driver-${SOC_ID}-dkms" "ti-img-rogue-umlibs-${SOC_ID}" "ti-img-rogue-tools-${SOC_ID}" "ti-img-rogue-firmware-${SOC_ID}") | ||
| TI_PACKAGES+=("mesa-vulkan-drivers" "libgl1-mesa-dri") | ||
| TI_PACKAGES+=("gstreamer1.0-tools" "gstreamer1.0-plugins-good" "gstreamer1.0-plugins-bad" "gstreamer1.0-plugins-base") |
There was a problem hiding this comment.
What uses these? Shouldn't the user pull these in if they need them like other packages?
There was a problem hiding this comment.
My rationale is: our boards are for evaluating and prototyping with our SoCs. If our SoC supports camera, then our image should ideally have by-default whatever it needs to run the camera. Our docs run the camera with GStreamer, which is why I added these. We had these in our previous flavour of Debian as well.
jsuhaas22
force-pushed
the
add-configure-more-packages
branch
from
e77bc6e to
98734f1
Compare
jsuhaas22
force-pushed
the
add-configure-more-packages
branch
from
98734f1 to
77db37c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
extensions/ti-debpkgs.sh (3)
57-58: DKMS kernel ABI detection: good fix; add guards and remove debug print.Nice adoption of the installed ABI directory to avoid version drift. Minor touches:
- Quote paths and variables.
- Guard against a missing/empty /lib/modules (e.g., kernel not yet staged).
- Drop the “echo SDCARD” debug line.
-echo "SDCARD: ${SDCARD}" -kernel_version_family="$(ls ${SDCARD}/lib/modules | sort -V | tail -n1)" # $(grab_version "${SRC}/cache/sources/${LINUXSOURCEDIR}") -chroot_sdcard "dkms autoinstall --verbose --kernelver ${kernel_version_family}" || display_alert "DKMS build failed for kernel ${kernel_version_family}" +if [[ -d "${SDCARD}/lib/modules" ]] && kernel_version_family="$(ls -1 "${SDCARD}/lib/modules" | sort -V | tail -n1)"; then + chroot_sdcard "dkms autoinstall --verbose --kernelver \"${kernel_version_family}\"" \ + || display_alert "DKMS build failed for kernel ${kernel_version_family}" +else + display_alert "No kernel ABI found under /lib/modules in the image; skipping DKMS autoinstall" +fi
44-50: Resolved: wrong path variable ($DEST → $SDCARD).Past concern about using $DEST was addressed; current code consistently uses $SDCARD.
40-42: Harden group/user creation and ensure auxiliary groups exist.Two issues that can break the build in chroot (bash -e):
- usermod will fail if any of video/render/input groups don’t exist.
- Create system group/user types to match distro conventions.
Proposed fix:
-chroot_sdcard "getent group nopasswdlogin || groupadd nopasswdlogin" -chroot_sdcard "id -u weston >/dev/null 2>&1 || useradd -m -s /bin/bash weston" -chroot_sdcard "usermod -aG video,render,input,nopasswdlogin weston" +chroot_sdcard "getent group nopasswdlogin || groupadd --system nopasswdlogin" +# Ensure auxiliary groups exist before usermod to keep the hook idempotent +chroot_sdcard "for g in video render input; do getent group \"\$g\" || groupadd --system \"\$g\"; done" +chroot_sdcard "id -u weston >/dev/null 2>&1 || useradd --system -m -s /bin/bash weston" +chroot_sdcard "usermod -aG video,render,input,nopasswdlogin weston" +# Optional: lock password to avoid tty1 logins while preserving emptty autologin on tty7 +# chroot_sdcard "passwd -l weston"
🧹 Nitpick comments (5)
packages/bsp/ti/emptty/pamconf (2)
2-3: Autologin via nopasswdlogin: acceptable, but lock down the autologin user.Using pam_succeed_if with nopasswdlogin is a standard way to enable passwordless autologin for a whitelisted user. Since only user “weston” is added to that group in this PR, scope is limited. To reduce attack surface outside TTY7, consider locking the user’s password after creation so it can’t be used on other PAM stacks (e.g., getty on tty1).
Follow-up in extensions/ti-debpkgs.sh (same hunk):
- After creating the user, run:
passwd -l weston(still works with emptty autologin).
4-5: Keyring/kwallet modules are still active (not removed).The lines with pam_gnome_keyring.so and pam_kwallet5.so are present (the leading “-” only ignores missing modules; it doesn’t disable them). The AI summary states these were “removed.” If the intent is to drop these helpers entirely for a minimal Wayland/Weston setup, delete the lines; otherwise keep them as-is.
Suggested removal:
- -auth optional pam_gnome_keyring.so - -auth optional pam_kwallet5.so - -session optional pam_gnome_keyring.so auto_start - -session optional pam_kwallet5.so auto_start force_runAlso applies to: 8-9
packages/bsp/ti/emptty/emptty.conf (1)
14-18: Autologin defaults: consider making this configurable per image.DEFAULT_USER=weston and AUTOLOGIN=true are reasonable for OOB demos, but some boards/images may prefer a login screen. Consider gating these via a build-time variable (e.g., TI_WESTON_AUTOLOGIN=yes/no) and templating this file accordingly during build.
If you want me to wire that into the extension hook, I can propose a small templating step conditioned on a variable.
extensions/ti-debpkgs.sh (2)
44-50: Use install(1) to set ownership/permissions deterministically.cp preserves source perms/umask; install makes the result predictable in /etc. Also, good catch on writing to /etc/emptty/conf (that is the canonical path for emptty’s config).
Proposed change:
-run_host_command_logged "mkdir -p $SDCARD/etc/xdg/weston/" -run_host_command_logged "cp -v $SRC/packages/bsp/ti/emptty/weston.ini $SDCARD/etc/xdg/weston/" -run_host_command_logged "mkdir -p $SDCARD/etc/pam.d/" -run_host_command_logged "cp -v $SRC/packages/bsp/ti/emptty/pamconf $SDCARD/etc/pam.d/emptty" -run_host_command_logged "mkdir -p $SDCARD/etc/emptty/" -run_host_command_logged "cp -v $SRC/packages/bsp/ti/emptty/emptty.conf $SDCARD/etc/emptty/conf" +run_host_command_logged "install -d \"$SDCARD/etc/xdg/weston\" \"$SDCARD/etc/pam.d\" \"$SDCARD/etc/emptty\"" +run_host_command_logged "install -m 0644 \"$SRC/packages/bsp/ti/emptty/weston.ini\" \"$SDCARD/etc/xdg/weston/weston.ini\"" +run_host_command_logged "install -m 0644 \"$SRC/packages/bsp/ti/emptty/pamconf\" \"$SDCARD/etc/pam.d/emptty\"" +run_host_command_logged "install -m 0644 \"$SRC/packages/bsp/ti/emptty/emptty.conf\" \"$SDCARD/etc/emptty/conf\""
51-51: Consider masking a conflicting getty and setting default target (optional).
- On some distros, getty@tty7 may exist or be pulled in; masking avoids accidental contention.
- If you expect a graphical UX by default, setting graphical.target may be desired (emptty works in multi-user too; your call).
Optional additions:
chroot_sdcard "systemctl enable [email protected]" || display_alert "systemctl enable failed for [email protected]" +chroot_sdcard "systemctl mask --now [email protected]" || true +# Optionally: chroot_sdcard "systemctl set-default graphical.target" || true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
config/boards/sk-am62b.conf(1 hunks)config/boards/sk-am64b.conf(1 hunks)config/cli/common/main/packages(1 hunks)config/sources/families/k3.conf(1 hunks)extensions/ti-debpkgs.sh(1 hunks)packages/bsp/ti/emptty/emptty.conf(1 hunks)packages/bsp/ti/emptty/pamconf(1 hunks)packages/bsp/ti/emptty/weston.ini(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- config/cli/common/main/packages
- config/boards/sk-am62b.conf
- config/boards/sk-am64b.conf
- config/sources/families/k3.conf
- packages/bsp/ti/emptty/weston.ini
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-16T15:34:34.672Z
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Applied to files:
extensions/ti-debpkgs.sh
🧬 Code graph analysis (1)
extensions/ti-debpkgs.sh (3)
lib/functions/logging/runners.sh (2)
chroot_sdcard(133-136)run_host_command_logged(204-206).github/generate_CODEOWNERS.sh (1)
display_alert(6-6)extensions/radxa-aic8800.sh (1)
post_install_kernel_debs__install_aic8800_dkms_package(11-63)
🔇 Additional comments (4)
packages/bsp/ti/emptty/pamconf (1)
1-10: PAM stack order looks sane for emptty.
- auth: short-circuits for nopasswdlogin before common-auth.
- account/session/password: defer to debian “common-*”.
No functional concerns from a PAM perspective.packages/bsp/ti/emptty/emptty.conf (3)
20-21: Verify session name matches .desktop entry.AUTOLOGIN_SESSION expects the Name field from /usr/share/wayland-sessions/weston.desktop. On Debian that is typically “Weston”, but please verify on the target suite to avoid a fallback to manual selection.
Would you like a small guard in the hook to read the .desktop Name and write it here automatically if Weston is installed?
29-33: DBus launch is fine; keep XINITRC_LAUNCH=false for Wayland.No issues; these defaults match a Weston Wayland session started via a .desktop entry.
64-66: Good: session error logging enabled (rotate).This will help diagnose early compositor failures without spamming the journal.
packages/bsp/ti/emptty/emptty.conf
Outdated
| AUTOLOGIN_MAX_RETRY=-1 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid infinite restart loops on session crash.
AUTOLOGIN_MAX_RETRY=-1 risks tight respawn loops if Weston fails repeatedly (e.g., missing GPU module). Recommend a finite retry count with a cool-off or fallback to the greeter.
Example change:
-AUTOLOGIN_MAX_RETRY=-1
+AUTOLOGIN_MAX_RETRY=5📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AUTOLOGIN_MAX_RETRY=-1 | |
| AUTOLOGIN_MAX_RETRY=5 |
🤖 Prompt for AI Agents
In packages/bsp/ti/emptty/emptty.conf around lines 23-24, AUTOLOGIN_MAX_RETRY is
set to -1 which allows infinite respawn loops if the session/Weston keeps
crashing; change this to a finite retry count (e.g., 3) and implement a
cool-off/backoff and fallback behavior so after the retry limit is reached the
system either waits progressively longer between attempts or launches the
greeter instead of continuously respawning the session. Ensure the config and
the session supervisor handle decrementing retries, a delay/backoff mechanism,
and a clear handoff to the greeter when retries are exhausted.
77db37c to
b3f6ff5
Compare
| TI_PACKAGES+=("weston" "emptty") | ||
| fi | ||
|
|
||
| TI_PACKAGES+=("firmware-cnm-wave" "firmware-ti-ipc-${SOC_ID}" "firmware-ti-connectivity") |
There was a problem hiding this comment.
We should fix up the packaging to that firmware-ti-ipc is not SoC specific. The files are small and do not conflict with each other, better to just have a common package, that is more inline with the "Debian" way of packaging.
There was a problem hiding this comment.
Good point!
If you want to push some files to a package armbian-ti-common in APA, just let me know. Alternatively, there is of course also the armbian-firmware package where these files could be added.
config/boards/sk-am64b.conf
Outdated
| OPTEE_PLATFORM="k3-am64x" | ||
| SOC_ID="am64" | ||
| EXTRA_BOARD_PACKAGES=("firmware-ti-prueth-am64" "firmware-ti-pruhsr-am64" "firmware-ti-prusw-am64") | ||
| HAS_VIDEO_OUTPUT="no" |
jsuhaas22
force-pushed
the
add-configure-more-packages
branch
from
b3f6ff5 to
13c670c
Compare
github-actions
bot
added
size/small
|
For now, based on internal discussions, I have removed the weston and emptty. It was decided that we should explore ways to introduce these as a separate desktop environment or something similar. So from now, this PR is for adding extra packages related to vim, firmware, kernel modules etc. Therefore I have also edited the PR heading. |
jsuhaas22
changed the title
The first patch still includes |
TI maintains a bunch of Debian packages for TI devices, such as a diagnostic tool (k3conf), firmware, GPU out-of-tree modules and mesa. Add these packages to the Debian image by-default. The GPU package is built using dkms. Add an explicit step to bind dkms packages to kernel. Currently the dkms package gets installed before the kernel, therefore if this step is excluded, the binding would occur on first boot, which would cause the first boot time to get longer. Therefore add the "dkms autoinstall" step. [0] https://www.debian.org/doc/manuals/maint-guide/dother.en.html#conffiles Signed-off-by: Suhaas Joshi <[email protected]>
`vim` is pretty widely used, so include it in the list of default packages to install in a CLI image. Signed-off-by: Suhaas Joshi <[email protected]>
jsuhaas22
force-pushed
the
add-configure-more-packages
branch
from
13c670c to
bf3d89c
Compare
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
TI Debian images support different features, such as IPC, Weston (as OOB screen), GPU acceleration, camera etc. So to have these working by-default in our images, this PR adds certain packages such as:
Additionally, there are 2 more things done here:
/lib/systemd/system/. This is to start weston on boot by-default. Note that weston doesn't launch on first boot when the armbian-firstlogin script runs, but starts launching from subsequent boots.dkms autoinstallbuilds and installs the ti-img-rogue--dkms package for the current kernel. If this isn't done here, it will done during first boot, which would make the first boot very slow.How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.
Checklist:
Please delete options that are not relevant.