-
Notifications
You must be signed in to change notification settings - Fork 30
fix: resolve dconfig installation issue #187
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
base: master
Are you sure you want to change the base?
Conversation
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. 测试会话启动和配置加载功能
Reviewer's guide (collapsed on small PRs)Reviewer's GuideCentralizes 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 buildsequenceDiagram
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()
Flow diagram for corrected dconf JSON discovery and installationflowchart 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]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这段代码修改主要涉及 CMake 构建脚本的调整,涉及依赖查找、路径处理和宏函数调用。以下是对这段 diff 的详细审查意见: 1. 语法与逻辑审查
2. 代码质量与规范
3. 代码性能
4. 代码安全
总结与改进建议这段代码修改整体上是积极的,主要体现在修正了路径引用错误和规范了依赖查找的位置。为了确保万无一失,建议进行以下改进:
修正后的 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}) |
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.
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.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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:
fix: 解决dconfig配置文件安装问题
在主CMakeLists.txt中添加缺失的DtkTools包依赖
修复misc/CMakeLists.txt中的dconfig文件路径,移除错误的BASE路径
从src/CMakeLists.txt中移除冗余的DtkTools包搜索,因为现在在顶层处理
由于错误的文件路径和缺失的依赖,dconfig配置文件之前未能正确安装
Log: 修复dconfig配置文件安装问题
Influence:
Summary by Sourcery
Fix dconfig configuration file installation and dependency resolution in the build system.
Bug Fixes:
Build: