Skip to content

chore: reorder GNUInstallDirs include in CMakeLists#269

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Johnson-zs:master
Mar 19, 2026
Merged

chore: reorder GNUInstallDirs include in CMakeLists#269
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Johnson-zs:master

Conversation

@Johnson-zs
Copy link
Copy Markdown
Contributor

Moved the include(GNUInstallDirs) directive to after the CMAKE_INSTALL_PREFIX check for better logical flow and to ensure proper variable initialization sequence. The GNUInstallDirs module should be included after setting the install prefix to ensure it uses the correct installation paths based on the configured prefix.

Influence:

  1. Verify CMake configuration completes successfully
  2. Check that installation paths are correctly generated
  3. Test build process to ensure no regressions
  4. Confirm that the project installs to the correct locations

chore: 在 CMakeLists 中重新排序 GNUInstallDirs 包含

将 include(GNUInstallDirs) 指令移动到 CMAKE_INSTALL_PREFIX 检查之后,以 实现更好的逻辑流程并确保正确的变量初始化顺序。GNUInstallDirs 模块应在设
置安装前缀后包含,以确保它基于配置的前缀使用正确的安装路径。

Influence:

  1. 验证 CMake 配置成功完成
  2. 检查安装路径是否正确生成
  3. 测试构建过程以确保没有回归问题
  4. 确认项目安装到正确的位置

Moved the include(GNUInstallDirs) directive to after the
CMAKE_INSTALL_PREFIX check for better logical flow and to ensure proper
variable initialization sequence. The GNUInstallDirs module should be
included after setting the install prefix to ensure it uses the correct
installation paths based on the configured prefix.

Influence:
1. Verify CMake configuration completes successfully
2. Check that installation paths are correctly generated
3. Test build process to ensure no regressions
4. Confirm that the project installs to the correct locations

chore: 在 CMakeLists 中重新排序 GNUInstallDirs 包含

将 include(GNUInstallDirs) 指令移动到 CMAKE_INSTALL_PREFIX 检查之后,以
实现更好的逻辑流程并确保正确的变量初始化顺序。GNUInstallDirs 模块应在设
置安装前缀后包含,以确保它基于配置的前缀使用正确的安装路径。

Influence:
1. 验证 CMake 配置成功完成
2. 检查安装路径是否正确生成
3. 测试构建过程以确保没有回归问题
4. 确认项目安装到正确的位置
@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Johnson-zs

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
Copy Markdown

deepin pr auto review

这段代码对 include(GNUInstallDirs) 的位置进行了调整。这是一个正确的改进,但为了代码的健壮性和可维护性,还有进一步优化的空间。

以下是详细的审查意见和改进建议:

1. 代码逻辑与语法审查

  • 当前变更分析
    • 修改前include(GNUInstallDirs) 位于文件顶部,CMAKE_INSTALL_PREFIX 被设置为 /usr 之前。
    • 修改后include(GNUInstallDirs) 移动到了 CMAKE_INSTALL_PREFIX 设置之后。
  • 逻辑正确性正确
    • GNUInstallDirs 模块会根据 CMAKE_INSTALL_PREFIX 的值来计算安装路径(如 CMAKE_INSTALL_BINDIR, CMAKE_INSTALL_LIBDIR 等)。
    • 如果在 CMAKE_INSTALL_PREFIX 设置为 /usr 之前就包含该模块,在某些情况下(特别是涉及多架构安装路径时,如 /usr/lib/x86_64-linux-gnu),路径计算可能基于默认前缀,导致逻辑不符合预期。虽然 CMake 变量通常是延迟求值的,但在显式设置安装前缀后包含该模块,语义更加清晰和安全。

2. 代码质量与改进意见

虽然当前的移动解决了潜在的顺序依赖问题,但代码中存在一个常见的硬编码问题,建议一并修改。

  • 问题点

    if (CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
      set(CMAKE_INSTALL_PREFIX /usr)
    endif ()

    这里直接硬编码了 /usr。这会覆盖用户通过 cmake -DCMAKE_INSTALL_PREFIX=... 指定的路径,且在非 Linux 系统(如 Windows 或 macOS)上,/usr 往往不是标准的安装位置(macOS 通常为 /usr/local/Library,Windows 为 C:/Program Files)。

  • 改进建议
    应该根据操作系统类型动态设置默认安装路径,或者仅在 Linux 下强制设置为 /usr。如果是通用的库项目,建议使用 GNUInstallDirs 提供的变量,而不是直接假设路径。

    优化后的代码片段

    # ... (前面的 Qt 查找代码) ...
    
    find_package(PkgConfig REQUIRED)
    
    # 改进点:根据操作系统设置默认安装路径
    if (CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
        if(UNIX AND NOT APPLE)
            set(CMAKE_INSTALL_PREFIX "/usr" CACHE PATH "Install path prefix, prepended onto install directories." FORCE)
        elseif(APPLE)
            set(CMAKE_INSTALL_PREFIX "/usr/local" CACHE PATH "Install path prefix, prepended onto install directories." FORCE)
        endif()
    endif()
    
    # 必须在设置完 CMAKE_INSTALL_PREFIX 之后包含
    include(GNUInstallDirs)
    
    # ... (后面的 add_subdirectory) ...

3. 代码性能

  • 影响:无。
  • 分析include(GNUInstallDirs) 是一个轻量级的脚本包含操作,无论放在文件的哪个位置,对构建过程的性能影响可以忽略不计。

4. 代码安全

  • 影响:低风险。
  • 分析
    • 修改后的顺序确保了路径变量计算的上下文是正确的,避免了潜在的路径拼接错误。
    • 需要注意的是,强制修改 CMAKE_INSTALL_PREFIX/usr 需要管理员权限才能执行 make install。如果这是为了构建 Linux 发行版软件包,通常由构建系统(如 rpmbuild/debian)在外部指定前缀,CMakeLists 中不应强制覆盖,除非是为了特定的发行版适配。

总结

这次 diff 将 include(GNUInstallDirs) 下移是合理的,确保了安装前缀变量被正确初始化后再计算相对路径。

主要建议:在修正顺序的同时,建议审查 set(CMAKE_INSTALL_PREFIX /usr) 这一行,确保它在所有目标平台上都能按预期工作,或者添加平台判断逻辑(如上面的改进建议所示),以提高代码的跨平台兼容性。

@Johnson-zs
Copy link
Copy Markdown
Contributor Author

/forcemerge

@deepin-bot
Copy link
Copy Markdown

deepin-bot bot commented Mar 19, 2026

This pr force merged! (status: blocked)

@deepin-bot deepin-bot bot merged commit 68078aa into linuxdeepin:master Mar 19, 2026
21 checks passed
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