8000 cmake: remove shader-gen step-targets from ggml-vulkan by bandoti · Pull Request #14226 · ggml-org/llama.cpp · GitHub
[go: up one dir, main page]
Skip to content

cmake: remove shader-gen step-targets from ggml-vulkan #14226

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 17, 2025
Merged

cmake: remove shader-gen step-targets from ggml-vulkan #14226

merged 2 commits into from
Jun 17, 2025

Conversation

bandoti
Copy link
Collaborator
@bandoti bandoti commented Jun 16, 2025 •
edited
Loading

The build/install steps of vulkan-shaders-gen are an implementation detail, but they are included as dependencies for ggml-vulkan. This removes such dependency so vulkan-shaders-gen doesn't get installed with the top-level "make install".

In addition it fixes an issue when the DESTDIR is specified for a Makefile generator when "make install" triggers the build step.

@bandoti bandoti requested a review from 0cc4m June 16, 2025 19:19
@bandoti bandoti linked an issue Jun 16, 2025 that may be closed by this pull request
Compile bug: vulkan-shaders-gen is installed into the build location instead of $PREFIX/bin/vulkan-shaders-gen #14190
Closed
@bandoti bandoti mentioned this pull request Jun 16, 2025
Closed
@github-actions github-actions bot added Vulkan Issues specific to the Vulkan backend ggml changes relating to the ggml tensor library for machine learning labels Jun 16, 2025
@yurivict
Copy link
Contributor

This section:

    ExternalProject_Add(
        vulkan-shaders-gen
        SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/vulkan-shaders
        CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${CMAKE_BINARY_DIR}/$<CONFIG>
                   -DCMAKE_INSTALL_BINDIR=.
                   -DCMAKE_BUILD_TYPE=$<CONFIG>
                   ${VULKAN_SHADER_GEN_CMAKE_ARGS}
    
        BUILD_COMMAND   ${CMAKE_COMMAND} --build   . --config $<CONFIG>
        INSTALL_COMMAND ${CMAKE_COMMAND} --install . --config $<CONFIG>
    )

contains the "INSTALL_COMMAND" instruction that seems to cause the failure during build.

It shouldn't be necessary to install the temporary toolchain command.

@yurivict
Copy link
Contributor

Build failure log:

-- Installing: /usr/ports/misc/llama-cpp/work/stage/usr/ports/misc/llama-cpp/work/.build/Release/./vulkan-shaders-gen
[ 25% 45/175] cd /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan && /usr/local/bin/cmake -E make_directory /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/CMakeFiles && /usr/local/bin/cmake -E touch /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/CMakeFiles/vulkan-shaders-gen-complete && /usr/local/bin/cmake -E touch /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix/src/vulkan-shaders-gen-stamp/vulkan-shaders-gen-done
[ 26% 46/175] cd /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan && /usr/ports/misc/llama-cpp/work/.build/Release/vulkan-shaders-gen --glslc /usr/local/bin/glslc --input-dir /usr/ports/misc/llama-cpp/work/llama.cpp-90ccac216290488aa79ae571b328d9aea6e6c584/ggml/src/ggml-vulkan/vulkan-shaders --output-dir /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/vulkan-shaders.spv --target-hpp /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/ggml-vulkan-shaders.hpp --target-cpp /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/ggml-vulkan-shaders.cpp --no-clean
FAILED: ggml/src/ggml-vulkan/ggml-vulkan-shaders.hpp ggml/src/ggml-vulkan/ggml-vulkan-shaders.cpp /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/ggml-vulkan-shaders.hpp /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/ggml-vulkan-shaders.cpp 
cd /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan && /usr/ports/misc/llama-cpp/work/.build/Release/vulkan-shaders-gen --glslc /usr/local/bin/glslc --input-dir /usr/ports/misc/llama-cpp/work/llama.cpp-90ccac216290488aa79ae571b328d9aea6e6c584/ggml/src/ggml-vulkan/vulkan-shaders --output-dir /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/vulkan-shaders.spv --target-hpp /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/ggml-vulkan-shaders.hpp --target-cpp /usr/ports/misc/llama-cpp/work/.build/ggml/src/ggml-vulkan/ggml-vulkan-shaders.cpp --no-clean
/bin/sh: /usr/ports/misc/llama-cpp/work/.build/Release/vulkan-shaders-gen: not found
ninja: build stopped: subcommand failed.

@bandoti
Copy link
Collaborator Author
bandoti commented Jun 17, 2025

Technically, external projects are part of the build process (not the install step) and supporting an install step should be perfectly valid—and separate from the top-level project install step.

Just to re-state and get to the core of the problem:

  1. ${CMAKE_BINARY_DIR}/$<CONFIG> becomes /usr/ports/misc/llama-cpp/work/stage/usr/ports/misc/llama-cpp/work/.build/Release/./
  2. set (_ggml_vk_genshaders_dir "${CMAKE_BINARY_DIR}/$<CONFIG>") is creating a different directory /usr/ports/misc/llama-cpp/work/.build/Release/.

So, the problem is not in the fact that ExternalProject_Add is using an install step, but that the same statement is evaluating to different paths. This is likely a problem with the generator expression being used in the set command, I'm thinking.

I need a way to reproduce this, though. Do you know how the stage directory is being set separately from the .build directory?

@yurivict
Copy link
Contributor

Do you know how the stage directory is being set separately from the .build directory?

The stage directory is set using the DESTDIR=/path/to/stage during "make install".
The stage directory is undefined during "make build".

@bandoti
Copy link
Collaborator Author
bandoti commented Jun 17, 2025

I ran the following and had no issues:

cmake -G "Unix Makefiles" -B build -DCMAKE_BUILD_TYPE=Release -DLLAMA_CURL=OFF -DGGML_VULKAN=ON
mkdir stage
cd build
make
DESTDIR="$(cd ..; pwd)/stage" make install

What I notice in your output, however, indicates that in fact the build step is using the staging directory:

-- Installing: /usr/ports/misc/llama-cpp/work/stage/usr/ports/misc/llama-cpp/work/.build/Release/./vulkan-shaders-gen

So I tried and skipped the separate "make" command, because the "make install" triggers the build, and was able to reproduce the issue:

cmake -G "Unix Makefiles" -B build -DCMAKE_BUILD_TYPE=Release -DLLAMA_CURL=OFF -DGGML_VULKAN=ON
mkdir stage
cd build
DESTDIR="$(cd ..; pwd)/stage" make install

So here's the real culprit: When DESTDIR is set for a "make install" that triggers the build step directly, vulkan-shaders-gen external project is picking up the same DESTDIR expected by the top-level install.

@bandoti
Copy link
Collaborator Author
bandoti commented Jun 17, 2025 •
edited
Loading

@yurivict I have fixed the issue on my side. If you would be so kind and test it on FreeBSD with the latest bandoti:cmake-vulkan-shaders-gen-install branch, that would be great. Thanks!

@yurivict
Copy link
Contributor

It builds successfully now.

@bandoti
Copy link
Collaborator Author
bandoti commented Jun 17, 2025

Awesome! We're good to go then. Thank you for catching this corner-case. 😊

@0cc4m whenever you give the approval we can merge this fix in. Thanks!

0cc4m
0cc4m approved these changes Jun 17, 2025
View reviewed changes
Copy link
Collaborator
@0cc4m 0cc4m left a comment

Choose a reason for hiding this comment

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

Thank you for fixing it.

@0cc4m 0cc4m merged commit c465030 into ggml-org:master Jun 17, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0cc4m 0cc4m 0cc4m approved these changes

Assignees
No one assigned
Labels
ggml changes relating to the ggml tensor library for machine learning Vulkan Issues specific to the Vulkan backend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Compile bug: vulkan-shaders-gen is installed into the build location instead of $PREFIX/bin/vulkan-shaders-gen
3 participants
@bandoti @yurivict @0cc4m
0