Bug 2480195
| Summary: | Review Request: xrt - AMD Xilinx Runtime (XRT) | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Soren Soe <stsoe> |
| Component: | Package Review | Assignee: | Tom.Rix |
| Status: | ASSIGNED --- | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | rawhide | CC: | package-review, Tom.Rix |
| Target Milestone: | --- | Flags: | Tom.Rix:
fedora-review?
|
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| URL: | https://github.com/Xilinx/XRT | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | --- | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | Type: | --- | |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Soren Soe
2026-05-20 16:04:33 UTC
Some broad strokes.
# SPDX-License-Identifier: Apache-2.0
# Spec layout mirrors src/debian/control and src/debian/rules.
# Assisted-by: Generic LLM chatbot
Fedora's default license is MIT, Apache is fine.
Remove the next two lines, no one will have the context of the debian package.
Assisted-by: and any other signoff should be in the git commit log and can be done later.
Release: 1%{?dist}
consider using the
Release: %autorelease
and later
%changelog
%autochangelog
Summary: AMD Xilinx FPGA and ACAP runtime (XRT)
I usually copy the 'about' section of the github page
consider using
Summary: Run Time for AIE and FPGA based platforms
License: Apache-2.0 AND MIT AND MIT-Khronos-old
# License breakdown:
# - Core XRT runtime: Apache-2.0
# - AIE binary utilities: MIT
# - Core XRT OpenCL library: Apache-2.0 and MIT-Khronos-old
Review the fedorareview output file licensecheck.
A lot of licenses were not captured in the license: tag
The breakdown of the licenses is too brief.
There are many license.txt or similar files not captured in the later %license in file
If code is not used, remove it in the %prep stage to make the license review easier.
Patch102: license.patch
remove this patch.
The full licenses are fine and any license changes should be done in the upstream not in distro patches.
# Debian patches
# Fedora patches
These are all fedora patches now, these comments are not needed
Patch0: 6.18.patch
Patch1: 6.19.patch
Patches should have a comment on what the patch does and if possible reference the upstream issue or pr
this is a general problem.
I find using git format patches more helpful as any updated to the package will require rebasing.
consider reworking these patches to be git friendly.
# Man pages not installed by CMake
Source10: aiebu-asm.1
Source11: aiebu-dump.1
How are these man pages generated ?
I see from aiebu-asm
SEE ALSO
The full documentation for aiebu-asm is maintained as a Texinfo manual. If the info and aiebu-asm programs are properly in‐
stalled at your site, the command
info aiebu-asm
they reference info, which afaik isn't in this package so these man pages will confuse users.
ExclusiveArch: aarch64 x86_64
Does this package and all its features work on aarch64 ?
hip i know is only x86_64.
BuildRequires: cmake >= 3.16
only 1 version of cmake, this is not needed.
The reset of the buildrequires looks autogenerated.
review what is actually needed and put then in a better order.
The packages are a matter of taste.
Mine is to limit the packages
I think the xrt-utils-* could be collapsed to the main package.
Similar for xrt-npu package.
For consistency with ROCm the programming interfaces opencl and hip should be
xrt-opencl, xrt-opencl-devel and xrt-hip and xrt-hip-devel
%cmake \
-DCMAKE_BUILD_TYPE=Release \
Release should change to RelWithDebInfo
%check
does this really work without hw ?
%files
...
%{_libdir}/libxrt++.so.*
This globbing it too aggressive.
At least the major so version needs to be captured.
consider something like
https://src.fedoraproject.org/rpms/rocclr/blob/rawhide/f/rocclr.spec#_440
The latest COPR build https://copr.fedorainfracloud.org/coprs/stsoe/xrt/build/10650329/ addresses most of the mentioned issues. What I am unsure about is the license breakdown. I removed all files under "Unknown or generated" that ware not needed, the remaining ones are needed and covered by what I listed under the breakdown. I have made upstream changes to fix some of the missing or incorrect licenses also. I did not combine or rename any packages, as mentioned it is a matter of choice and current packages reflect what we are doing for Debian. If a must, then of course this can be changed. However, I do not feel that utils* can be collapsed to main package; the main package is all that needed to for run time. The %check runs user space tests, there are no HW tests. Please advise what further changes are required. Thank you for your feedback. (In reply to Soren Soe from comment #2) > The latest COPR build > https://copr.fedorainfracloud.org/coprs/stsoe/xrt/build/10650329/ addresses > most of the mentioned issues. > > What I am unsure about is the license breakdown. I removed all files under > "Unknown or generated" that ware not needed, the remaining ones are needed > and covered by what I listed under the breakdown. I have made upstream > changes to fix some of the missing or incorrect licenses also. > > I did not combine or rename any packages, as mentioned it is a matter of > choice and current packages reflect what we are doing for Debian. If a > must, then of course this can be changed. However, I do not feel that > utils* can be collapsed to main package; the main package is all that needed > to for run time. > > The %check runs user space tests, there are no HW tests. > > Please advise what further changes are required. Thank you for your > feedback. Forgot to mention that the man pages are copied from Debian. The reference to info is the Linux info command. I can remove the man pages if necessary. Please add the updated spec and srpm this bz. copr builds are temporary, use a url that will be around for a while.
Use Spec/SRPM URL: <url to>
To be useful, this package should do something.
I likely have the hw needed to run this package, could you recommend something maybe using the lemonade server ?
here is what i get on strix halo, is aie2p supported by xrt runtime ?
*******
Agent 3
*******
Name: aie2p
Uuid: AIE-XX
Marketing Name: RyzenAI-npu5
Vendor Name: AMD
Feature: AGENT_DISPATCH
Profile: BASE_PROFILE
Float Round Mode: NEAR
Max Queue Number: 1(0x1)
Queue Min Size: 64(0x40)
Queue Max Size: 64(0x40)
Queue Type: SINGLE
Node: 0
Device Type: DSP
Cache Info:
L2: 2048(0x800) KB
L3: 32768(0x8000) KB
Chip ID: 0(0x0)
ASIC Revision: 0(0x0)
Cacheline Size: 0(0x0)
Max Clock Freq. (MHz): 0
BDFID: 0
Internal Node ID: 0
Compute Unit: 0
SIMDs per CU: 0
Shader Engines: 0
Shader Arrs. per Eng.: 0
WatchPts on Addr. Ranges:0
Memory Properties:
Features: AGENT_DISPATCH
Pool Info:
Pool 1
Segment: GLOBAL; FLAGS: KERNARG, COARSE GRAINED
Size: 131151216(0x7d13570) KB
Allocatable: TRUE
Alloc Granule: 4KB
Alloc Recommended Granule:4KB
Alloc Alignment: 4KB
Accessible by all: TRUE
Pool 2
Segment: GLOBAL; FLAGS: COARSE GRAINED
Size: 65536(0x10000) KB
Allocatable: TRUE
Alloc Granule: 4KB
Alloc Recommended Granule:0KB
Alloc Alignment: 4KB
Accessible by all: TRUE
Pool 3
Segment: GLOBAL; FLAGS: COARSE GRAINED
Size: 131151216(0x7d13570) KB
Allocatable: TRUE
Alloc Granule: 4KB
Alloc Recommended Granule:4KB
Alloc Alignment: 4KB
Accessible by all: TRUE
ISA Info:
*** Done ***
Not sure where to add a URL or SRPM when there is no server hosting XRT? Anyway, here is a link: - https://github.com/stsoe/XRT-DEBIAN/tree/2.21.75/src/fedora - https://github.com/stsoe/XRT-DEBIAN/blob/2.21.75/src/fedora/xrt-2.21.75-1.fc42.src.rpm The current packages created from the SRPM are functional. Minimum install these packages: - xrt-2.21.75-1.fc42.x86_64.rpm - xrt-npu-2.21.75-1.fc42.x86_64.rpm - xrt-utils-2.21.75-1.fc42.x86_64.rpm # Examine system and install % xrt-smi examine # Validate the system, follow instructions to install binary validation archive % xrt-smi validate AIE2P is supported. Copr build: https://copr.fedorainfracloud.org/coprs/build/10758829 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2480195-xrt/srpm-builds/10758829/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string. Corrected SRPM raw link: - https://raw.githubusercontent.com/stsoe/XRT-DEBIAN/2.21.75/src/fedora/xrt-2.21.75-1.fc42.src.rpm Updated links in description. Repeated here Spec URL: https://raw.githubusercontent.com/stsoe/XRT-DEBIAN/2.21.75/src/fedora/xrt.spec SRPM URL: https://raw.githubusercontent.com/stsoe/XRT-DEBIAN/2.21.75/src/fedora/xrt-2.21.75-1.fc42.src.rpm Copr build: https://copr.fedorainfracloud.org/coprs/build/10758907 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2480195-xrt/fedora-rawhide-x86_64/10758907-xrt/fedora-review/review.txt Found issues: - License file LICENSE.aie-rt is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text - Not a valid SPDX expression 'Apache-2.0 AND MIT AND Khronos'. Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 - Upstream MD5sum check error, diff is in /var/lib/copr-rpmbuild/results/xrt/diff.txt Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string. Copr build: https://copr.fedorainfracloud.org/coprs/build/10758922 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2480195-xrt/fedora-rawhide-x86_64/10758922-xrt/fedora-review/review.txt Found issues: - License file LICENSE.aie-rt is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text - Not a valid SPDX expression 'Apache-2.0 AND MIT AND Khronos'. Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 - Upstream MD5sum check error, diff is in /var/lib/copr-rpmbuild/results/xrt/diff.txt Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string. |