Skip to content

Conversation

@kashif
Copy link

@kashif kashif commented Nov 16, 2025

This commit adds complete support for building and installing the AMD XDNA driver on Arch Linux. Needs Xilinx/XRT#9442

Changes:

  • tools/amdxdna_deps.sh: Add pacman support for jq installation
  • CMake/pkg.cmake: Add Arch Linux detection and TGZ package generation
  • xrt submodule: Updated with Arch Linux package list and Boost 1.69+ compatibility
  • README.md: Add detailed Arch Linux installation instructions

New files:

  • PKGBUILD-xrt-base: Arch package build script for XRT base
  • PKGBUILD-xrt-npu: Arch package build script for XRT NPU
  • PKGBUILD-xrt-plugin: Arch package build script for XDNA driver plugin
  • xrt-plugin-amdxdna.install: Pacman install hooks for DKMS and kernel module

The build system now properly detects Arch Linux and generates .tar.gz packages that can be repackaged into proper Arch packages (.pkg.tar.zst) using the provided PKGBUILDs.

Tested on Arch Linux with kernel 6.17.8 and AMD Ryzen AI 9 HX 370 (NPU Strix).

@AMDGithubSCIMAdmin
Copy link

Can one of the admins verify this patch?

This commit adds complete support for building and installing the AMD XDNA
driver on Arch Linux.

Changes:
- tools/amdxdna_deps.sh: Add pacman support for jq installation
- CMake/pkg.cmake: Add Arch Linux detection and TGZ package generation
- xrt submodule: Updated with Arch Linux support (packaging + dependencies)
- README.md: Add detailed Arch Linux installation instructions

New files:
- PKGBUILD-xrt-plugin: Arch package build script for XDNA driver plugin
- xrt-plugin-amdxdna.install: Pacman install hooks for DKMS and kernel module

XRT-specific packaging:
- XRT PKGBUILDs moved to xrt/packaging/arch/ (maintained in XRT submodule)
- See xrt/packaging/arch/README.md for XRT packaging instructions

The build system now properly detects Arch Linux and generates .tar.gz
packages that can be repackaged into proper Arch packages (.pkg.tar.zst)
using the provided PKGBUILDs.

Tested on Arch Linux with kernel 6.17.8 and AMD Ryzen AI 9 HX 370 (NPU Strix).
- Point .gitmodules to https://github.com/kashif/XRT.git arch branch
- Ensures users get Arch-specific XRT patches automatically
- Move Arch packaging to xrt/build/arch/ to match existing convention
- XRT already has build/debian/, so build/arch/ is consistent
- Update README.md with correct paths
- Update xrt submodule reference
- Replace absolute paths with relative paths using XDNA_BUILD_DIR variable
- Default to build/Release (relative path)
- Allow customization via environment variable
- Add error checking for missing tarballs
- Makes PKGBUILD portable across different systems
- Use $startdir to properly resolve paths when makepkg changes to $srcdir
- Fixes tarball and install script paths
- Update xrt submodule reference to include PKGBUILD fixes
Copilot AI review requested due to automatic review settings November 22, 2025 11:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds comprehensive Arch Linux support for building and installing the AMD XDNA driver, enabling it to work alongside existing Ubuntu/Debian and Fedora support.

Key changes:

  • Adds Arch Linux package manager (pacman) support for dependency installation
  • Implements TGZ package generation with Arch Linux detection in CMake
  • Provides PKGBUILD scripts for creating native Arch packages and pacman install hooks for DKMS management

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
xrt Updates XRT submodule to include Arch Linux support
tools/amdxdna_deps.sh Adds pacman package manager detection for jq installation
build/xrt-plugin-amdxdna.install New pacman install/remove hooks for DKMS and kernel module management
build/PKGBUILD-xrt-plugin New Arch package build script for XDNA driver plugin
README.md Documents Arch Linux build and installation process
CMake/pkg.cmake Adds Arch Linux detection and TGZ package generation support
.gitmodules Points XRT submodule to fork with Arch support

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -0,0 +1,38 @@
# Maintainer: Your Name <[email protected]>
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maintainer information should be updated with actual contact details before distribution.

Suggested change
# Maintainer: Your Name <your@email.com>
# Maintainer: Jane Doe <jane.doe@example.com>

Copilot uses AI. Check for mistakes.
.gitmodules Outdated
Comment on lines 3 to 4
url = https://github.com/kashif/XRT.git
branch = arch
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The XRT submodule is now pointing to a personal fork instead of the official Xilinx/XRT repository. This should be temporary until the changes are merged upstream. Consider documenting this dependency in the PR description or adding a comment explaining when this will be reverted to the official repository.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings November 22, 2025 11:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if [ ! -f ${udev_rules_d}/${amdxdna_rules_file} ]; then
touch ${udev_rules_d}/${amdxdna_rules_file}
fi
echo 'KERNEL=="accel*",DRIVERS=="amdxdna",MODE="0666"' > ${udev_rules_d}/${amdxdna_rules_file}
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting MODE="0666" grants read/write access to all users for the device. This may be overly permissive. Consider using a dedicated group (e.g., MODE="0660", GROUP="render") to restrict access to authorized users only.

Suggested change
echo 'KERNEL=="accel*",DRIVERS=="amdxdna",MODE="0666"' > ${udev_rules_d}/${amdxdna_rules_file}
echo 'KERNEL=="accel*",DRIVERS=="amdxdna",MODE="0660",GROUP="render"' > ${udev_rules_d}/${amdxdna_rules_file}

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings November 22, 2025 12:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

message(STATUS "Note: For Arch Linux, you'll need to run these scripts manually or create a PKGBUILD")
endif()
else("${XDNA_CPACK_LINUX_PKG_FLAVOR}" MATCHES "debian")
message(FATAL_ERROR "Unknown Linux package flavor: ${XDNA_CPACK_LINUX_PKG_FLAVOR}")
Copy link

Copilot AI Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling for unknown package flavors should include 'arch' in the conditional chain. The final else clause will incorrectly catch Arch Linux as an unknown flavor if the elseif on line 132 doesn't match. Consider changing line 143 to else() without the condition, or ensure the Arch detection pattern is comprehensive.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings November 24, 2025 12:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

CMake/pkg.cmake Outdated
elseif("${XDNA_CPACK_LINUX_PKG_FLAVOR}" MATCHES "arch")
set(CPACK_GENERATOR "TGZ")
# For Arch Linux, we generate a tarball that can be used for manual installation
# or to create a PKGBUILD. Post-install scripts need to be run manually.
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states 'Post-install scripts need to be run manually', but the PKGBUILD-xrt-plugin file and xrt-plugin-amdxdna.install show that install scripts are automatically executed by pacman. This documentation is misleading and should clarify that when using the provided PKGBUILD, the install hooks handle this automatically.

Suggested change
# or to create a PKGBUILD. Post-install scripts need to be run manually.
# or to create a PKGBUILD. When using the provided PKGBUILD, post-install scripts are executed automatically by pacman. If installing manually from the TGZ, you need to run the post-install scripts yourself.

Copilot uses AI. Check for mistakes.
Arch Linux changes have been merged upstream, so we can now point
to the official XRT repository instead of the fork.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Copilot AI review requested due to automatic review settings November 28, 2025 22:59
Copilot finished reviewing on behalf of kashif November 28, 2025 23:03
Applied review suggestions from PR amd#866:
- Clarify that PKGBUILD handles install hooks automatically via pacman
- Fix incorrect else("condition") syntax to proper else()
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.

Comments suppressed due to low confidence (1)

CMake/pkg.cmake:145

  • The else statement incorrectly references "debian" instead of being a plain else(). In CMake, when using else(), you should not provide a condition. This should be just else() without the condition argument. Similarly, line 145's endif() should also be a plain endif() without the condition.
else("${XDNA_CPACK_LINUX_PKG_FLAVOR}" MATCHES "debian")
  message(FATAL_ERROR "Unknown Linux package flavor: ${XDNA_CPACK_LINUX_PKG_FLAVOR}")
endif("${XDNA_CPACK_LINUX_PKG_FLAVOR}" MATCHES "debian")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 21 to 22
# The tarball should be at $startdir/$XDNA_BUILD_DIR/xrt_plugin*.tar.gz
local tarballs=("$startdir/${XDNA_BUILD_DIR}"/xrt_plugin*-amdxdna.tar.gz)
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array check [ ${#tarballs[@]} -eq 0 ] will not work as expected because bash glob expansion that finds no matches will result in an array with one element containing the literal pattern. The correct check should be [ ! -f "${tarballs[0]}" ] only, or use shopt -s nullglob before the glob to handle no matches properly.

Suggested change
# The tarball should be at $startdir/$XDNA_BUILD_DIR/xrt_plugin*.tar.gz
local tarballs=("$startdir/${XDNA_BUILD_DIR}"/xrt_plugin*-amdxdna.tar.gz)
# The tarball should be at $startdir/$XDNA_BUILD_DIR/xrt_plugin*.tar.gz
local _nullglob_set=$(shopt -p nullglob)
shopt -s nullglob
local tarballs=("$startdir/${XDNA_BUILD_DIR}"/xrt_plugin*-amdxdna.tar.gz)
$_nullglob_set

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,39 @@
# Maintainer: Your Name <[email protected]>
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maintainer information is a placeholder. Replace "Your Name <[email protected]>" with actual maintainer details before distribution. This is important for package maintainability and user support.

Suggested change
# Maintainer: Your Name <your@email.com>
# Maintainer: Jane Doe <jane.doe@example.com>

Copilot uses AI. Check for mistakes.
Comment on lines 22 to 29
local tarballs=("$startdir/${XDNA_BUILD_DIR}"/xrt_plugin*-amdxdna.tar.gz)
if [ ${#tarballs[@]} -eq 0 ] || [ ! -f "${tarballs[0]}" ]; then
error "XDNA plugin tarball not found in ${XDNA_BUILD_DIR}"
error "Please build XDNA driver first: cd build && ./build.sh -release"
return 1
fi

local tarball="${tarballs[0]}"
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The glob pattern uses a generic wildcard that could match multiple tarballs if different versions exist. Consider using a more specific pattern or add logic to handle multiple matches, selecting the most recent one if needed.

Suggested change
local tarballs=("$startdir/${XDNA_BUILD_DIR}"/xrt_plugin*-amdxdna.tar.gz)
if [ ${#tarballs[@]} -eq 0 ] || [ ! -f "${tarballs[0]}" ]; then
error "XDNA plugin tarball not found in ${XDNA_BUILD_DIR}"
error "Please build XDNA driver first: cd build && ./build.sh -release"
return 1
fi
local tarball="${tarballs[0]}"
# Find all matching tarballs and select the most recently modified one
local tarball
tarball=$(ls -1t "$startdir/${XDNA_BUILD_DIR}"/xrt_plugin*-amdxdna.tar.gz 2>/dev/null | head -n 1)
if [ -z "$tarball" ] || [ ! -f "$tarball" ]; then
error "XDNA plugin tarball not found in ${XDNA_BUILD_DIR}"
error "Please build XDNA driver first: cd build && ./build.sh -release"
return 1
fi

Copilot uses AI. Check for mistakes.

# Install DKMS module
echo "Installing amdxdna Linux kernel module via DKMS..."
$install_datadir/dkms_driver.sh --install
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script executes dkms_driver.sh --install without checking if the script exists or if it's executable. If the script is missing or fails, the installation will fail without a clear error message. Consider adding an existence check and proper error handling.

Suggested change
$install_datadir/dkms_driver.sh --install
if [ ! -x "$install_datadir/dkms_driver.sh" ]; then
echo "Error: $install_datadir/dkms_driver.sh not found or not executable." >&2
return 1
fi
"$install_datadir/dkms_driver.sh" --install

Copilot uses AI. Check for mistakes.
# Load the kernel module
echo "Loading amdxdna kernel module..."
rmmod amdxdna > /dev/null 2>&1 || true
modprobe amdxdna
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The modprobe amdxdna command can fail if the module isn't built or if there are dependency issues, but the script doesn't check its exit status. Consider adding error handling to inform the user if the module fails to load, e.g., modprobe amdxdna || echo "Warning: Failed to load amdxdna module. Check dmesg for details."

Suggested change
modprobe amdxdna
modprobe amdxdna || echo "Warning: Failed to load amdxdna module. Check dmesg for details."

Copilot uses AI. Check for mistakes.
# Log out and log back in (or reboot) for memory limit changes to take effect
```

**Note for Arch Linux users**: The build system generates `.tar.gz` packages which are repackaged into proper Arch packages (`.pkg.tar.zst`) using the provided PKGBUILDs:
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note mentions that XRT packages are in xrt/build/arch/, but the instructions in lines 136-147 show building XRT in xrt/build and then moving to a subdirectory arch. It would be clearer to explicitly state that the PKGBUILDs are generated during the XRT build process, or if they're provided by XRT, to mention that upfront.

Suggested change
**Note for Arch Linux users**: The build system generates `.tar.gz` packages which are repackaged into proper Arch packages (`.pkg.tar.zst`) using the provided PKGBUILDs:
**Note for Arch Linux users**: The build system generates `.tar.gz` packages which are repackaged into proper Arch packages (`.pkg.tar.zst`) using PKGBUILDs.
The PKGBUILD files for XRT (`PKGBUILD-xrt-base`, `PKGBUILD-xrt-npu`) and the XDNA driver (`PKGBUILD-xrt-plugin`) are provided by the XRT repository and are available in the following locations after building:

Copilot uses AI. Check for mistakes.
elif [ -x "$(command -v dnf)" ]; then
dnf install -y jq
elif [ -x "$(command -v pacman)" ]; then
pacman -Syu --needed --noconfirm jq
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -Syu flag performs a full system upgrade in addition to installing jq, which is inconsistent with the behavior of apt-get install and dnf install on lines 10 and 12. For consistency, use pacman -S --needed --noconfirm jq instead, which only installs the package without upgrading the entire system.

Suggested change
pacman -Syu --needed --noconfirm jq
pacman -S --needed --noconfirm jq

Copilot uses AI. Check for mistakes.
dracut_conf_file=amdxdna.dracut.conf

# On systems with dracut, exclude driver from initramfs
if [ -e ${dracut_conf_d} ]; then
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional check uses -e to test for directory existence, but should use -d instead. The -e flag tests for file existence (any type), while -d specifically tests for directories. This could lead to unexpected behavior if a file with the same name exists.

Suggested change
if [ -e ${dracut_conf_d} ]; then
if [ -d ${dracut_conf_d} ]; then

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +28
grep -q '^KERNEL=="accel\*",DRIVERS=="amdxdna",MODE="0666"$' ${udev_rules_d}/${amdxdna_rules_file} || \
echo 'KERNEL=="accel*",DRIVERS=="amdxdna",MODE="0666"' >> ${udev_rules_d}/${amdxdna_rules_file}
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The udev rule sets MODE="0666" which grants read and write access to all users. While this may be intentional for device access, it could pose a security risk by allowing any user to access the NPU device. Consider using GROUP-based permissions with a dedicated group (e.g., "render" or "video") and MODE="0660" instead.

Suggested change
grep -q '^KERNEL=="accel\*",DRIVERS=="amdxdna",MODE="0666"$' ${udev_rules_d}/${amdxdna_rules_file} || \
echo 'KERNEL=="accel*",DRIVERS=="amdxdna",MODE="0666"' >> ${udev_rules_d}/${amdxdna_rules_file}
grep -q '^KERNEL=="accel\*",DRIVERS=="amdxdna",GROUP="video",MODE="0660"$' ${udev_rules_d}/${amdxdna_rules_file} || \
echo 'KERNEL=="accel*",DRIVERS=="amdxdna",GROUP="video",MODE="0660"' >> ${udev_rules_d}/${amdxdna_rules_file}

Copilot uses AI. Check for mistakes.
conflicts=('amdxdna-driver' 'amdxdna-driver-bin')
install=xrt-plugin-amdxdna.install

# Set this to your xdna-driver build output directory (relative to PKGBUILD location)
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "relative to PKGBUILD location", but $startdir in the package() function is actually the directory containing the PKGBUILD, making this slightly misleading. Consider clarifying that XDNA_BUILD_DIR is relative to the PKGBUILD's directory (i.e., $startdir).

Suggested change
# Set this to your xdna-driver build output directory (relative to PKGBUILD location)
# Set this to your xdna-driver build output directory (relative to the PKGBUILD's directory, i.e., $startdir)

Copilot uses AI. Check for mistakes.
- Use -d instead of -e for directory check in dracut config
- Add existence check for dkms_driver.sh before execution
- Add warning message if modprobe amdxdna fails
- Change pacman -Syu to -S to avoid full system upgrade
- Clarify PKGBUILD comment about $startdir
Copilot AI review requested due to automatic review settings December 3, 2025 08:32
Copilot finished reviewing on behalf of kashif December 3, 2025 08:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.

Comments suppressed due to low confidence (1)

build/xrt-plugin-amdxdna.install:32

  • The udev rule sets MODE="0666" which grants read/write access to all users without any authentication. While this may be intentional for device access, it's a potential security concern as any user on the system can access the NPU hardware. Consider if MODE="0660" with a specific group (e.g., "render" or "video") would be more appropriate, requiring users to be added to that group for access.
  grep -q '^KERNEL=="accel\*",DRIVERS=="amdxdna",MODE="0666"$' ${udev_rules_d}/${amdxdna_rules_file} || \
    echo 'KERNEL=="accel*",DRIVERS=="amdxdna",MODE="0666"' >> ${udev_rules_d}/${amdxdna_rules_file}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

install=xrt-plugin-amdxdna.install

# Set this to your xdna-driver build output directory (relative to $startdir, the PKGBUILD's directory)
: ${XDNA_BUILD_DIR:="Release"}
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable assignment : ${XDNA_BUILD_DIR:="Release"} uses the colon null command which is a valid bash idiom for setting defaults. However, this pattern is unconventional in PKGBUILDs. The standard approach would be to use XDNA_BUILD_DIR=${XDNA_BUILD_DIR:-Release} which is clearer and more maintainable.

Suggested change
: ${XDNA_BUILD_DIR:="Release"}
XDNA_BUILD_DIR=${XDNA_BUILD_DIR:-Release}

Copilot uses AI. Check for mistakes.

# Unload kernel module
if lsmod | grep -q "amdxdna"; then
rmmod amdxdna
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rmmod amdxdna command has no error handling. If the module is in use or fails to unload for any reason, the script will fail silently or abort. Consider adding error handling with a descriptive message: rmmod amdxdna || echo "Warning: Failed to unload amdxdna module. It may be in use." This is especially important during package removal when users need to understand why the removal failed.

Suggested change
rmmod amdxdna
rmmod amdxdna || echo "Warning: Failed to unload amdxdna module. It may be in use."

Copilot uses AI. Check for mistakes.
fi

# Remove DKMS module
$install_datadir/dkms_driver.sh --remove
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DKMS removal script is called without error handling or existence check, unlike the installation which checks for the script's existence (line 20-23). If the script doesn't exist or fails, the removal will abort with an unclear error. Consider adding: if [ -x "$install_datadir/dkms_driver.sh" ]; then "$install_datadir/dkms_driver.sh" --remove || echo "Warning: Failed to remove DKMS module"; fi

Suggested change
$install_datadir/dkms_driver.sh --remove
if [ -x "$install_datadir/dkms_driver.sh" ]; then
"$install_datadir/dkms_driver.sh" --remove || echo "Warning: Failed to remove DKMS module"
fi

Copilot uses AI. Check for mistakes.
Comment on lines +35 to +38
tar -xzf "$tarball" -C "$pkgdir"

# Copy the install scripts for reference (they'll be called from .install file)
mkdir -p "$pkgdir/opt/xilinx/xrt/share/amdxdna/package"
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The install commands lack proper error handling. If the postinst or prerm files don't exist in the expected location, the install command will fail silently or cause the build to fail. Consider adding existence checks before these install commands or using || true if the files are optional.

Suggested change
tar -xzf "$tarball" -C "$pkgdir"
# Copy the install scripts for reference (they'll be called from .install file)
mkdir -p "$pkgdir/opt/xilinx/xrt/share/amdxdna/package"
if [ -f "$startdir/${XDNA_BUILD_DIR}/package/postinst" ]; then
install -Dm755 "$startdir/${XDNA_BUILD_DIR}/package/postinst" \
"$pkgdir/opt/xilinx/xrt/share/amdxdna/package/postinst"
fi
if [ -f "$startdir/${XDNA_BUILD_DIR}/package/prerm" ]; then
install -Dm755 "$startdir/${XDNA_BUILD_DIR}/package/prerm" \
"$pkgdir/opt/xilinx/xrt/share/amdxdna/package/prerm"
fi

Copilot uses AI. Check for mistakes.
Comment on lines +158 to +159
sudo bash -c 'echo "* soft memlock unlimited" >> /etc/security/limits.conf'
sudo bash -c 'echo "* hard memlock unlimited" >> /etc/security/limits.conf'
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using bash -c with echo and >> redirection is unnecessarily complex and potentially error-prone. A more robust approach would be to use tee -a or create a dedicated script. Additionally, directly appending to /etc/security/limits.conf without checking for existing entries could create duplicates on repeated runs. Consider: echo "* soft memlock unlimited" | sudo tee -a /etc/security/limits.conf or add a check to prevent duplicates.

Suggested change
sudo bash -c 'echo "* soft memlock unlimited" >> /etc/security/limits.conf'
sudo bash -c 'echo "* hard memlock unlimited" >> /etc/security/limits.conf'
grep -qxF '* soft memlock unlimited' /etc/security/limits.conf || echo '* soft memlock unlimited' | sudo tee -a /etc/security/limits.conf
grep -qxF '* hard memlock unlimited' /etc/security/limits.conf || echo '* hard memlock unlimited' | sudo tee -a /etc/security/limits.conf

Copilot uses AI. Check for mistakes.
Comment on lines +37 to +38
modprobe amdxdna || echo "Warning: Failed to load amdxdna module. Check dmesg for details."

Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message "Warning: Failed to load amdxdna module. Check dmesg for details." could be more helpful by including the actual error code or suggesting specific troubleshooting steps. Consider capturing the modprobe exit status and including it in the message, or providing common failure reasons (e.g., kernel version mismatch, missing dependencies).

Suggested change
modprobe amdxdna || echo "Warning: Failed to load amdxdna module. Check dmesg for details."
if ! modprobe amdxdna; then
exit_code=$?
echo "Warning: Failed to load amdxdna module (exit code: $exit_code)."
echo "Check 'dmesg' for details. Common reasons for failure:"
echo " - Kernel version mismatch"
echo " - Missing dependencies (run 'depmod -a' and try again)"
echo " - Secure Boot enabled (may block unsigned modules)"
fi

Copilot uses AI. Check for mistakes.
Comment on lines +35 to +37
tar -xzf "$tarball" -C "$pkgdir"

# Copy the install scripts for reference (they'll be called from .install file)
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path $startdir/${XDNA_BUILD_DIR}/package/postinst uses unquoted variable expansion which could fail if XDNA_BUILD_DIR contains spaces or special characters. While "Release" is the default and unlikely to have spaces, it's good practice to quote variable expansions: "$startdir/${XDNA_BUILD_DIR}/package/postinst"

Suggested change
tar -xzf "$tarball" -C "$pkgdir"
# Copy the install scripts for reference (they'll be called from .install file)
install -Dm755 "$startdir/${XDNA_BUILD_DIR}/package/postinst" \
"$pkgdir/opt/xilinx/xrt/share/amdxdna/package/postinst"
install -Dm755 "$startdir/${XDNA_BUILD_DIR}/package/prerm" \

Copilot uses AI. Check for mistakes.
Comment on lines +144 to +145
else()
message(FATAL_ERROR "Unknown Linux package flavor: ${XDNA_CPACK_LINUX_PKG_FLAVOR}")
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition will never be true because if XDNA_CPACK_LINUX_PKG_FLAVOR matches "debian", it would have already been caught by the first if statement at line 116. This appears to be a copy-paste error. Consider removing this unreachable code block or changing the condition if it was intended to check for a different package flavor.

Copilot uses AI. Check for mistakes.
echo "Installing amdxdna Linux kernel module via DKMS..."
if [ ! -x "$install_datadir/dkms_driver.sh" ]; then
echo "Error: $install_datadir/dkms_driver.sh not found or not executable." >&2
return 1
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is written to stderr but the function returns 1 without properly handling the error. In pacman install hooks, returning non-zero will abort the installation. However, this script appears to be designed to work as a shell script sourced by pacman hooks, where return statements may not behave as expected. Consider using exit 1 instead of return 1 or document the expected behavior more clearly.

Suggested change
return 1
exit 1

Copilot uses AI. Check for mistakes.

This ensures proper integration with pacman for installation, upgrades, and removal.

You will find `xrt_plugin*-amdxdna.deb` (Ubuntu/Debian) or `xrt_plugin*.tar.gz` (Arch Linux) in Release/ folder. This package includes:
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation states that the package will be in the "Release/" folder, but for Arch Linux builds, the actual .pkg.tar.zst packages are created in the respective build directories (xrt/build/arch/ and build/). Only the intermediate .tar.gz file is in Release/. Consider clarifying: "You will find the intermediate xrt_plugin*.tar.gz in Release/ folder (which is repackaged into xrt-plugin-amdxdna-*.pkg.tar.zst using the PKGBUILD)."

Suggested change
You will find `xrt_plugin*-amdxdna.deb` (Ubuntu/Debian) or `xrt_plugin*.tar.gz` (Arch Linux) in Release/ folder. This package includes:
You will find the intermediate `xrt_plugin*.tar.gz` (Arch Linux) or `xrt_plugin*-amdxdna.deb` (Ubuntu/Debian) in the Release/ folder. For Arch Linux, this `.tar.gz` is repackaged into the final installable package `xrt-plugin-amdxdna-*.pkg.tar.zst` using the provided PKGBUILD, which can be found in the respective build directories (`xrt/build/arch/` or `build/`). These packages include:

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants