Skip to content

do not install libze_loader #1285

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

Merged
merged 3 commits into from
May 8, 2025

Conversation

bratpiorka
Copy link
Contributor

@bratpiorka bratpiorka commented Apr 23, 2025

Never build and install ze_loader by using FetchContent_Populate instead of MakeAvailable. After this change, we have to find ze_loader installed on the system instead of using the one built by us.
Additionally, add testing cmake 3.14, which is the oldest supported version by UMF.

Example of failed installation/deinstallation test (requires using CMake 3.14.0):
https://github.com/oneapi-src/unified-memory-framework/actions/runs/14692346477/job/41229764121?pr=1285

fixes: #1110

@bratpiorka bratpiorka force-pushed the rrudnick_fix_ze_install branch 16 times, most recently from 44016f2 to 94ae262 Compare April 30, 2025 12:25
@bratpiorka bratpiorka force-pushed the rrudnick_fix_ze_install branch 14 times, most recently from 749a2a4 to fcf3a5b Compare May 5, 2025 17:17
@bratpiorka bratpiorka force-pushed the rrudnick_fix_ze_install branch from c205207 to 0f54c89 Compare May 6, 2025 09:35
@bratpiorka bratpiorka force-pushed the rrudnick_fix_ze_install branch 3 times, most recently from 56257b9 to 8c3972b Compare May 6, 2025 10:32
@bratpiorka bratpiorka force-pushed the rrudnick_fix_ze_install branch from 8c3972b to e65cf01 Compare May 6, 2025 13:25
@bratpiorka bratpiorka requested a review from Copilot May 7, 2025 06:43
Copy link

@Copilot 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 removes the custom installation of ze_loader by no longer using FetchContent_Populate, and updates the installation command for UMF as well as the GitHub Actions workflow to test with CMake 3.14, the oldest supported version by UMF.

  • Updated UMF installation command from "cmake --install" to "cmake --build ... --target install"
  • Modified GitHub Actions workflow to conditionally install a minimum CMake version for specific test matrices

Reviewed Changes

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

File Description
test/test_installation.py Changes the UMF installation command and adds a debug print for the command
.github/workflows/reusable_basic.yml Updates matrix configurations and adds a step to install CMake when required
Files not reviewed (9)
  • CMakeLists.txt: Language not supported
  • benchmark/CMakeLists.txt: Language not supported
  • cmake/FindZE_LOADER.cmake: Language not supported
  • examples/CMakeLists.txt: Language not supported
  • examples/cmake/FindZE_LOADER.cmake: Language not supported
  • examples/cuda_shared_memory/CMakeLists.txt: Language not supported
  • examples/ipc_level_zero/CMakeLists.txt: Language not supported
  • examples/level_zero_shared_memory/CMakeLists.txt: Language not supported
  • test/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (2)

test/test_installation.py:184

  • Removing the '--prefix' argument from the install command may lead to an installation in an unintended directory if the build configuration does not already specify the correct CMAKE_INSTALL_PREFIX. Consider confirming that the build configuration predefines the install directory or reintroducing the '--prefix {self.install_dir}' parameter.
install_cmd = f"cmake --build {self.build_dir} --config {self.build_type.title()} --target install"

.github/workflows/reusable_basic.yml:138

  • Removing 'cmake' from the apt-get install command may result in systems with an outdated default CMake when matrix.cmake_ver is set to 'latest'. Ensure that jobs using 'latest' have an appropriate CMake version available or add an additional update step.
sudo apt-get install -y clang libnuma-dev lcov

@bratpiorka bratpiorka force-pushed the rrudnick_fix_ze_install branch 2 times, most recently from 97f97f5 to 5f4b6b6 Compare May 7, 2025 08:47
@bratpiorka bratpiorka force-pushed the rrudnick_fix_ze_install branch from 5f4b6b6 to a0eb6cb Compare May 7, 2025 12:06
@bratpiorka bratpiorka merged commit 0c09d06 into oneapi-src:main May 8, 2025
251 of 254 checks passed
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.

never build or install libze_loader
4 participants