-
Notifications
You must be signed in to change notification settings - Fork 55
fix: 删掉重复编译处理方式——处理打包单元测试报错问题。 #1357
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
Conversation
Reviewer's GuideThis PR refines the dock task manager app item appearance by reducing the border width in QML and fixes runtime linking of taskmanager plugin libraries for unit tests by configuring explicit RPATH and linker rpath flags for the rolecombinemodel_tests and rolegroupmodel_tests CMake targets. 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 there - I've reviewed your changes - here's some feedback:
- The test targets now configure RPATH both via
set_target_properties(... BUILD_RPATH ...)and explicittarget_link_options(... -rpath,...); consider consolidating to a single mechanism (preferablyBUILD_RPATH/CMAKE_BUILD_RPATH) to avoid redundancy and potential divergence if one is updated without the other. - Hardcoding
${CMAKE_BINARY_DIR}/pluginsand${CMAKE_BINARY_DIR}/framein the test RPATH makes the configuration less flexible; if possible, derive these paths from the corresponding plugin targets or use$ORIGIN-based paths so the tests are more robust to layout changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test targets now configure RPATH both via `set_target_properties(... BUILD_RPATH ...)` and explicit `target_link_options(... -rpath,...)`; consider consolidating to a single mechanism (preferably `BUILD_RPATH`/`CMAKE_BUILD_RPATH`) to avoid redundancy and potential divergence if one is updated without the other.
- Hardcoding `${CMAKE_BINARY_DIR}/plugins` and `${CMAKE_BINARY_DIR}/frame` in the test RPATH makes the configuration less flexible; if possible, derive these paths from the corresponding plugin targets or use `$ORIGIN`-based paths so the tests are more robust to layout changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
77f8b7a to
f95272b
Compare
24982ba to
6741a3c
Compare
临时修复,后续再找到方法改回来。 Logs:
6741a3c to
708869d
Compare
deepin pr auto review我来对这个git diff进行审查:
改进建议:
示例改进后的代码: # reproducible编译参数
# 注释掉SKIP_BUILD_RPATH是因为需要解决[bug编号]中描述的问题
# 临时启用RPATH功能,后续会寻找更安全的解决方案
#DEB_CMAKE_EXTRA_FLAGS += -DCMAKE_SKIP_BUILD_RPATH=ON
# 替代方案:使用RUNPATH(更安全)
DEB_CMAKE_EXTRA_FLAGS += -DCMAKE_SKIP_RPATH=ON -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, wjyrich 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 |
|
/forcemerge |
|
This pr force merged! (status: behind) |
临时修复,后续再找到方法改回来。
Logs: