Skip to content

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 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