Skip to content

Conversation

@deepin-ci-robot
Copy link
Contributor

Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#532

Synchronize source files from linuxdeepin/dtkcore.

Source-pull-request: linuxdeepin/dtkcore#532
@deepin-ci-robot
Copy link
Contributor Author

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deepin-ci-robot

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

@deepin-ci-robot
Copy link
Contributor Author

deepin pr auto review

这份代码变更主要目的是重构 DTK (Deepin Tool Kit) 的版本控制逻辑,从基于 VERSION 文件自动推断主版本号(5或6)的方式,改为使用 CMake 的 option() 机制显式控制(DTK5/DTK6),并引入了 DTK_NAME_SUFFIX 来统一处理库文件命名和依赖查找。

以下是详细的审查意见,分为语法逻辑、代码质量、代码性能和代码安全四个方面:

1. 语法逻辑

优点:

  • 逻辑清晰化:将版本判断从隐式(读取文件内容字符串比较)改为显式(option(DTK5 ...)),使得构建配置更加直观,用户在 CMake GUI 或命令行时能更清楚地看到并控制构建目标。
  • 命名一致性:引入 DTK_NAME_SUFFIX 变量(DTK5为空,DTK6为"6"),统一了库文件名(如 dtkcore vs dtk6core)、CMake 配置文件名、查找包名(find_package)和命名空间。这解决了之前可能存在的命名不一致问题。
  • 依赖关系修正:在 src/log/LogManager.cpp 中,修正了一个明显的逻辑错误:
    // 修改前
    } else if (m_fallbackConfig && m_dsgConfig->isInitializeSucceed() && ...) {
    // 修改后
    } else if (m_fallbackConfig && m_fallbackConfig->isInitializeSucceed() && ...) {
    这确保了检查的是正确的配置对象状态。

潜在问题与建议:

  • 版本号解析的缺失
    CMakeLists.txt 开头:
    file(READ "${CMAKE_CURRENT_SOURCE_DIR}/VERSION" FILE_VERSION)
    string(STRIP "${FILE_VERSION}" FILE_VERSION)
    代码读取了完整的版本号(例如 "5.6.0"),但在后续逻辑中,直接将其赋值给 project(VERSION ${FILE_VERSION})。虽然 CMake 的 project 命令会自动解析 PROJECT_VERSION_MAJOR 等变量,但代码紧接着手动构建了 DTK_VERSION
    set(DTK_VERSION_MAJOR "5") # or "6" based on option
    set(DTK_VERSION_MINOR ${PROJECT_VERSION_MINOR})
    set(DTK_VERSION_PATCH ${PROJECT_VERSION_PATCH})
    set(DTK_VERSION "${DTK_VERSION_MAJOR}.${DTK_VERSION_MINOR}.${DTK_VERSION_PATCH}")
    风险:如果 VERSION 文件中的主版本号(如 "5")与 option(DTK5 ...) 的选择(如选择构建 DTK6)不一致,会导致生成的 DTK_VERSION 变量语义混乱(例如 DTK6 构建却生成了 "5.x.x" 的版本字符串)。
    建议:增加一致性检查,或者完全依赖 option 来决定版本号,忽略 VERSION 文件中的主版本号,仅使用其次版本号和修订号。或者,移除 option,完全由 VERSION 文件内容决定(但这违背了此次重构显式控制的初衷)。最好的做法是:
    # 读取 VERSION 文件
    file(READ "${CMAKE_CURRENT_SOURCE_DIR}/VERSION" _FILE_VERSION_FULL)
    string(STRIP "${_FILE_VERSION_FULL}" _FILE_VERSION_FULL)
    
    # 解析主版本号用于验证或设置默认值
    string(REGEX MATCH "^([0-9]+)\\.([0-9]+)\\.([0-9]+)" _ "${_FILE_VERSION_FULL}")
    set(_FILE_MAJOR ${CMAKE_MATCH_1})
    set(_FILE_MINOR ${CMAKE_MATCH_2})
    set(_FILE_PATCH ${CMAKE_MATCH_3})
    
    # 根据 option 决定实际构建的主版本
    if(DTK5)
        set(DTK_VERSION_MAJOR "5")
    else()
        set(DTK_VERSION_MAJOR "6")
    endif()
    
    # 警告用户如果 option 和文件不一致
    if(NOT _FILE_MAJOR STREQUAL DTK_VERSION_MAJOR)
        message(WARNING "VERSION file specifies major version ${_FILE_MAJOR}, but DTK5 option is set to ${DTK5}. Using ${DTK_VERSION_MAJOR} as the major version.")
    endif()
    
    set(DTK_VERSION_MINOR ${_FILE_MINOR})
    set(DTK_VERSION_PATCH ${_FILE_PATCH})
    set(DTK_VERSION "${DTK_VERSION_MAJOR}.${DTK_VERSION_MINOR}.${DTK_VERSION_PATCH}")

2. 代码质量

优点:

  • 可维护性提升:使用 DTK_NAME_SUFFIX 替代到处拼接 DTK_VERSION_MAJOR,减少了字符串操作的重复代码,降低了出错概率。
  • 代码清理:移除了 dtkcore.cmake 中不再使用的变量设置逻辑(如重复设置 DTK_VERSION_MAJOR)和注释掉的代码。
  • 条件编译优化:在 src/settings/settings.cmakesrc/util/util.cmake 中,将条件判断从 if(DTK_VERSION_MAJOR)(隐式依赖变量非空即真)改为 if(NOT DTK5),逻辑意图更加明确。

潜在问题与建议:

  • 拼写错误
    dtkcore.cmake 中:
    option(BUIILD_TESTING "Build tests" OFF)
    BUIILD_TESTING 拼写错误(多了一个 I)。虽然这只是一个定义,不会导致构建失败,但用户如果尝试使用 -DBUIILD_TESTING=ON 可能会困惑为什么没生效,或者如果后续代码中有引用 BUILD_TESTING(标准变量名),则该 option 将无效。
    建议:修正为 option(BUILD_TESTING "Build tests" OFF)
  • 测试构建优化
    tests/CMakeLists.txt 中,将 Address Sanitizer 限制在 Debug 模式下是很好的实践:
    if (CMAKE_BUILD_TYPE STREQUAL "Debug")
        add_compile_options(-fsanitize=address)
        add_link_options(-fsanitize=address)
    endif()
    这避免了 Release 版本不必要的性能损耗。
    建议:考虑进一步封装,允许用户通过 option 控制是否开启 sanitizer,例如 option(ENABLE_ASAN "Enable Address Sanitizer" OFF),并在 Debug 模式下默认开启。

3. 代码性能

  • 构建系统性能:此次变更主要是逻辑层面的重构,对 CMake 处理性能影响极小。移除了一些冗余的变量设置,可能对配置时间有微小的正面影响。
  • 运行时性能LogManager.cpp 中的逻辑修正虽然不直接提升性能,但确保了代码按预期执行,避免了因逻辑错误导致的潜在异常路径。

4. 代码安全

  • RPATH 设置
    tests/CMakeLists.txt 中添加了:
    set_target_properties(${BIN_NAME} PROPERTIES
        SKIP_BUILD_RPATH FALSE
    )
    这有助于在构建目录中正确运行测试,找到依赖的共享库(如 libvtabletest*.so)。这是一个好的实践,确保了测试环境的完整性。
  • 输入验证
    如前所述,VERSION 文件内容与 DTK5 option 之间缺乏一致性检查可能导致版本号欺骗或混淆。虽然不是严重的安全漏洞,但在软件供应链中,版本号的准确性很重要。建议采纳上文提到的检查逻辑。

总结

这是一次高质量的代码重构,显著改善了构建系统的可维护性和逻辑清晰度。主要的改进在于显式化版本控制选项和统一命名规范。

主要建议修改点:

  1. 修正 dtkcore.cmake 中的拼写错误:BUIILD_TESTING -> BUILD_TESTING
  2. CMakeLists.txt 顶部增加 VERSION 文件内容与 DTK5 option 的一致性检查逻辑,防止版本号混淆。
  3. 考虑将 Sanitizer 的开启改为更灵活的 option 配置。

@github-actions
Copy link
Contributor

  • 敏感词检查失败, 检测到2个文件存在敏感词
详情
{
    "CMakeLists.txt": [
        {
            "line": "  HOMEPAGE_URL \"https://github.com/linuxdeepin/dtkcore\"",
            "line_number": 9,
            "rule": "S35",
            "reason": "Url link | e18a6b5679"
        }
    ],
    "docs/CMakeLists.txt": [
        {
            "line": "execute_process(COMMAND git clone https://github.com/linuxdeepin/doxygen-theme.git --depth=1",
            "line_number": 41,
            "rule": "S35",
            "reason": "Url link | d2deace9b0"
        }
    ]
}

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.

2 participants