Safer handling of the initial installation of add-ons by seanbudd · Pull Request #16851 · nvaccess/nvda

seanbudd · 2024-07-12T05:44:42Z

Link to issue number:

Fixes #16704
Fixup of #15967

Summary of the issue:

In the following code block:

https://github.com/nvaccess/nvda/blob/release-2024.2/source/addonHandler/__init__.py#L432-L454

If an error is raised from running addon.runInstallTask("onInstall"), it is caught, and then re-raised as AddonError
However, this exception is caught again by the finally statement returning the add-on
returning from finally suppresses the exception. This means the callers don't correctly handle the expected exception, as it is suppressed, where it should instead prevent the installation from continuing.
We have to return the add-on to perform clean up tasks, but we have to know exceptions were raised to cancel the add-on installation

Description of user facing changes

Add-on installation failures should fail gracefully

Description of development approach

Instead of raising exceptions, store them and exit the installation process when a failure occurs.
If exceptions are stored, cancel the installation and perform the require clean-up tasks

Testing strategy:

Test external add-on install

Download add-on which raises an error on installation installTasks.nvda-addon.zip
- remove .zip from the name
Install the add-on through the Add-on Store (external install button) and through opening an add-on file,
- Note on alpha, this installation fails silently as exceptions are ignored
- Note on this PR, a warning is shown for installation failure

Test install/update from add-on store

Update add-on through add-on store
Before exiting the store:
- Go to %APPDATA%\nvda\addonStore\_dl
- replace the downloaded add-on file with the installTasks.nvda-addon. Rename the file to match
Exit the add-on store
- Note on alpha, this installation fails silently as exceptions are ignored
- Note on this PR, a warning is shown for installation failure

Known issues with pull request:

None

Code Review Checklist:

Documentation:
- Change log entry
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
API is compatible with existing add-ons.
Security precautions taken.

Summary by CodeRabbit

New Features
- Enhanced exception handling during addon installation, improving reliability and user feedback.
- Installation process now provides better logging and error management.
Improvements
- Improved cleanup handling in the event of installation failure to ensure system stability.
- Updated error handling to ensure proper cleanup of addon imports and more robust exception management during the installation process.
Documentation
- Updated function descriptions and parameter documentation for clarity.
Refactor
- Refactored functions to improve readability and maintainability.
- Introduced new context manager for better resource management during addon installation.

coderabbitai · 2024-07-15T02:03:20Z

Walkthrough

The changes aim to improve error handling and logging during the addon bundle installation process. The installAddonBundle function in the addonHandler module now captures and stores exceptions more effectively, ensuring proper cleanup and accurate return values. Similarly, the installAddon function in addonGui.py has been refactored for better exception handling, introducing new context managers and improving installation flow robustness.

Changes

File	Change Summary
`source/addonHandler/__init__.py`	Enhanced error handling and logging in `installAddonBundle`, storing exceptions in `bundle._installExceptions`, and ensuring proper cleanup on failure.
`source/addonStore/install.py`	Improved exception handling and control flow in `installAddon`.
`source/gui/addonGui.py`	Refactored `installAddon` into `_performExternalAddonBundleInstall`, added `_doneAndDestroy` context manager, and enhanced exception handling and logging.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant GUI as addonGui
    participant Handler as addonHandler
    participant Store as addonStore

    User->>GUI: Initiates Addon Installation
    GUI->>Handler: Calls installAddonBundle
    Handler->>Handler: Extracts Bundle and Installs Addon
    Handler->>Handler: Catches Exceptions and Logs Errors
    Handler->>Handler: Stores Exceptions in bundle._installExceptions
    Handler-->>GUI: Returns Installed Addon or None

    alt Installation Successful
        GUI->>Store: Proceed with Installation
    else Installation Fails
        GUI->>Store: Handle Cleanup
    end
    GUI-->>User: Provides Installation Result

Assessment against linked issues

Objective (Issue #)	Addressed	Explanation
Prevent removal of previous addon version if `installTasks.onInstall` fails during update (#16704)	✅
Ensure NVDA presents an addon installation error dialog on failure (#16704)	✅

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

seanbudd · 2024-07-15T04:57:02Z

@josephsl - could you test this build? could you also provide a sample add-on to confirm testing with?

josephsl · 2024-07-15T05:26:42Z

Hi,

Tested from the source code copy and things work as expected. To reproduce, use the attached test version of Windows App Essentials build. When the add-on is run with the PR applied, you'll get a message about unsupported Windows release (don't worry about the message itself as we're testing the install failure mechanism).

Thanks.

josephsl · 2024-07-15T05:28:11Z

Be sure to rename from .zip to .nvda-addon extension.

seanbudd · 2024-07-15T05:30:21Z

@josephsl - I think that failed to attach

josephsl · 2024-07-15T05:32:21Z

Hi,

As an alternative, try the following add-on for testing purposes:

https://github.com/josephsl/wintenApps/releases/download/24.06/wintenApps-20240714.0.0.nvda-addon

Thanks.

michaelDCurran · 2024-07-15T06:36:28Z

+	during the installation process.
+
+	:return: The installed add-on object, or None if the installation failed.
+	Regardless if installation failed or not, the created add-on object should be returned


Seems this line contradicts the previous line? I.e. returning None if installation fails verses the addon object should ways be returned even if installation fails.

Safer handling of the initial installation of add-ons

059beaa

seanbudd added this to the 2024.3 milestone Jul 12, 2024

seanbudd marked this pull request as ready for review July 15, 2024 02:03

seanbudd requested a review from a team as a code owner July 15, 2024 02:03

seanbudd requested review from SaschaCowley and removed request for a team July 15, 2024 02:03

seanbudd marked this pull request as draft July 15, 2024 02:03

seanbudd marked this pull request as ready for review July 15, 2024 05:29

seanbudd requested a review from gerald-hartig July 15, 2024 05:30

michaelDCurran reviewed Jul 15, 2024

View reviewed changes

michaelDCurran previously approved these changes Jul 15, 2024

View reviewed changes

seanbudd commented Jul 15, 2024

View reviewed changes

Comment thread source/addonHandler/__init__.py Outdated

Update source/addonHandler/__init__.py

075cfc2

seanbudd dismissed michaelDCurran’s stale review via 075cfc2 July 15, 2024 06:53

seanbudd commented Jul 15, 2024

View reviewed changes

Comment thread source/addonHandler/__init__.py Outdated

seanbudd added 2 commits July 15, 2024 17:06

Update source/addonHandler/__init__.py

5148730

update changes

27ad6a1

seanbudd merged commit 7655f8e into beta Jul 15, 2024

seanbudd deleted the saferAddonPreInstall branch July 15, 2024 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Safer handling of the initial installation of add-ons#16851

Safer handling of the initial installation of add-ons#16851
seanbudd merged 4 commits into
betafrom
saferAddonPreInstall

seanbudd commented Jul 12, 2024 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 15, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

Uh oh!

seanbudd commented Jul 15, 2024

Uh oh!

josephsl commented Jul 15, 2024 •

edited

Loading

Uh oh!

josephsl commented Jul 15, 2024

Uh oh!

seanbudd commented Jul 15, 2024

Uh oh!

josephsl commented Jul 15, 2024

Uh oh!

michaelDCurran Jul 15, 2024

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

seanbudd commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Assessment against linked issues

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

seanbudd commented Jul 15, 2024

Uh oh!

josephsl commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephsl commented Jul 15, 2024

Uh oh!

seanbudd commented Jul 15, 2024

Uh oh!

josephsl commented Jul 15, 2024

Uh oh!

michaelDCurran Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seanbudd commented Jul 12, 2024 •

edited

Loading

coderabbitai Bot commented Jul 15, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

josephsl commented Jul 15, 2024 •

edited

Loading