-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add shader-compiler option and versions 1.0.6, 1.0.7 and 1.0.8 #18996
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 6a681b9vsg/1.0.5
vsg/1.0.0
vsg/1.0.3
vsg/1.0.8
vsg/1.0.6
vsg/1.0.7
|
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 964bdb1vsg/1.0.5
vsg/1.0.0
vsg/1.0.3
vsg/1.0.7
vsg/1.0.8
vsg/1.0.6
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit e0a7992vsg/1.0.3
vsg/1.0.0
vsg/1.0.5
vsg/1.0.7
vsg/1.0.8
vsg/1.0.6
|
removed 1.0.6/1.0.7 version
This comment has been minimized.
This comment has been minimized.
removed versions
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution. I'm concerned about the shader_compiler
option, as it will install another project by git command.
package_type = "library" | ||
settings = "os", "arch", "compiler", "build_type" | ||
options = { | ||
"shared": [True, False], | ||
"shader_compiler": [True, False], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove the option shader_compiler. It will execute an extra step that will run git clone to another project:
if (VSG_SUPPORTS_ShaderCompiler)
if (NOT EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/src/glslang/build_vars.cmake)
if (Git_FOUND)
set(glslang_URL "https://github.com/vsg-dev/glslang.git" CACHE STRING "URL of the glslang git repository")
set(glslang_branch "VSG-1.0.x" CACHE STRING "branch/tag of the glslang git repository")
execute_process(COMMAND ${GIT_EXECUTABLE} clone --depth 1 --branch ${glslang_branch} ${glslang_URL}
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/src
RESULT_VARIABLE GIT_SUBMOD_RESULT)
if(NOT GIT_SUBMOD_RESULT EQUAL "0")
message(WARNING "git clone of glslang failed. ShaderCompile support disabled.")
set(VSG_SUPPORTS_ShaderCompiler 0)
endif()
else()
message(WARNING "git clone of glslang failed. ShaderCompile support disabled.")
set(VSG_SUPPORTS_ShaderCompiler 0)
endif()
endif()
endif()
In case needing an external dependency, it should be converted to a Conan package first, so we can manage it in the dependency, otherwise, we will have no control over any version, settings or options change.
For this very specific case, glslang
is already available in Conan Center, so you could use it, but it would require a patch to use it as external dependency. Thinking about maintenance, it would be even better adding a feature directly to VSG project, giving the option to declared glslang
as configurable, as a separated project, like USE_GLSLANG_SYSTEM
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the shader_compiler the VSG is not usable to its full extent. The original author integrates glslang here by adding directly into the source graph of VSG and uses headers that are not exposed by the glslang conan-package. I've tried really hard to integrate a custom glslang-package, but this required massive changes in the source-structure which are not maintainable on the long-run. As glslang-sources are inlined with VSG and the author likes to control which exact versions to be bundled with VSG I don't really get the argument about control over version.
I also tried to convince the author not to use git-submodules or direct checkouts but he arguments that he wants to have VSG as self-contained as possible. I can start another attempt to create a custom vsg-glslang package, but honestly I don't think I will have the time in the long run to maintain changes here by creating patches. Still, I'll try.
Sorry for inconveniences, but I thought checking it out into the own source-graph should be ok, as it is not visible from the outside, thus not interfering downstream with other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I revisited the issue and found the culprit being a wrong assumption about the glslang version used. I was able to tackle the issue by adding a new version to glslang. So basically this PR has to wait for the glslang-PR completed. (link TBD)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
removing non-supported versions for now
Conan v1 pipeline ❌Failure in build 19 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
vsg/1.0.6-1.0.8
Depends on #19980
updated versions and re-enabled shader-compiler option starting with 1.0.5