Skip to content

Conversation

shramov
Copy link
Contributor

@shramov shramov commented Sep 14, 2025

Problem solved by the commit

Debian build rules are not used in regular builds and break from time to time.

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

How problem was solved, alternative solutions (if any) and why they were rejected

Workflow uses dependencies list extracted from debian/control with simple perl script to make sure they are valid.

Risks (if any) associated the changes in the commit

What has been tested and how, request additional testing if necessary

Documentation impact (if any)

This is draft until some issues are clarified and resolved:

  • It looks like packaging is gradually shifting from /opt/xilinx/xrt to /usr (see f47d3cd for example). Is it true?
  • What is minimal amount of tests that should be performed on installed package? Calling xclbinutil and compiling xrt/00_hello tests is enought?
  • This pull request uses ccache (that greatly reduces compilation time) that depends on Add minimal version info header to help compiler cache #9231 (if it is not merged then using ccache does not make sense).
  • Also there are files installed (but not packaged) in /usr/src/xrt-aws-2.20.0 (dkms files reports that this is xrt-awsmgmt driver). Should it be packaged into xrt-aws-dkms package?

xrt/detail/abi.h includes autogenerated version.h that contains timestamp
but use only major/minor version macros. Using smaller header enables
use of compiler caches (like ccache).

Signed-off-by: Pavel Shramov <[email protected]>
@shramov shramov marked this pull request as draft September 14, 2025 17:57
@gbuildx
Copy link
Collaborator

gbuildx commented Sep 14, 2025

Can one of the admins verify this patch?

@gbuildx
Copy link
Collaborator

gbuildx commented Sep 14, 2025

shramov - is not a collaborator
Can XRT admins please validate PR

@gbuildx
Copy link
Collaborator

gbuildx commented Sep 14, 2025

shramov - is not a collaborator
Can XRT admins please validate PR

Install dependencies described in debian/control file, build package
and run basic tests with installed result

Signed-off-by: Pavel Shramov <[email protected]>
@gbuildx
Copy link
Collaborator

gbuildx commented Sep 15, 2025

shramov - is not a collaborator
Can XRT admins please validate PR

@stsoe
Copy link
Collaborator

stsoe commented Sep 15, 2025

It looks like packaging is gradually shifting from /opt/xilinx/xrt to /usr (see f47d3cd for example). Is it true?

The CMake files were changed to obey and never set CMAKE_INSTALL_PREFIX. So it's entirely up to the build to specify the install for packages.

What is minimal amount of tests that should be performed on installed package? Calling xclbinutil and compiling xrt/00_hello tests is enough?

Sound sufficient to me.

This pull request uses ccache (that greatly reduces compilation time) that depends on #9231 (if it is not merged then using ccache does not make sense).

I will look at #9231. However, ccache is already supported with build.sh. It sets -DXRT_CCACHE=1 during configuration and disables inclusion of version.h. Do we need anything additional? I do like the fact that the generated version file is in the same relative location as the installed header.

Also there are files installed (but not packaged) in /usr/src/xrt-aws-2.20.0 (dkms files reports that this is xrt-awsmgmt driver). Should it be packaged into xrt-aws-dkms package?

@ManojTakasi , @chvamshi-xilinx ?

@shramov
Copy link
Contributor Author

shramov commented Sep 16, 2025

The CMake files were changed to obey and never set CMAKE_INSTALL_PREFIX. So it's entirely up to the build to specify the install for packages.

But what is right way in your opinion? Use /opt/xilinx/xrt or /usr for packaging? Official packages (last available at least) use /opt/xilinx/xrt.

I will look at #9231. However, ccache is already supported with build.sh. It sets -DXRT_CCACHE=1 during configuration and disables inclusion of version.h. Do we need anything additional? I do like the fact that the generated version file is in the same relative location as the installed header.

Oh, i've missed XRT_CCACHE define, but it is not needed if datetime field is not included into whole build tree. Please take a look at #9231, maybe you'll find it usefull.

@stsoe
Copy link
Collaborator

stsoe commented Sep 16, 2025

But what is right way in your opinion? Use /opt/xilinx/xrt or /usr for packaging? Official packages (last available at least) use /opt/xilinx/xrt.

I am under the impression that upstream is in control of CMAKE_INSTALL_PREFIX. I am working on a Debian upstream for XRT for NPU. It will use /usr as the install root. I am not familiar with what build/debian is used for or by whom? But I would not recommend changing the install location as this time; let's see if ultimately this build/debian can be merged with what I am working on.

Please take a look at #9231, maybe you'll find it usefull.

Yes, very useful. Thank you.

@shramov
Copy link
Contributor Author

shramov commented Sep 16, 2025

I am not familiar with what build/debian is used for or by whom?

We were using build/debian (with minor tweaks) for 3+ years for Alveo cards but it looks like I've picked up wrong base for packaging.

At first glance 'official' debianization that can be found at https://salsa.debian.org/xilinx-packages-team/xrt lacks xrt-xocl-dkms. So at least for now build/debian is pretty useful.

I'll try to use package from salsa and report back.

@stsoe
Copy link
Collaborator

stsoe commented Sep 16, 2025

I am not familiar with what build/debian is used for or by whom?

We were using build/debian (with minor tweaks) for 3+ years for Alveo cards but it looks like I've picked up wrong base for packaging.

At first glance 'official' debianization that can be found at https://salsa.debian.org/xilinx-packages-team/xrt lacks xrt-xocl-dkms. So at least for now build/debian is pretty useful.

I'll try to use package from salsa and report back.

The salsa repo XRT packages is what I am working on updating. But this upstreaming will change to support NPU only. It will not be an edge build and there will be no xocl-dkms. It sounds like buid/debian serves a different purpose.

@shramov
Copy link
Contributor Author

shramov commented Sep 17, 2025

The salsa repo XRT packages is what I am working on updating. But this upstreaming will change to support NPU only. It will not be an edge build and there will be no xocl-dkms. It sounds like buid/debian serves a different purpose.

Is it true that XRT_NPU and XRT_ALVEO (or XRT_XRT) modes of compilation are in conflict? It looks like that enabling both (NPU and ALVEO or NPU and XRT) is possible.

XRT_NPU mostly affect CMake component names that are not used in packaging, at least one test is disabled (xclbinutil file-check) and src/runtime_src/core/common/xdp/profile.cpp compiled with -DXDP_CLIENT_BUILD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants