Skip to content

Downstream patches from Fedora #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

xanderlent
Copy link

@xanderlent xanderlent commented Feb 1, 2025

I wanted to let you folks know about the downstream changes I was making as we work on packaging this driver for Fedora.
When finished, I'd really appreciate any feedback you folks could give, since we prefer to reduce downstream patches when possible. Right now, the driver is incomplete; I'm trying to build the compiler as a separate component to reduce the number of inputs.

Summary of patches:

  • Add USE_SYSTEM_LIBRARIES option for distro packagers
    This adds a partially-finished option that turns off the bundled/vendored libraries as much as possible.
    I'm open to alternative approaches to using system dependencies; one other option would be to have several ENABLE_SYSTEM_WHATEVER options like OpenVINO does.
  • (not a patch in this tree) Make extension headers reference system headers
    As of v1.16.0, some change to the build process of the validation sub-tree doesn't look for ze_api.h in the system header locations, so the specfile patches the extension headers like so to reference the system headers:
    sed -i "s/#include \"ze_api.h\"/#include <level_zero\/ze_api.h>/" third_party/level-zero-npu-extensions/ze_graph_ext.h

Future Work:

  • Right now, the compiler-in-driver component uses an un-versioned shared object, which is generally prohibited in distro packages. Since the driver and compiler have a versioned interface it might make sense to version that file with the same number. I'll cross that bridge once I figure out how to package the compiler bits.

@jwludzik
Copy link
Contributor

jwludzik commented Feb 3, 2025

  • Make firmware respect CMAKE_INSTALL_PREFIX

We set fixed path for firmware installation to avoid copying the firmware outside the /lib/firmware. Using prefix might be very misleading, because user might copy it into /usr/local by mistake. Building Debian packages has the same issue with building. When using cmake --build <build-dir> --target packages the issue goes away. This solution requires to add RPM to https://github.com/intel/linux-npu-driver/tree/main/cmake/packaging

  • Fix compiler flags.

I was able to reproduce the issue only with -DCMAKE_BUILD_TYPE=Debug. I would recommend to add a condition for FORTIFY_SOURCE to be only applied for Release. This should fix a compiler warning. Adding -O is not desirable, because you are overriding -O2 flag (depends on CMake build type) with your own. This means the -O is fixed for all build types

@jwludzik
Copy link
Contributor

jwludzik commented Feb 3, 2025

To generate RPMs you can add:

elseif(EXISTS "/etc/redhat-release")
    set(PACKAGE_TYPE "rpm")

to

if (EXISTS "/etc/debian_version")
set(PACKAGE_TYPE "deb")
elseif(EXISTS "/etc/portage")
set(PACKAGE_TYPE "ebuild")
else()
message(WARNING "Unable to detect package type for this system")
return()
endif()

Create new file in cmake/packaging/generators/rpm.cmake

set(CPACK_GENERATOR RPM)
set(CPACK_RPM_COMPRESSION_TYPE "xz")
set(CPACK_RPM_COMPONENT_INSTALL ON)

And build package target:

cmake -B build -S .
cmake --build build -j8 --target package

@mateusztabaka
Copy link
Contributor

@xanderlent, I've tried to build rpm packages based on your spec and without "Make firmware respect CMAKE_INSTALL_PREFIX" patch and it seems to work fine for me:

# rpm -ql /root/rpmbuild/RPMS/noarch/intel-npu-firmware-1.13.0-1.fc41.noarch.rpm
/lib/firmware/updates/intel
/lib/firmware/updates/intel/vpu
/lib/firmware/updates/intel/vpu/mtl_vpu_v0.0.bin
/lib/firmware/updates/intel/vpu/vpu_37xx_v0.0.bin
/lib/firmware/updates/intel/vpu/vpu_40xx_v0.0.bin
/usr/share/licenses/intel-npu-firmware
/usr/share/licenses/intel-npu-firmware/COPYRIGHT

The absolute path for firmware is intentional.
In addition to what Józef said, in some distros /lib is not a symbolic link to /usr/lib, so it does make a difference whether you install the firmware to /lib/firmware or /usr/lib/firmware.

but it is also useful if you just want to install the driver to an arbitrary location to inspect the build outputs

For that cases, I think you can use DESTDIR variable.

@xanderlent
Copy link
Author

Thank you for the review! I appreciate the information about CMake as I am not particularly familiar with it.

I plan to create a revised patchset addressing the comments on this one.

I do not have a timeframe yet as my packaging work is a volunteer effort; Please feel free to close this PR as it may be some time before I have revised patches.

@jwludzik
Copy link
Contributor

Sure, no problem! I am glad for any help! In the meantime I created a similar change in #76, feel free to pick up it for testing

@xanderlent xanderlent marked this pull request as draft April 7, 2025 04:21
@xanderlent xanderlent changed the title Two minor build fixes Downstream patches from Fedora Apr 7, 2025
@xanderlent
Copy link
Author

@xanderlent, I've tried to build rpm packages based on your spec and without "Make firmware respect CMAKE_INSTALL_PREFIX" patch and it seems to work fine for me:

# rpm -ql /root/rpmbuild/RPMS/noarch/intel-npu-firmware-1.13.0-1.fc41.noarch.rpm
/lib/firmware/updates/intel
/lib/firmware/updates/intel/vpu
/lib/firmware/updates/intel/vpu/mtl_vpu_v0.0.bin
/lib/firmware/updates/intel/vpu/vpu_37xx_v0.0.bin
/lib/firmware/updates/intel/vpu/vpu_40xx_v0.0.bin
/usr/share/licenses/intel-npu-firmware
/usr/share/licenses/intel-npu-firmware/COPYRIGHT

The absolute path for firmware is intentional. In addition to what Józef said, in some distros /lib is not a symbolic link to /usr/lib, so it does make a difference whether you install the firmware to /lib/firmware or /usr/lib/firmware.

but it is also useful if you just want to install the driver to an arbitrary location to inspect the build outputs

For that cases, I think you can use DESTDIR variable.

Thanks, I'll look into dropping this patch in the next cycle. It might be a hold-over from when I was manually building the codebase.

@xanderlent
Copy link
Author

Sure enough, I was able to drop the change to the CMake install directories, since the RPM Macros use DESTDIR. Thanks for the suggestion.

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