build(deps): automatically pull in dependencies #5982
Conversation
To ease development, this patch pulls in dependencies automatically using FetchContent[^1]. Test dependencies are only pulled in if tests are enabled. [^1]: https://cmake.org/cmake/help/latest/module/FetchContent.html
|
|
There was a problem hiding this comment.
I don't think we want to go in a direction that will breaking existing users. We want to allow other project that use SPIRV-Tools to pick their own version of SPIRV-Headers.
I don't know how the fetch works, but I'm pretty sure this would break the choromium builds because they want to pick their version for each of our dependencies.
| fetch_content( | ||
| SPIRV-Headers | ||
| GIT_REPOSITORY https://github.com/KhronosGroup/SPIRV-Headers.git | ||
| GIT_TAG origin/main | ||
| OVERRIDE_FIND_PACKAGE | ||
| ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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() | ||
|
|
There was a problem hiding this comment.
You cannot remove this. We cannot change the way it works for existing users. This would break many projects.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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.
Can you elaborate? Do they provide a different configuration file for pulling the dependencies? |
You can try building shaderc with your change in third_party. I get this error: SPIRV-Headers will already be defined by shaderc. |
To ease development, this patch pulls in dependencies automatically using
FetchContent1. Test dependencies are only pulled in if tests are enabled.
There' still a few open questions, I opened this PR to see if the direction
I'm going with is good.
Open questions:
The path will now be "dynamic" as it depends what is passed to
cmake -B ${BUILD_FOLDER}. This means that scripts liketools/sva/tools/process_grammar.rbwhich has a hard coded pathwill need to account for this. My suggestion is for example to cache
the SPIRV headers directory and have the script run something like
cmake -LA -N | grep SPIRV_HEADERS_DIR.I'm not familiar with bazel, does the build config there need to be
adjusted to reflect these changes as well?
Footnotes
https://cmake.org/cmake/help/latest/module/FetchContent.html ↩