fix(build): workaround qt_add_shaders bug with '@' in build path#864
fix(build): workaround qt_add_shaders bug with '@' in build path#864glyvut wants to merge 1 commit into
Conversation
Replace qt_add_shaders with treeland_add_shaders using add_custom_command to invoke Qt6::qsb directly, avoiding the '@' splitting bug in Qt6ShaderToolsMacros.cmake. 替换 qt_add_shaders 为自定义的 treeland_add_shaders，通过 add_custom_command 直接调用 Qt6::qsb，绕过 Qt6ShaderToolsMacros.cmake 中 '@' 字符被错误拆分的 bug。 Log: 修复构建路径含 '@' 时着色器编译失败的问题 Issue: Fixes linuxdeepin#845 Influence: 构建目录路径包含 '@' 字符（如 /tmp/u@123）时不再编译失败。
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: glyvut The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @glyvut. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
CLA Assistant Lite bot: |
Reviewer's GuideReplaces usage of qt_add_shaders with a custom treeland_add_shaders CMake helper that calls Qt6::qsb via add_custom_command to avoid a Qt shader tools bug when build paths contain '@', and wires the generated .qsb files into qt_add_resources for libtreeland. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The custom qsb output directory
${CMAKE_CURRENT_BINARY_DIR}/.qsbis assumed to exist but never created; consider adding afile(MAKE_DIRECTORY ...)or a smalladd_custom_commandwithcmake -E make_directoryto avoid build failures on a clean tree. - The
PRECOMPILEargument is parsed but not used intreeland_add_shaders; either implement equivalent behavior to the originalqt_add_shadersflag or drop the argument to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The custom qsb output directory `${CMAKE_CURRENT_BINARY_DIR}/.qsb` is assumed to exist but never created; consider adding a `file(MAKE_DIRECTORY ...)` or a small `add_custom_command` with `cmake -E make_directory` to avoid build failures on a clean tree.
- The `PRECOMPILE` argument is parsed but not used in `treeland_add_shaders`; either implement equivalent behavior to the original `qt_add_shaders` flag or drop the argument to avoid confusion.
## Individual Comments
### Comment 1
<location path="cmake/DefineTarget.cmake" line_range="68-69" />
<code_context>
+
+ set(qsb_outputs "")
+ foreach(shader_file IN LISTS arg_FILES)
+ get_filename_component(shader_name "${shader_file}" NAME)
+ set(qsb_file "${CMAKE_CURRENT_BINARY_DIR}/.qsb/${shader_name}.qsb")
+ list(APPEND qsb_outputs "${qsb_file}")
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Using only the file name for qsb output paths can cause collisions for shaders with the same basename in different directories.
Since `shader_name` uses `NAME`, inputs like `foo/lighting.glsl` and `bar/lighting.glsl` both produce `.qsb/lighting.glsl.qsb`, causing overwrites and incorrect dependencies. Use a scheme that guarantees uniqueness, e.g. include part of the directory in the name or hash the full input path when generating the output filename.
</issue_to_address>
### Comment 2
<location path="cmake/DefineTarget.cmake" line_range="69-72" />
<code_context>
+ set(qsb_outputs "")
+ foreach(shader_file IN LISTS arg_FILES)
+ get_filename_component(shader_name "${shader_file}" NAME)
+ set(qsb_file "${CMAKE_CURRENT_BINARY_DIR}/.qsb/${shader_name}.qsb")
+ list(APPEND qsb_outputs "${qsb_file}")
+
+ add_custom_command(
+ OUTPUT "${qsb_file}"
+ COMMAND Qt6::qsb --qt6 $<$<BOOL:${arg_BATCHABLE}>:--batchable> "${shader_file}" -o "${qsb_file}"
</code_context>
<issue_to_address>
**issue (bug_risk):** The `.qsb` output directory may not exist when the custom command runs, causing qsb to fail.
CMake does not create `${CMAKE_CURRENT_BINARY_DIR}/.qsb` automatically, so the first `Qt6::qsb` invocation can fail with "No such file or directory". Add a `${CMAKE_COMMAND} -E make_directory "${CMAKE_CURRENT_BINARY_DIR}/.qsb"` command before calling qsb, or create the directory once outside the loop with `file(MAKE_DIRECTORY ...)`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| get_filename_component(shader_name "${shader_file}" NAME) | ||
| set(qsb_file "${CMAKE_CURRENT_BINARY_DIR}/.qsb/${shader_name}.qsb") |
There was a problem hiding this comment.
issue (bug_risk): Using only the file name for qsb output paths can cause collisions for shaders with the same basename in different directories.
Since shader_name uses NAME, inputs like foo/lighting.glsl and bar/lighting.glsl both produce .qsb/lighting.glsl.qsb, causing overwrites and incorrect dependencies. Use a scheme that guarantees uniqueness, e.g. include part of the directory in the name or hash the full input path when generating the output filename.
| set(qsb_file "${CMAKE_CURRENT_BINARY_DIR}/.qsb/${shader_name}.qsb") | ||
| list(APPEND qsb_outputs "${qsb_file}") | ||
|
|
||
| add_custom_command( |
There was a problem hiding this comment.
issue (bug_risk): The .qsb output directory may not exist when the custom command runs, causing qsb to fail.
CMake does not create ${CMAKE_CURRENT_BINARY_DIR}/.qsb automatically, so the first Qt6::qsb invocation can fail with "No such file or directory". Add a ${CMAKE_COMMAND} -E make_directory "${CMAKE_CURRENT_BINARY_DIR}/.qsb" command before calling qsb, or create the directory once outside the loop with file(MAKE_DIRECTORY ...).
2 participants
Replace qt_add_shaders with treeland_add_shaders using add_custom_command to invoke Qt6::qsb directly, avoiding the '@' splitting bug in Qt6ShaderToolsMacros.cmake.
替换 qt_add_shaders 为自定义的 treeland_add_shaders，通过 add_custom_command 直接调用 Qt6::qsb，绕过 Qt6ShaderToolsMacros.cmake 中 '@' 字符被错误拆分的 bug。
Log: 修复构建路径含 '@' 时着色器编译失败的问题
Issue: Fixes #845
Influence: 构建目录路径包含 '@' 字符（如 /tmp/u@123）时不再编译失败。
Summary by Sourcery
Work around a Qt shader compilation bug by replacing direct use of qt_add_shaders with a custom helper that invokes qsb explicitly and packages the generated shader binaries as resources.
Bug Fixes:
Enhancements: