Skip to content

Conversation

@wineee
Copy link
Member

@wineee wineee commented Aug 21, 2025

  1. Replace direct reference to "treeland-ddm-v1.c" in DAEMON_SOURCES with variable ${TREELAND_DDM_SOURCE} to ensure correct path resolution
  2. Restructure protocol file generation using set() and add_custom_command() for better maintainability and clarity
  3. Add DEPENDS and COMMENT fields to improve build system traceability and debugging
  4. Ensure generated source files are correctly referenced during compilation

fix: 解决 treeland-ddm-v1.c 源文件找不到的问题

  1. 将 DAEMON_SOURCES 中对 "treeland-ddm-v1.c" 的硬编码引用替换为变量 ${TREELAND_DDM_SOURCE},确保路径正确解析
  2. 使用 set() 和 add_custom_command() 重构协议文件生成流程,提高可维护性 和可读性
  3. 添加 DEPENDS 和 COMMENT 字段以增强构建系统的可追踪性和调试能力
  4. 确保在编译时正确引用生成的源文件

Summary by Sourcery

Update daemon CMake build to properly generate and include the treeland-ddm-v1 protocol sources

Bug Fixes:

  • Fix missing treeland-ddm-v1.c source file by referencing the generated source via variable

Enhancements:

  • Introduce TREELAND_DDM_HEADER and TREELAND_DDM_SOURCE variables and split protocol file generation into separate add_custom_command invocations

Build:

  • Add DEPENDS and COMMENT fields to protocol file generation commands
  • Replace hardcoded treeland-ddm-v1.c in DAEMON_SOURCES with ${TREELAND_DDM_SOURCE}

1. Replace direct reference to "treeland-ddm-v1.c" in DAEMON_SOURCES
with variable ${TREELAND_DDM_SOURCE} to ensure correct path resolution
2. Restructure protocol file generation using set() and
add_custom_command() for better maintainability and clarity
3. Add DEPENDS and COMMENT fields to improve build system traceability
and debugging
4. Ensure generated source files are correctly referenced during
compilation

fix: 解决 treeland-ddm-v1.c 源文件找不到的问题

1. 将 DAEMON_SOURCES 中对 "treeland-ddm-v1.c" 的硬编码引用替换为变量
${TREELAND_DDM_SOURCE},确保路径正确解析
2. 使用 set() 和 add_custom_command() 重构协议文件生成流程,提高可维护性
和可读性
3. 添加 DEPENDS 和 COMMENT 字段以增强构建系统的可追踪性和调试能力
4. 确保在编译时正确引用生成的源文件
@wineee
Copy link
Member Author

wineee commented Aug 21, 2025

https://build.deepin.com/build/vioken/deepin_develop/aarch64/ddm/_log

@apr3vau @zccrs

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码是CMake构建系统的一部分,用于生成Wayland协议相关的文件。我来对这段代码进行分析和评价:

语法逻辑

代码语法正确,逻辑清晰。原来的add_custom_target被替换为两个add_custom_command,这是CMake中更推荐的做法,因为这样可以更好地处理依赖关系。

代码质量

  1. 改进后的代码质量有明显提升:

    • 使用了明确的变量名(TREELAND_DDM_HEADERTREELAND_DDM_SOURCE)替代硬编码的路径
    • 添加了COMMENT参数,使构建过程更透明
    • 明确指定了DEPENDS关系,确保在协议文件更改时重新生成
  2. 唯一的小建议是,可以考虑将这两个add_custom_command包装在一个add_custom_target中,以便作为一个整体目标来管理:

add_custom_target(treeland-ddm ALL
    DEPENDS ${TREELAND_DDM_HEADER} ${TREELAND_DDM_SOURCE}
)

代码性能

  1. 性能方面没有明显问题,但有以下建议:
    • 考虑将这两个命令合并为一个,避免两次扫描同一个XML文件
    • 如果协议文件很大,可以考虑添加WORKING_DIRECTORY参数,避免使用绝对路径

代码安全

  1. 安全性良好,但有以下建议:
    • 可以添加对wayland-scanner工具存在性的检查,确保构建环境正确
    • 考虑添加对输入XML文件存在性的验证

总体评价

这次重构是一个很好的改进,将简单的add_custom_target升级为更规范的add_custom_command,提供了更好的依赖管理和构建透明度。建议的额外改进可以进一步增强构建系统的健壮性和可维护性。

以下是整合了建议的改进版本:

# Generate treeland-ddm protocol files
find_program(WAYLAND_SCANNER wayland-scanner)
if(NOT WAYLAND_SCANNER)
    message(FATAL_ERROR "wayland-scanner not found")
endif()

set(TREELAND_DDM_HEADER ${CMAKE_CURRENT_BINARY_DIR}/treeland-ddm-v1.h)
set(TREELAND_DDM_SOURCE ${CMAKE_CURRENT_BINARY_DIR}/treeland-ddm-v1.c)
set(TREELAND_DDM_XML ${TREELAND_PROTOCOLS_DATA_DIR}/treeland-ddm-v1.xml)

if(NOT EXISTS ${TREELAND_DDM_XML})
    message(FATAL_ERROR "Protocol file ${TREELAND_DDM_XML} not found")
endif()

add_custom_command(
    OUTPUT ${TREELAND_DDM_HEADER}
    COMMAND ${WAYLAND_SCANNER} client-header < ${TREELAND_DDM_XML} > ${TREELAND_DDM_HEADER}
    DEPENDS ${TREELAND_DDM_XML}
    COMMENT "Generating treeland-ddm-v1.h"
)

add_custom_command(
    OUTPUT ${TREELAND_DDM_SOURCE}
    COMMAND ${WAYLAND_SCANNER} private-code < ${TREELAND_DDM_XML} > ${TREELAND_DDM_SOURCE}
    DEPENDS ${TREELAND_DDM_XML}
    COMMENT "Generating treeland-ddm-v1.c"
)

add_custom_target(treeland-ddm ALL
    DEPENDS ${TREELAND_DDM_HEADER} ${TREELAND_DDM_SOURCE}
)

这个改进版本增加了工具和输入文件的检查,并添加了一个统一的目标来管理生成的文件,使构建系统更加健壮和可维护。

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes - here's some feedback:

  • Group the header and source generation into a single add_custom_command or a dedicated custom target to unify protocol file generation and clarify dependencies.
  • Declare the generated files as BYPRODUCTS in your add_custom_command (and consider using MAIN_DEPENDENCY) to ensure correct clean and incremental build behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Group the header and source generation into a single add_custom_command or a dedicated custom target to unify protocol file generation and clarify dependencies.
- Declare the generated files as BYPRODUCTS in your add_custom_command (and consider using MAIN_DEPENDENCY) to ensure correct clean and incremental build behavior.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@sourcery-ai
Copy link

sourcery-ai bot commented Aug 21, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

This PR replaces hardcoded protocol source references with variables, refactors protocol file generation using set() and add_custom_command(), and enhances the build system with DEPENDS and COMMENT fields for improved maintainability and traceability.

File-Level Changes

Change Details Files
Use variables for generated protocol files
  • Define TREELAND_DDM_HEADER and TREELAND_DDM_SOURCE via set()
  • Replace hardcoded treeland-ddm-v1.c in DAEMON_SOURCES with ${TREELAND_DDM_SOURCE}
src/daemon/CMakeLists.txt
Refactor protocol file generation commands
  • Replace single add_custom_target with two add_custom_command blocks
  • Add DEPENDS on the XML protocol file
  • Include COMMENT for each generation step
src/daemon/CMakeLists.txt

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wineee, zccrs

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zccrs zccrs merged commit f157784 into linuxdeepin:master Aug 21, 2025
9 checks passed
@wineee wineee deleted the fix branch August 21, 2025 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants