Skip to content

[CI] Temporarily disable tests requiring spirv-tools in CI #16743

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
Jan 23, 2025

Conversation

uditagarwal97
Copy link
Contributor

@uditagarwal97 uditagarwal97 commented Jan 23, 2025

#16724 installed spirv-tools on Linux docker containers and now some llvm-spirv tests are failing due to this (example: https://github.com/intel/llvm/actions/runs/12915358763/job/36017038536).
In this PR, we disable detection of spirv-tools LIT feature temporarily in CI to fix post-commit.

Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock fixing CI failures, but llvm-spirv changes should either be upstreamed, or tracked in #7592 to remind us to revert them

@maarquitos14
Copy link
Contributor

I'm okay with merging this to fix post-commit.

About the root cause, the problem seems to be that the path for all spirv-tools binaries is not found:

llvm-lit: /__w/llvm/llvm/src/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find spirv-as in SPIRV_AS_PATH-NOTFOUND
llvm-lit: /__w/llvm/llvm/src/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find spirv-dis in SPIRV_AS_PATH-NOTFOUND
llvm-lit: /__w/llvm/llvm/src/llvm/utils/lit/lit/llvm/subst.py:[12](https://github.com/intel/llvm/actions/runs/12918158681/job/36026109985#step:14:13)6: note: Did not find spirv-link in SPIRV_AS_PATH-NOTFOUND
llvm-lit: /__w/llvm/llvm/src/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find spirv-val in SPIRV_AS_PATH-NOTFOUND

I'm not sure why, I cannot reproduce locally. If you help me reproduce the issue, I can try and fix it.

Co-authored-by: Marcos Maronas <[email protected]>
@uditagarwal97 uditagarwal97 changed the title [CI] Temporarily disable tests requiring spriv-tools in CI [CI] Temporarily disable tests requiring spirv-tools in CI Jan 23, 2025
@uditagarwal97
Copy link
Contributor Author

@intel/llvm-gatekeepers I think we are good to merge this.

@sarnex sarnex merged commit f12546b into sycl Jan 23, 2025
22 of 30 checks passed
@aelovikov-intel aelovikov-intel deleted the sycl-devops-pr/udit/llvm_spirv branch January 23, 2025 18:52
@uditagarwal97
Copy link
Contributor Author

uditagarwal97 commented Jan 25, 2025

Hi @maarquitos14, @intel/dpcpp-spirv-reviewers,
I think I know what caused postcommit failures after installing spirv-tools on CI:

So, in llvm-spirv, we detect spirv-tools using two methods: using pkg-config (main) or cmake (fallback)(https://github.com/intel/llvm/blob/sycl/llvm-spirv/CMakeLists.txt#L123)

  1. Using pkg-config:
    Our CI Linux container does not have pkg-config installed, unlike internally or in Podman's Ubuntu24 image.
    So, if I just install pkg-config in the docker container, we are able to correctly find spirv-tools and llvm-spirv LIT tests passes correctly.

  2. Using cmake:
    Since we did not have pkg-config installed, llvm-spirv was using cmake to find spirv-tools. However, I think there's a bug in spiv-tools's official Ubuntu 24 installation package that we obtained via:

. /etc/os-release
curl -L "https://packages.lunarg.com/lunarg-signing-key-pub.asc" | apt-key add -
echo "deb https://packages.lunarg.com/vulkan $VERSION_CODENAME main" | tee -a /etc/apt/sources.list
apt update && apt install -yqq spirv-tools

The installation package does not have the correct CMake target import file. Here's the import files apt install spirv-tools installed:

$:/usr/lib/x86_64-linux-gnu/cmake/SPIRV-Tools-tools# ls
SPIRV-Tools-toolsConfig.cmake   SPIRV-Tools-toolsTargets.cmake  SPIRV-Tools-toolsTargets-none.cmake

Notice the SPIRV-Tools-toolsTargets-none.cmake file - the none here refers to the build config. So, if we do get_target_property(SPIRV_AS_PATH spirv-as IMPORTED_LOCATION_RELEASE) or get_target_property(SPIRV_AS_PATH spirv-as IMPORTED_LOCATION_DEBUG) (that we use to get spirv-as path: https://github.com/intel/llvm/blob/sycl/llvm-spirv/test/CMakeLists.txt#L56), it will not load this file because none is an invalid build config for LLVM project. If I rename this file, SPIRV-Tools-toolsTargets-none.cmake --> SPIRV-Tools-toolsTargets-debug.cmake and manually replace NONE to DEBUG in build config, cmake correctly loads the file and all llvm-spriv LIT tests correct passes. Btw, if we build spirv-tools locally, it produces the SPIRV-Tools-toolsTargets-debug.cmake or SPIRV-Tools-toolsTargets-release.cmake file, depending on build config.
So, that's why I think there's a bug in spiv-tools official installation package.

Solution:
For the time being, I think we can just install pkg-config in the CI container - we also use pkg-config internally, plus, it will take some time to confirm and get the cmake bug fixed, which, at best, will happen in the next spirv-tools release.

PS:- I've installed spirv-tools on Windows CI machines and it is working fine: https://github.com/intel/llvm/actions/runs/12934269927/job/36192622174#step:14:1

@maarquitos14
Copy link
Contributor

@uditagarwal97 Thanks for the thorough investigation! I'm good with the suggested solution, let me know if you need any help.

sarnex pushed a commit that referenced this pull request Jan 27, 2025
`pkg-config` is required for `llvm-spirv` to detect `spirv-tools`
installation.
See #16743 (comment) for
more info.
uditagarwal97 added a commit that referenced this pull request Jan 27, 2025
sarnex pushed a commit that referenced this pull request Jan 28, 2025
…#16801)

Reverts #16743. After installing `pkg-config` in CI,
`llvm-spirv` can correctly find `spriv-tools` so this workaround is no
longer needed.
Example run:
https://github.com/intel/llvm/actions/runs/12996166493/job/36244369469?pr=16801#step:14:16
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.

5 participants