fix: recovery rpath #1134
Merged
fix: recovery rpath #1134
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepin-mozart The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR refactors runtime library path handling by removing explicit path additions in the application code and reinstates RPATH configuration in the build system, including logging of the install RPATH. Class diagram for changes in main.cpp plugin and library path handlingclassDiagram
class LifeCycle {
+setPluginPaths(paths)
+pluginPaths()
}
class CustomPaths {
+global(type)
}
class QApplication {
+libraryPaths()
}
class QCoreApplication {
+addLibraryPath(path)
}
class App {
}
App --> LifeCycle : uses
App --> CustomPaths : uses
App --> QApplication : uses
App --> QCoreApplication : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/app/main.cpp:39` </location>
<code_context>
- QCoreApplication::addLibraryPath(pluginsPath);
-
- qInfo() << "Current library paths:" << QApplication::libraryPaths();
+ qInfo() << "Depend library paths:" << QApplication::libraryPaths();
qInfo() << "Load plugin paths: " << dpf::LifeCycle::pluginPaths();
</code_context>
<issue_to_address>
**suggestion:** The log message label may be misleading after removing library path additions.
Update the log label to match the actual library paths being shown, as the code no longer adds dependlibs or pluginsPath.
```suggestion
qInfo() << "Current Qt library paths:" << QApplication::libraryPaths();
```
</issue_to_address>
### Comment 2
<location> `CMakeLists.txt:50` </location>
<code_context>
+set(CMAKE_INSTALL_RPATH ${LIBRARY_INSTALL_PREFIX} ${PLUGIN_PATH})
message(STATUS "LIBRARY_INSTALL_PREFIX ${LIBRARY_INSTALL_PREFIX}")
message(STATUS "RUNTIME_INSTALL_RPEFIX ${RUNTIME_INSTALL_RPEFIX}")
</code_context>
<issue_to_address>
**issue (typo):** Repeated typo: 'RPEFIX' should likely be 'PREFIX' in variable names.
Please update 'RUNTIME_INSTALL_RPEFIX', 'SOURCES_INSTALL_RPEFIX', and 'INCLUDE_INSTALL_RPEFIX' to use 'PREFIX' instead of 'RPEFIX'.
Suggested implementation:
```
message(STATUS "RUNTIME_INSTALL_PREFIX ${RUNTIME_INSTALL_PREFIX}")
message(STATUS "INCLUDE_INSTALL_PREFIX ${INCLUDE_INSTALL_PREFIX}")
```
If the variables `RUNTIME_INSTALL_RPEFIX` and `INCLUDE_INSTALL_RPEFIX` are defined elsewhere in the file or project, you should also rename those definitions to use `PREFIX` instead of `RPEFIX`. Similarly, check for `SOURCES_INSTALL_RPEFIX` and update it to `SOURCES_INSTALL_PREFIX` wherever it appears.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| QCoreApplication::addLibraryPath(pluginsPath); | ||
|
|
||
| qInfo() << "Current library paths:" << QApplication::libraryPaths(); | ||
| qInfo() << "Depend library paths:" << QApplication::libraryPaths(); |
There was a problem hiding this comment.
suggestion: The log message label may be misleading after removing library path additions.
Update the log label to match the actual library paths being shown, as the code no longer adds dependlibs or pluginsPath.
Suggested change
| qInfo() << "Depend library paths:" << QApplication::libraryPaths(); | |
| qInfo() << "Current Qt library paths:" << QApplication::libraryPaths(); |
| message(STATUS "LIBRARY_INSTALL_PREFIX ${LIBRARY_INSTALL_PREFIX}") | ||
| message(STATUS "RUNTIME_INSTALL_RPEFIX ${RUNTIME_INSTALL_RPEFIX}") | ||
| message(STATUS "SOURCES_INSTALL_RPEFIX ${SOURCES_INSTALL_RPEFIX}") | ||
| message(STATUS "INCLUDE_INSTALL_RPEFIX ${INCLUDE_INSTALL_RPEFIX}") |
There was a problem hiding this comment.
issue (typo): Repeated typo: 'RPEFIX' should likely be 'PREFIX' in variable names.
Please update 'RUNTIME_INSTALL_RPEFIX', 'SOURCES_INSTALL_RPEFIX', and 'INCLUDE_INSTALL_RPEFIX' to use 'PREFIX' instead of 'RPEFIX'.
Suggested implementation:
message(STATUS "RUNTIME_INSTALL_PREFIX ${RUNTIME_INSTALL_PREFIX}")
message(STATUS "INCLUDE_INSTALL_PREFIX ${INCLUDE_INSTALL_PREFIX}")
If the variables RUNTIME_INSTALL_RPEFIX and INCLUDE_INSTALL_RPEFIX are defined elsewhere in the file or project, you should also rename those definitions to use PREFIX instead of RPEFIX. Similarly, check for SOURCES_INSTALL_RPEFIX and update it to SOURCES_INSTALL_PREFIX wherever it appears.
Log: Change-Id: I715be11bc12142168fb36e03bb73cc1785321377
deepin pr auto review我来对这段代码变更进行审查：
+ set(CMAKE_INSTALL_RPATH ${LIBRARY_INSTALL_PREFIX} ${PLUGIN_PATH})优点：
问题：
建议修改为： set(CMAKE_INSTALL_RPATH ${LIBRARY_INSTALL_PREFIX} ${PLUGIN_INSTALL_PATH})
- QString dependlibs = CustomPaths::global(CustomPaths::DependLibs);
- QCoreApplication::addLibraryPath(dependlibs);
- QCoreApplication::addLibraryPath(pluginsPath);
- qInfo() << "Current library paths:" << QApplication::libraryPaths();
+ qInfo() << "Depend library paths:" << QApplication::libraryPaths();问题：
建议：
- private slots:
- void closeMainWindow(); // 声明用于关闭主窗口的槽函数问题：
总体建议：
安全性和性能：
建议在提交前：
|
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Log:
Change-Id: I715be11bc12142168fb36e03bb73cc1785321377
Summary by Sourcery
Fix runtime path configuration and streamline plugin load logging
Enhancements:
Build: