Skip to content

build(deps): automatically pull in dependencies #5982

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

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

build(deps): automatically pull in dependencies #5982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,30 @@ cmake_minimum_required(VERSION 3.22.1)

project(spirv-tools)

include(FetchContent)
function(fetch_content content_name)
message(STATUS "Fetching ${content_name}...")

list(REMOVE_AT ARGV 0) # remove content_name

FetchContent_Declare(
${content_name}
${ARGV}
)

FetchContent_MakeAvailable(${content_name})
endfunction()

# TODO: these should probably be locked to a specific commit?
fetch_content(
SPIRV-Headers
GIT_REPOSITORY https://github.com/KhronosGroup/SPIRV-Headers.git
GIT_TAG origin/main
OVERRIDE_FIND_PACKAGE
)
Comment on lines +34 to +39
Copy link
Copy Markdown
Collaborator

@s-perron s-perron Feb 3, 2025

Choose a reason for hiding this comment

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

We should not unconditionally fetching the dependency. This should only happen if the directory does not already exist.

This will also create problem if SPIRV Headers get ahead of SPIR-V tools. This may not always compile. We use the DEPS file to keep track of a currently working version. We try to keep that as up-to-date as possible. You should try to get the commit hash from there.

With that said, I'm not sure if this is too useful since all people have to do to get the deps is call python3 utils/git-sync-deps. See https://github.com/KhronosGroup/SPIRV-Tools?tab=readme-ov-file#getting-the-source. This creates a second way of doing the samething.

Copy link
Copy Markdown
Author

@vikke1234 vikke1234 Feb 3, 2025

Choose a reason for hiding this comment

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

We should not unconditionally fetching the dependency. This should only happen if the directory does not already exist.

It does not do that.

This will also create problem if SPIRV Headers get ahead of SPIR-V tools. This may not always compile. We use the DEPS file to keep track of a currently working version. We try to keep that as up-to-date as possible. You should try to get the commit hash from there.

There's a TODO to for me to lock it into the correct commit. I wasn't sure which one was the correct one. Just wanted to get initial reviews. The commit IDs can be set by the GIT_TAG argument.

With that said, I'm not sure if this is too useful since all people have to do to get the deps is call python3 utils/git-sync-deps. See https://github.com/KhronosGroup/SPIRV-Tools?tab=readme-ov-file#getting-the-source. This creates a second way of doing the samething.

It automates the process though to make the build more comfortable.

Copy link
Copy Markdown
Collaborator

@s-perron s-perron Feb 5, 2025

Choose a reason for hiding this comment

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

It does not do that.

Sorry, I don't know cmake too well. I guess it only fetches the content if the package could not be found.

There's a TODO to for me to lock it into the correct commit.

I saw the TODO. I just want to make sure that you get the commit from the DEPS file. We do not want to keep track of the proper commit in multiple places.

Now I see why the fetching is use, if you are relying on find_package.

get_target_property(SPIRV_HEADER_INCLUDE_DIR SPIRV-Headers INTERFACE_INCLUDE_DIRECTORIES)


# Avoid a bug in CMake 3.22.1. By default it will set -std=c++11 for
# targets in test/*, when those tests need -std=c++17.
# https://github.com/KhronosGroup/SPIRV-Tools/issues/5340
Expand Down Expand Up @@ -239,6 +263,7 @@ endif()

# Tests require Python3
find_host_package(Python3 REQUIRED)
find_package(SPIRV-Headers CONFIG REQUIRED)

# Check for symbol exports on Linux.
# At the moment, this check will fail on the OSX build machines for the Android NDK.
Expand Down Expand Up @@ -317,7 +342,6 @@ macro(spvtools_pch SRCS PCHPREFIX)
endif()
endmacro(spvtools_pch)

add_subdirectory(external)

# Warning about extra semi-colons.
#
Expand Down Expand Up @@ -386,8 +410,8 @@ add_custom_command(
-DSPIRV_SHARED_LIBRARIES=${SPIRV_SHARED_LIBRARIES}
-P ${CMAKE_CURRENT_SOURCE_DIR}/cmake/write_pkg_config.cmake
DEPENDS "CHANGES" "${CMAKE_CURRENT_SOURCE_DIR}/cmake/SPIRV-Tools-shared.pc.in" "${CMAKE_CURRENT_SOURCE_DIR}/cmake/write_pkg_config.cmake")
add_custom_target(spirv-tools-pkg-config
ALL
add_custom_target(spirv-tools-pkg-config
ALL
DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/SPIRV-Tools-shared.pc ${CMAKE_CURRENT_BINARY_DIR}/SPIRV-Tools.pc)

# Install pkg-config file
Expand Down
52 changes: 0 additions & 52 deletions external/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,61 +33,9 @@ else()
set(SPIRV_HEADER_DIR ${CMAKE_CURRENT_SOURCE_DIR}/spirv-headers)
endif()

if (IS_DIRECTORY ${SPIRV_HEADER_DIR})
# TODO(dneto): We should not be modifying the parent scope.
set(SPIRV_HEADER_INCLUDE_DIR ${SPIRV_HEADER_DIR}/include PARENT_SCOPE)

# Add SPIRV-Headers as a sub-project if it isn't already defined.
# Do this so enclosing projects can use SPIRV-Headers_SOURCE_DIR to find
# headers to include.
if (NOT DEFINED SPIRV-Headers_SOURCE_DIR)
add_subdirectory(${SPIRV_HEADER_DIR})
endif()
else()
message(FATAL_ERROR
"SPIRV-Headers was not found - please checkout a copy at external/spirv-headers.")
endif()

Comment on lines -36 to -50
Copy link
Copy Markdown
Collaborator

@s-perron s-perron Feb 3, 2025

Choose a reason for hiding this comment

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

You cannot remove this. We cannot change the way it works for existing users. This would break many projects.

Copy link
Copy Markdown
Author

@vikke1234 vikke1234 Feb 3, 2025

Choose a reason for hiding this comment

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

You cannot remove this. We cannot change the way it works for existing users. This would break many projects.

I see, so the dependencies would need to be cloned to the same places?

Copy link
Copy Markdown
Collaborator

@s-perron s-perron Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Consider how spirv-tools is used in https://github.com/google/shaderc. This repo has two "submodule" that use spirv-headers: glslang, spirv-tools. Shaderc does not want to compile with two different versions of spriv-headers for these tools, so it will checkout its own version, and tell glslang and spirv-tools where to find it:

https://github.com/google/shaderc/blob/ab0d3ed184e4919a236eecddb6514e4923a5e38f/third_party/CMakeLists.txt

In the SPIR-V tools cmake files, we have no way of knowing which version the parent project wants to use nor do we know where they have put it. A few other tools are the same way.

If you want to use find_package you cannot change the behaviour of existing working project. You might want to replace the fatal error with the call to find_package, with an error if that fails for some reason. Do not change the rest of the logic.

if (NOT ${SPIRV_SKIP_TESTS})
# Find gmock if we can. If it's not already configured, then try finding
# it in external/googletest.
if (TARGET gmock)
message(STATUS "Google Mock already configured")
else()
if (NOT GMOCK_DIR)
set(GMOCK_DIR ${CMAKE_CURRENT_SOURCE_DIR}/googletest)
endif()
if(EXISTS ${GMOCK_DIR})
if(MSVC)
# Our tests use ::testing::Combine. Work around a compiler
# detection problem in googletest, where that template is
# accidentally disabled for VS 2017.
# See https://github.com/google/googletest/issues/1352
add_definitions(-DGTEST_HAS_COMBINE=1)
endif()
if(WIN32)
option(gtest_force_shared_crt
"Use shared (DLL) run-time lib even when Google Test is built as static lib."
ON)
endif()
# gtest requires special defines for building as a shared
# library, simply always build as static.
push_variable(BUILD_SHARED_LIBS 0)
add_subdirectory(${GMOCK_DIR} ${CMAKE_CURRENT_BINARY_DIR}/googletest EXCLUDE_FROM_ALL)
pop_variable(BUILD_SHARED_LIBS)
endif()
endif()
if (TARGET gmock)
set(GTEST_TARGETS
gtest
gtest_main
gmock
gmock_main
)
foreach(target ${GTEST_TARGETS})
set_property(TARGET ${target} PROPERTY FOLDER GoogleTest)
endforeach()
endif()

# Find Effcee and RE2, for testing.

Expand Down
4 changes: 1 addition & 3 deletions source/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,10 @@ macro(spvtools_opencl_tables CONFIG_VERSION)
list(APPEND EXTINST_CPP_DEPENDS ${GRAMMAR_INC_FILE})
endmacro(spvtools_opencl_tables)

message("HEADER DIR: ${SPIRV_HEADER_INCLUDE_DIR}")
macro(spvtools_vendor_tables VENDOR_TABLE SHORT_NAME OPERAND_KIND_PREFIX)
set(INSTS_FILE "${spirv-tools_BINARY_DIR}/${VENDOR_TABLE}.insts.inc")
set(GRAMMAR_FILE "${SPIRV_HEADER_INCLUDE_DIR}/spirv/unified1/extinst.${VENDOR_TABLE}.grammar.json")
if(NOT EXISTS ${GRAMMAR_FILE})
set(GRAMMAR_FILE "${spirv-tools_SOURCE_DIR}/source/extinst.${VENDOR_TABLE}.grammar.json")
endif()
add_custom_command(OUTPUT ${INSTS_FILE}
COMMAND Python3::Interpreter ${GRAMMAR_PROCESSING_SCRIPT}
--extinst-vendor-grammar=${GRAMMAR_FILE}
Expand Down
56 changes: 47 additions & 9 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,49 @@
if (${SPIRV_SKIP_TESTS})
return()
endif()
set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
fetch_content(
googletest
GIT_REPOSITORY https://github.com/google/googletest.git
GIT_TAG origin/main
FIND_PACKAGE_ARGS NAMES GTest
)
find_package(GTest CONFIG REQUIRED)

fetch_content(
protobuf
GIT_REPOSITORY https://github.com/protocolbuffers/protobuf.git
GIT_TAG origin/main
OVERRIDE_FIND_PACKAGE
)

set(ABSL_ENABLE_INSTALL ON)
fetch_content(
absl
GIT_REPOSITORY https://github.com/abseil/abseil-cpp.git
GIT_TAG origin/master
OVERRIDE_FIND_PACKAGE
)

fetch_content(
re2
GIT_REPOSITORY https://github.com/google/re2.git
GIT_TAG origin/main
OVERRIDE_FIND_PACKAGE
)

set(EFFCEE_BUILD_TESTING OFF)
fetch_content(
effcee
GIT_REPOSITORY https://github.com/google/effcee.git
GIT_TAG origin/main
OVERRIDE_FIND_PACKAGE
)

find_package(re2 CONFIG REQUIRED)
find_package(effcee CONFIG REQUIRED)
find_package(protobuf CONFIG REQUIRED)

if (TARGET gmock_main)
message(STATUS "Found Google Mock, building tests.")
else()
message(STATUS "Did not find googletest, tests will not be built. "
"To enable tests place googletest in '<spirv-dir>/external/googletest'.")
endif()

# Add a SPIR-V Tools unit test. Signature:
# add_spvtools_unittest(
Expand Down Expand Up @@ -60,14 +96,16 @@ function(add_spvtools_unittest)
target_compile_options(${target} PRIVATE /DGTEST_HAS_COMBINE=1)
endif()
target_include_directories(${target} PRIVATE
${SPIRV_HEADER_INCLUDE_DIR}
${spirv-tools_SOURCE_DIR}
${spirv-tools_SOURCE_DIR}/include
${spirv-tools_SOURCE_DIR}/test
${spirv-tools_BINARY_DIR}
${gtest_SOURCE_DIR}/include
${gmock_SOURCE_DIR}/include
)
target_link_libraries(${target} PRIVATE
SPIRV-Headers
googletest
)

if (TARGET effcee)
# If using Effcee for testing, then add its include directory.
target_include_directories(${target} PRIVATE ${effcee_SOURCE_DIR})
Expand Down