Skip to content
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

fix: improve Qt version check in CMake #169

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

wyu71
Copy link
Contributor

@wyu71 wyu71 commented Jan 9, 2025

  • Replace commented version check with proper CMake generator expression
  • Use standard CMake LESS operator instead of < operator
  • Clean up QGSettings conditional compilation for Qt5/6
  • Remove trailing empty lines

Log: improve Qt version check in CMake

* Replace commented version check with proper CMake generator expression
* Use standard CMake LESS operator instead of < operator
* Clean up QGSettings conditional compilation for Qt5/6
* Remove trailing empty lines

Log: improve Qt version check in CMake
@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. CMake条件判断

    • 在CMakeLists.txt文件中,使用了if (QT_VERSION_MAJOR LESS 6)来替代原有的#if (QT_VERSION_MAJOR < 6),这是一个好的做法,因为它更加清晰和符合CMake的语法规范。
  2. 条件编译

    • 对于target_include_directoriestarget_link_libraries,使用了$<$<VERSION_LESS:${QT_VERSION_MAJOR},6>:${QGSettings_INCLUDE_DIRS}>$<$<VERSION_LESS:${QT_VERSION_MAJOR},6>:${QGSettings_LIBRARIES}>来替代原有的条件编译注释,这是一个更现代和灵活的方法。
  3. 注释清理

    • 移除了不必要的注释,使得代码更加简洁和易读。
  4. 潜在问题

    • 没有检查pkg_check_modules是否成功,如果dde-dockgsettings-qt库不可用,可能会导致编译失败。建议添加错误处理逻辑。
  5. 代码风格

    • target_link_libraries中,库的顺序可能需要根据依赖关系进行调整,以确保正确的链接顺序。
  6. 安装路径

    • install命令中,默认的安装路径是/usr/local,这可能需要根据实际部署环境进行调整。
  7. 版本兼容性

    • 代码中使用了QT_VERSION_MAJOR来检查Qt的版本,这是一个好的做法,但需要确保在所有目标平台上,Qt的版本信息都是可用的。

总体来说,代码的修改提高了可读性和灵活性,但需要注意潜在的编译依赖问题,并确保在所有目标平台上都能正确处理Qt版本信息。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lzwind, wyu71

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

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

@wyu71
Copy link
Contributor Author

wyu71 commented Jan 9, 2025

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Jan 9, 2025

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit 3ae1cbc into linuxdeepin:develop/qt6.8 Jan 9, 2025
7 of 9 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.

3 participants