Skip to content

Conversation

@fly602
Copy link
Contributor

@fly602 fly602 commented Jan 29, 2026

Added missing DtkTools package dependency in main CMakeLists.txt Fixed dconfig file path in misc/CMakeLists.txt by removing incorrect BASE path
Removed redundant DtkTools package search from src/CMakeLists.txt since it's now handled at the top level
The dconfig configuration files were not being installed properly due to incorrect file paths and missing dependencies

Log: Fixed dconfig configuration file installation issue

Influence:

  1. Verify dconfig files are properly installed to correct locations
  2. Test that session configuration settings are applied correctly
  3. Check that all dependencies are resolved during build process
  4. Validate that the build completes without missing package errors
  5. Test session startup and configuration loading

fix: 解决dconfig配置文件安装问题

在主CMakeLists.txt中添加缺失的DtkTools包依赖
修复misc/CMakeLists.txt中的dconfig文件路径,移除错误的BASE路径
从src/CMakeLists.txt中移除冗余的DtkTools包搜索,因为现在在顶层处理
由于错误的文件路径和缺失的依赖,dconfig配置文件之前未能正确安装

Log: 修复dconfig配置文件安装问题

Influence:

  1. 验证dconfig文件是否正确安装到指定位置
  2. 测试会话配置设置是否正确应用
  3. 检查构建过程中所有依赖是否正常解析
  4. 验证构建过程是否完成且无缺失包错误
  5. 测试会话启动和配置加载功能

Summary by Sourcery

Fix dconfig configuration file installation and dependency resolution in the build system.

Bug Fixes:

  • Correct dconfig file globbing and metadata configuration so dconfig JSON files are installed from the proper directory without an incorrect base path.

Build:

  • Centralize the Dtk Tools package lookup in the top-level CMakeLists and remove the redundant find_package from src/CMakeLists to ensure required tooling is available during the build.

Added missing DtkTools package dependency in main CMakeLists.txt
Fixed dconfig file path in misc/CMakeLists.txt by removing incorrect
BASE path
Removed redundant DtkTools package search from src/CMakeLists.txt since
it's now handled at the top level
The dconfig configuration files were not being installed properly due to
incorrect file paths and missing dependencies

Log: Fixed dconfig configuration file installation issue

Influence:
1. Verify dconfig files are properly installed to correct locations
2. Test that session configuration settings are applied correctly
3. Check that all dependencies are resolved during build process
4. Validate that the build completes without missing package errors
5. Test session startup and configuration loading

fix: 解决dconfig配置文件安装问题

在主CMakeLists.txt中添加缺失的DtkTools包依赖
修复misc/CMakeLists.txt中的dconfig文件路径,移除错误的BASE路径
从src/CMakeLists.txt中移除冗余的DtkTools包搜索,因为现在在顶层处理
由于错误的文件路径和缺失的依赖,dconfig配置文件之前未能正确安装

Log: 修复dconfig配置文件安装问题

Influence:
1. 验证dconfig文件是否正确安装到指定位置
2. 测试会话配置设置是否正确应用
3. 检查构建过程中所有依赖是否正常解析
4. 验证构建过程是否完成且无缺失包错误
5. 测试会话启动和配置加载功能
@sourcery-ai
Copy link

sourcery-ai bot commented Jan 29, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Centralizes and fixes DtkTools configuration handling so that dconfig JSON metadata files are found and installed correctly by moving the DtkTools find_package to the top-level CMakeLists and correcting the dconf file glob and macro invocation in misc/CMakeLists.

Sequence diagram for dconfig installation during build

sequenceDiagram
    actor Dev
    participant CMakeTop as CMake_top_level
    participant CMakeMisc as CMake_misc
    participant DtkTools as DtkTools_package
    participant DtkMacro as dtk_add_config_meta_files
    participant FS as Filesystem

    Dev->>CMakeTop: configure_project()
    CMakeTop->>DtkTools: find_package(DtkTools)
    DtkTools-->>CMakeTop: package_found

    CMakeTop->>CMakeMisc: process_subdirectory(misc)
    CMakeMisc->>CMakeMisc: file_glob(dconf/*.json)
    CMakeMisc->>DtkMacro: invoke(APPID, DCONFIG_FILES)
    DtkMacro->>FS: install_dconfig_metadata()
Loading

Flow diagram for corrected dconf JSON discovery and installation

flowchart TD
    A[Start CMake misc configuration] --> B[Glob dconf JSON files with pattern dconf/*.json]
    B --> C[Collect DCONFIG_FILES]
    C --> D[Call dtk_add_config_meta_files with APPID org.deepin.dde.session and FILES DCONFIG_FILES]
    D --> E[Generate install rules for dconfig metadata]
    E --> F[Install dconfig JSON files to correct system location]
    F --> G[End]
Loading

File-Level Changes

Change Details Files
Fix dconfig JSON discovery and config-meta generation in misc CMake logic.
  • Change the glob pattern for dconfig JSON files to look in the local dconf directory instead of misc/dconf
  • Remove the BASE argument from dtk_add_config_meta_files so it uses the corrected file paths directly
misc/CMakeLists.txt
Centralize DtkTools package discovery at the project root.
  • Add a required find_package call for Dtk Tools to the top-level CMakeLists so DtkTools is available project-wide
  • Remove the redundant DtkTools find_package call from src/CMakeLists now that it is handled at the top level
CMakeLists.txt
src/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

deepin pr auto review

这段代码修改主要涉及 CMake 构建脚本的调整,涉及依赖查找、路径处理和宏函数调用。以下是对这段 diff 的详细审查意见:

1. 语法与逻辑审查

  • 依赖查找位置变更

    • 修改点:将 find_package(Dtk${DTK_VERSION_MAJOR} REQUIRED COMPONENTS Tools)src/CMakeLists.txt 移动到了根目录的 CMakeLists.txt
    • 审查意见合理。CMake 推荐将 find_package 调用放在根目录的 CMakeLists.txt 中,而不是子目录中。这样可以确保依赖在全局范围内可用,避免重复查找,且有助于 CMake 更好地处理依赖关系和构建顺序。
  • 路径引用变更

    • 修改点:在 misc/CMakeLists.txt 中,将 file(GLOB DCONFIG_FILES "misc/dconf/*.json") 修改为 file(GLOB DCONFIG_FILES "dconf/*.json")
    • 审查意见合理。通常 CMakeLists.txt 文件所在的当前源目录 ${CMAKE_CURRENT_SOURCE_DIR} 会自动作为相对路径的基准。由于 misc/CMakeLists.txt 本身就在 misc 目录下,之前的路径 misc/dconf 实际上指向的是 misc/misc/dconf,除非构建目录结构非常特殊,否则之前的写法很可能是错误的。修改为 dconf 符合常规逻辑。
  • DTK 宏参数变更

    • 修改点:在 misc/CMakeLists.txt 中,将 dtk_add_config_meta_files(... BASE misc/dconf ...) 修改为 dtk_add_config_meta_files(... FILES ...),去掉了 BASE 参数。
    • 审查意见需确认。这取决于 dtk_add_config_meta_files 的具体实现。
      • 如果该宏在计算文件安装路径时,会基于 BASE 参数计算相对路径,那么去掉 BASE 可能会导致安装后的目录结构发生变化(例如文件可能被直接安装到根配置目录,而不是子目录)。
      • 如果该宏仅仅是为了注册元数据,且 FILES 中的路径已经是绝对路径或相对于预期的正确路径,那么修改是安全的。
      • 建议:需要确认 dtk_add_config_meta_files 的文档或源码,确保去掉 BASE 后不会影响最终生成的元数据文件路径或安装路径。

2. 代码质量与规范

  • GLOB 的使用
    • 审查意见:在 misc/CMakeLists.txt 中使用了 file(GLOB ...) 来收集文件。
    • 改进建议:CMake 官方文档并不推荐在生产环境中使用 GLOB 来收集源文件,因为如果添加了新文件但 CMake 没有重新运行(重新生成构建系统),构建系统可能不知道新文件的存在。
    • 建议:虽然对于配置文件(.json)来说,这种风险比源代码文件要小,但为了严谨性,建议显式列出所有需要安装的 JSON 文件,或者确保构建流程中包含了强制重新运行 CMake 的步骤。

3. 代码性能

  • 影响:本次修改主要是逻辑和路径的调整,对构建性能影响极小。将 find_package 移至根目录实际上可能略微减少重复解析的时间,但可以忽略不计。

4. 代码安全

  • 路径安全file(GLOB ...) 使用相对路径 dconf/*.json。只要 misc 目录下确实存在 dconf 目录且只包含预期的 JSON 文件,就是安全的。
  • 依赖版本:代码中使用了 set(DTK_VERSION_MAJOR 6) 并硬编码查找 DTK6。这是符合项目预期的,没有安全问题。

总结与改进建议

这段代码修改整体上是积极的,主要体现在修正了路径引用错误和规范了依赖查找的位置。为了确保万无一失,建议进行以下改进:

  1. 验证 DTK 宏行为
    务必测试去掉 BASE misc/dconf 参数后,dtk_add_config_meta_files 生成的元数据是否正确,以及这些配置文件最终是否被安装到了正确的系统目录(例如 /etc/dsg/configs/org.deepin.dde.session/ 或类似路径)。如果发现路径层级少了一级,可能需要找回 BASE 参数或者调整 DESTINATION

  2. 显式列出配置文件(可选优化)
    如果 dconf 目录下的文件数量不多且不频繁变动,建议将 file(GLOB ...) 替换为显式列表,以提高构建系统的可预测性。

    # 替代方案
    set(DCONFIG_FILES 
        dconf/file1.json
        dconf/file2.json
    )
  3. 注释说明
    misc/CMakeLists.txt 中修改路径时,建议添加注释说明为什么去掉 misc 前缀(即因为当前 CMake 目录即为 misc),方便后续维护者理解。

修正后的 misc/CMakeLists.txt 示例(建议):

# ... (其他代码)

# 使用相对路径,因为 CMAKE_CURRENT_SOURCE_DIR 即为 misc 目录
file(GLOB DCONFIG_FILES "dconf/*.json")

# 显式列出文件替代 GLOB (推荐做法,防止文件变动未触发 CMake 重跑)
# set(DCONFIG_FILES 
#     dconf/example.json
# )

# 注意:确认 dtk_add_config_meta_files 不需要 BASE 参数来计算安装相对路径
dtk_add_config_meta_files(APPID org.deepin.dde.session FILES ${DCONFIG_FILES})

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 - I've left some high level feedback:

  • In misc/CMakeLists.txt, consider using an explicit path for DCONFIG_FILES (e.g., file(GLOB DCONFIG_FILES "${CMAKE_CURRENT_SOURCE_DIR}/dconf/*.json")) to avoid reliance on the current working directory and make the glob more robust.
  • When using file(GLOB) for the dconf JSON files, you may want to add CONFIGURE_DEPENDS (file(GLOB CONFIGURE_DEPENDS ...)) so that CMake regenerates when new config files are added without requiring a manual cmake re-run.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In misc/CMakeLists.txt, consider using an explicit path for DCONFIG_FILES (e.g., file(GLOB DCONFIG_FILES "${CMAKE_CURRENT_SOURCE_DIR}/dconf/*.json")) to avoid reliance on the current working directory and make the glob more robust.
- When using file(GLOB) for the dconf JSON files, you may want to add CONFIGURE_DEPENDS (file(GLOB CONFIGURE_DEPENDS ...)) so that CMake regenerates when new config files are added without requiring a manual cmake re-run.

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.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fly602, yixinshark

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

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