-
Notifications
You must be signed in to change notification settings - Fork 37
refactor: Remove pzip plugin and update CMake configuration #353
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
refactor: Remove pzip plugin and update CMake configuration #353
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the build configuration to move the pzip-dependent logic from 3rdparty into src, while keeping clipzipplugin gated behind the same architecture/Qt6 conditions and updating comments/messages accordingly. Flow diagram for conditional enabling of pzip and clipzippluginflowchart TD
A[Configure CMake] --> B{Architecture is aarch64 or arm or x86_64 or amd64?}
B -->|no| C[Build without pzip and clipzipplugin]
B -->|yes| D{QT_VERSION_MAJOR equals 6?}
D -->|no| C
D -->|yes| E[Enable src pzip via add_subdirectory in src/CMakeLists]
E --> F[Enable clipzipplugin via add_subdirectory in 3rdparty/CMakeLists]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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:
- The PR description mentions removing the pzip plugin, but
src/CMakeLists.txtnow addsadd_subdirectory(pzip)—please reconcile the description with the actual change and ensure thesrc/pzipdirectory and build targets exist as expected. - In
3rdparty/CMakeLists.txt, the status message still says it is enabling the "pzip 高性能压缩插件" even though onlyclipzippluginis added now; consider updating the message and the new comment about depending onsrc/pzipto clearly reflect the new structure and dependency direction.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The PR description mentions removing the pzip plugin, but `src/CMakeLists.txt` now adds `add_subdirectory(pzip)`—please reconcile the description with the actual change and ensure the `src/pzip` directory and build targets exist as expected.
- In `3rdparty/CMakeLists.txt`, the status message still says it is enabling the "pzip 高性能压缩插件" even though only `clipzipplugin` is added now; consider updating the message and the new comment about depending on `src/pzip` to clearly reflect the new structure and dependency direction.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
49adfe9 to
2301519
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengzhongyuan365-dev, lzwind 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 |
|
/forecemerge |
deepin pr auto review经过对代码差异的审查,这次变更主要是将 以下是从语法逻辑、代码质量、代码性能和代码安全四个方面的详细审查意见: 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
总结与改进建议这次代码重构主要是目录结构的调整,逻辑上是自洽的。为了提高代码的健壮性和可维护性,建议采纳以下改进:
|
- Deleted the pzip plugin directory and its associated files, including CMakeLists.txt, source, and header files. - Updated CMakeLists.txt to reflect the removal of the pzip plugin, ensuring proper build configuration without it. - Adjusted the add_subdirectory command for pzip to use the correct source and binary directories. Log: Clean up project structure by removing the pzip plugin and updating build configurations accordingly.
2301519 to
103509b
Compare
|
/forcemerge |
|
This pr force merged! (status: unstable) |
b00add3
into
linuxdeepin:develop/snipe
Log: Clean up project structure by removing the pzip plugin and updating build configurations accordingly.
Summary by Sourcery
Update CMake configuration to relocate and gate the pzip-related build logic under src while simplifying third-party plugin setup.
Build: