feat(uninstall): Add support for APM package uninstallation#59
feat(uninstall): Add support for APM package uninstallation#59shenmo7192 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shenmo7192 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 |
|
Hi @shenmo7192. Thanks for your PR. 😃 |
|
Hi @shenmo7192. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideImplements APM package uninstallation support by wiring a new APM-specific uninstall path into the D-Bus launcher, adding an APM package type, installing a new privileged uninstall script, and updating polkit/CMake configuration accordingly. Sequence diagram for APM package uninstallation flowsequenceDiagram
actor User
participant LauncherUI
participant Launcher1Compat
participant pkexec
participant dde_appwiz_apm_uninstaller_sh
participant apm
User->>LauncherUI: Click_uninstall_on_APM_app
LauncherUI->>Launcher1Compat: RequestUninstall(desktop, skipPreinstallHook)
Launcher1Compat->>Launcher1Compat: Parse_desktop_file
Launcher1Compat->>Launcher1Compat: Detect_X_APM_APPID
Launcher1Compat->>Launcher1Compat: m_packageDisplayName = ddeDisplayName()
Launcher1Compat->>dde_appwiz_apm_uninstaller_sh: start_via_pkexec("/usr/libexec/dde-appwiz-apm-uninstaller.sh", desktopFilePath)
activate pkexec
pkexec->>dde_appwiz_apm_uninstaller_sh: Run_with_elevated_privileges
deactivate pkexec
activate dde_appwiz_apm_uninstaller_sh
dde_appwiz_apm_uninstaller_sh->>dde_appwiz_apm_uninstaller_sh: Validate_desktop_file_and_extract_APPID
dde_appwiz_apm_uninstaller_sh->>apm: apm_autoremove(APPID,_y)
apm-->>dde_appwiz_apm_uninstaller_sh: Exit_status
deactivate dde_appwiz_apm_uninstaller_sh
dde_appwiz_apm_uninstaller_sh-->>Launcher1Compat: Exit_code_and_output
alt Uninstall_success
Launcher1Compat->>Launcher1Compat: sendNotification(m_packageDisplayName,true,m_base64Icon)
Launcher1Compat->>Launcher1Compat: postUninstallCleanUp(fileName,PackageType_APM)
Launcher1Compat-->>LauncherUI: UninstallSuccess(desktopFilePath)
else Uninstall_failure
Launcher1Compat->>Launcher1Compat: sendNotification(m_packageDisplayName,false,m_base64Icon)
Launcher1Compat-->>LauncherUI: UninstallFailure(desktopFilePath)
end
Updated class diagram for Launcher1Compat and PackageType with APM supportclassDiagram
class PackageType {
<<enumeration>>
Linglong
Flatpak
Deb
DCM
APM
}
class Launcher1Compat {
+RequestUninstall(desktop, skipPreinstallHook) void
-uninstallPackageByScript(pkgDisplayName, packageDesktopFilePath) void
-uninstallAPMPackage(pkgDisplayName, packageDesktopFilePath) void
-sendNotification(pkgDisplayName, success, base64Icon) void
-postUninstallCleanUp(desktopId, packageType) void
-m_packageDisplayName QString
-m_base64Icon QString
-m_desktopFilePath QString
}
Launcher1Compat --> PackageType : 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 - I've found 3 issues, and left some high level feedback:
- The new
uninstallAPMPackagepath layerspkexecin C++ on top of a script that may also elevate, which is both redundant and makes control flow harder to reason about; consider consolidating privilege escalation to either the C++ caller or the script and passing arguments topkexecwithout going through an extra shell where possible. - In
uninstallAPMPackageyou callpostUninstallCleanUp(fi.fileName(), PackageType::APM)with a known// FIXME: THIS IS NOT DESKTOP IDcomment; it would be better to resolve the correct desktop ID (or explicitly document why this is acceptable for APM) before relying on this in the new uninstall path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `uninstallAPMPackage` path layers `pkexec` in C++ on top of a script that may also elevate, which is both redundant and makes control flow harder to reason about; consider consolidating privilege escalation to either the C++ caller or the script and passing arguments to `pkexec` without going through an extra shell where possible.
- In `uninstallAPMPackage` you call `postUninstallCleanUp(fi.fileName(), PackageType::APM)` with a known `// FIXME: THIS IS NOT DESKTOP ID` comment; it would be better to resolve the correct desktop ID (or explicitly document why this is acceptable for APM) before relying on this in the new uninstall path.
## Individual Comments
### Comment 1
<location path="dbus/launcher1compat.cpp" line_range="183-189" />
<code_context>
}
}
+void Launcher1Compat::uninstallAPMPackage(const QString & pkgDisplayName, const QString & packageDesktopFilePath)
+{
+ // call `/usr/libexec/dde-appwiz-apm-uninstaller.sh <packageDesktopFilePath>` and check the return code.
+ qDebug() << "Calling dde-appwiz-apm-uninstaller.sh to uninstall" << pkgDisplayName << packageDesktopFilePath << "via script";
+ QProcess process;
+ process.start("pkexec", QStringList{"/usr/libexec/dde-appwiz-apm-uninstaller.sh", packageDesktopFilePath});
+ process.waitForFinished();
+
+ QString standardOutput = process.readAllStandardOutput();
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider using a timeout and checking the return value of waitForFinished to avoid potential hangs.
Without a timeout or checking the return value, `waitForFinished()` can block this thread indefinitely if `pkexec` or the script hangs. Please either use `waitForFinished(ms)` and handle the `false` case, or at least check the boolean return and treat an unfinished process as a failure when determining which notification to send.
</issue_to_address>
### Comment 2
<location path="dbus/launcher1compat.cpp" line_range="199-200" />
<code_context>
+ if (process.exitCode() != 0) {
+ sendNotification(pkgDisplayName, false, m_base64Icon);
+ } else {
+ sendNotification(pkgDisplayName, true, m_base64Icon);
+ QFileInfo fi(m_desktopFilePath);
+ // FIXME: THIS IS NOT DESKTOP ID
+ postUninstallCleanUp(fi.fileName(), PackageType::APM);
</code_context>
<issue_to_address>
**issue (bug_risk):** Using m_desktopFilePath instead of the function parameter can cause incorrect cleanup.
In `uninstallAPMPackage`, `packageDesktopFilePath` is passed in but `QFileInfo` is built from `m_desktopFilePath`. If `m_desktopFilePath` is stale or differs from the argument, `postUninstallCleanUp` could operate on the wrong desktop file. Construct `QFileInfo` from `packageDesktopFilePath` instead so cleanup is correctly scoped to the function input.
</issue_to_address>
### Comment 3
<location path="scripts/dde-appwiz-apm-uninstaller.sh" line_range="39-47" />
<code_context>
+echo "找到 APM ID: $APPID"
+
+# 构建命令
+CMD="apm autoremove $APPID -y"
+
+# 如果不是root用户，则使用pkexec
+if [ "$EUID" -ne 0 ]; then
+ echo "当前不是root用户，使用 pkexec 提升权限..."
+ exec pkexec bash -c "$CMD"
+else
+ echo "以root用户身份执行命令..."
+ exec $CMD
+fi
\ No newline at end of file
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Avoid building a single shell command string when APPID is user- or file-derived.
Building `CMD="apm autoremove $APPID -y"` and then calling `exec $CMD`/`exec pkexec bash -c "$CMD"` depends on word-splitting and lets unexpected characters in `APPID` alter the command. Instead, pass arguments directly to `exec` and quote `APPID`:
```bash
if [ "$EUID" -ne 0 ]; then
exec pkexec apm autoremove "$APPID" -y
else
exec apm autoremove "$APPID" -y
fi
```
This avoids `bash -c` and reduces the risk of command/argument injection.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
424225a to
65718b9
Compare
- Add APM uninstallation permissions in polkit rules - Add new APM uninstallation script - Add APM type to PackageType enum - Implement APM package uninstallation logic - Update CMakeLists installation configuration
BLumia
left a comment
BLumia
left a comment
There was a problem hiding this comment.
这些 desktop 文件是完全由 APM 管理的吗？是否可以使用 X-Deepin-PreUninstall 字段？
这个字段的含义是：通常指向一个 shell script，卸载前会先执行这个字段的脚本或命令，如果返回 0 则会继续执行原有卸载行为，返回非 0 不会进行 DDE 的卸载行为。
如果这些 desktop 完全是 APM 管理的话，直接使用这个字段指向 APM 的卸载脚本就可以了，只需要确保能正常卸载并且返回非 0 值即可。
下面有一些预留的返回值含义，是兼容模式在使用的：
- 0：正常执行了卸载前钩子，需要继续进行后续的常规卸载行为
- 101：用户在卸载前钩子阶段取消了卸载（比如卸载前钩子脚本中弹对话框提示确认，用户点了取消）
- 103：已有（由钩子一侧维护的）卸载行为正在进行，拒绝进行当前卸载
这些desktop是由APM维护的，在打包时就已经维护好了，目前 APM 并不支持主动修改软件包的desktop文件。 X-Deepin-PreUninstall 听起来像是由DDE维护的字段，由 APM 直接编辑不太合适吧.... APM本身只包含bash和一些挂载工具，并不提供对话框，如果要做额外的确认的话，估计要用zenity了，这可能会降低用户体验一致性 |
|
建议使用这个字段是单纯因为这个字段是 DDE 产品需求的一部分，不会轻易移除。APM 的卸载功能假定能合入也不会是产品需求的一部分，不会确保会不会哪天相关代码由别的组/团队维护后因为完全不清楚这个非需求文档描述的需求的事情而被移除。
是表示由 DDE 提供支持的字段，或者说是 DDE 的扩展字段。值应当由有使用此扩展字段的一方主动提供。APM 是可以用的。
和是否需要提供对话框没关系，上面提到只是因为兼容模式自己提供了。 |
这个钩子的格式是什么？我直接写这个脚本的绝对路径，然后dde-app-wiz会把这个desktop的路径传过去这样？ |
单纯调脚本，不会额外传任何参数。所以如果需要什么额外的信息的话，需要在这个字段里自行提供。 直接看代码里有 |
|
3 participants
Add APM package uninstallation functionality, including:
Summary by Sourcery
Add support for uninstalling APM-packaged applications through the application wizard.
New Features:
Enhancements:
Build: