Skip to content

allow importing add-on code from an addons namespace#14405

Closed
LeonarddeR wants to merge 5 commits into
nvaccess:masterfrom
LeonarddeR:addonNamespace
Closed

allow importing add-on code from an addons namespace#14405
LeonarddeR wants to merge 5 commits into
nvaccess:masterfrom
LeonarddeR:addonNamespace

Conversation

@LeonarddeR

@LeonarddeR LeonarddeR commented Nov 26, 2022 •
edited by feerrenrut
Loading

Copy link
Copy Markdown
Collaborator

Cc @josephsl

Link to issue number:

Closes #14359

Summary of the issue:

It is currently only possible to import add-on code from the standard NVDA plugin paths, like appModules or globalPlugins. Therefore, there is no elegant way add-ons can store code that has to be shared between appModules and globalPlugins, for example.

Description of user facing changes

@feerrenrut suggested the following approach in #14359 (comment):

I'd prefer that addon code isn't mixed in with the standard namespaces. Instead, a solution that maintained the separation of the add-on.

When importing shared code from within the addon this should be achieved in a way that avoids namespace collisions: Given an addon called myAddon you might import code from a lib directory within the add-on with from myAddon.lib import comInterfaces

  • This would help to prevent add-ons from inadvertently depending on each other.
  • We don't want to end up in a situation where the addition of a new file in NVDA could break add-ons because of namespace pollution.

The final solution is slightly different in that it adds an addons part in the naming scheme: Given an addon called myAddon you can import code from a lib directory within the add-on with `from addons.myAddon.lib import comInterfaces.

Description of development approach

This solution depends heavily on importlib. It generates a blank module spec based on a module name and optional path, and constructs a module from that spec. The module is inserted in sys.modules. The addons themselves are simulated Native Namespace Packages. Long story short, these are python packages that don't require an __init__.py file.

Testing strategy:

Created an addon called example. In the addon folder, added a folder called lib and in there a module called test.py. Successfully imported the module with from addons.example.lib import test

Known issues with pull request:

None known

Change log entries:

For Developers

  • Add-ons can now contain python code anywhere in their directory structure. The contents of an add-on folder are exposed under the addons namespace. For example, a test module in an add-on called example can be imported with "from addons.example import test". (Add package for addons to store plugin independent code #14359)

Code Review Checklist:

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

@LeonarddeR LeonarddeR requested a review from a team as a code owner November 26, 2022 18:49
@LeonarddeR LeonarddeR requested a review from seanbudd November 26, 2022 18:49
@AppVeyorBot

AppVeyorBot commented Nov 26, 2022

Copy link
Copy Markdown

See test results for failed build of commit d16d3b5292

seanbudd
seanbudd approved these changes Nov 27, 2022
View reviewed changes

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me

Comment thread source/addonHandler/packaging.py Outdated
@seanbudd seanbudd requested a review from feerrenrut November 27, 2022 22:57
@LeonarddeR LeonarddeR changed the title Prototype: allow importing add-on code from an addons namespace allow importing add-on code from an addons namespace Nov 28, 2022
@feerrenrut

feerrenrut commented Nov 30, 2022

Copy link
Copy Markdown
Contributor

@LeonarddeR The implementation so far looks good.

This might be a good candidate for an automated test. It's functionality that would be easy for NVDA core devs to forget about. It would need to be a system test, perhaps two simple add-ons, one that shares the code from the other and does something like announces the "hello world" message sourced from the other add-on.

One other aspect to consider here is managing dependencies between add-ons. This will become a new problem, add-ons that only work when others are installed. Users will need a way to find out what dependencies to install and receive warnings when the dependencies aren't met.

@LeonarddeR

LeonarddeR commented Nov 30, 2022

Copy link
Copy Markdown
Collaborator Author

One other aspect to consider here is managing dependencies between add-ons. This will become a new problem, add-ons that only work when others are installed. Users will need a way to find out what dependencies to install and receive warnings when the dependencies aren't met.

I think that interdependent add-ons are out of scope for this solution. Note that this possibility is already available. If add-on 1 bundles a globalPlugin and add-on 2 bundles a synthDriver, it is possible to do from globalPlugins import addonOnePlugin from add-on two and vise versa. The primary goal with this pr is that an add-on can bundle plugin independent code that can be used in several plugins in the add-on itself.

CyrilleB79
CyrilleB79 reviewed Nov 30, 2022
View reviewed changes
Comment thread devDocs/developerGuide.t2t Outdated
@LeonarddeR @CyrilleB79
Update devDocs/developerGuide.t2t
ead8ff4 
Co-authored-by: Cyrille Bougot <cyrille.bougot2@laposte.net>
@LeonarddeR

LeonarddeR commented Nov 30, 2022

Copy link
Copy Markdown
Collaborator Author

Looking at the system tests library stuff, it all seems a bit hacky to me. There is a special function that constructs a scratchpad from several modules. Why was this made like this? Should we consider making this an add-on instead?

@feerrenrut

feerrenrut commented Dec 2, 2022

Copy link
Copy Markdown
Contributor

The primary goal with this pr is that an add-on can bundle plugin independent code that can be used in several plugins in the add-on itself.

While it was possible before, this PR is introducing interdependence between add-ons as a first-class use-case. Regardless of whether it's in scope of this PR or not, this is a problem that will need to be solved either in the add-ons or through NVDA add-on management. It's a tricky problem, the phrase "dependency hell" exists for a reason. Consider:

  • Addon-A needs version 1 of Addon-base
  • Addon-B needs version 2 of Addon-base

It's very tempting to say this isn't a path we want to go down, add-ons should just duplicate dependencies they need.
Instead, it might be better to look at how the different parts of an add-on (appModules, brailleDisplayDrivers, synthDrivers, globalPlugins and visionEnhancementProviders) can share code internally, but prevent leaking this to other add-ons.
I'd recommend a dependency injection approach:

  • A lib package in the addon contain code to be shared within this add-on.
  • NVDA imports this and then injects it into each plugin type via a receiveLib() function with a default implementation.
  • I'm not sure what the most appropriate thing to inject would be. Can it be the module? If not, a static class might be more appropriate, or perhaps a callable with the result implementation defined (I.E. receiveLib(getLib: Callable[(), Any]))?

There is a special function that constructs a scratchpad from several modules. Why was this made like this? Should we consider making this an add-on instead?

Off topic for this PR, but happy for alternatives to be considered in a new issue/PR. Using the scratchpad approach was taken because it was considered simpler while fundamentally providing the same outcome. This approach means there is no need for an add-on packaging step and no add-on install step. While working on the tests, and potentially changing libraries, the files need to be redeployed to ensure the tests run with the latest version. This happens for every 'start NVDA'.

@LeonarddeR LeonarddeR marked this pull request as draft December 2, 2022 07:24
@LeonarddeR

LeonarddeR commented Dec 2, 2022

Copy link
Copy Markdown
Collaborator Author

It's very tempting to say this isn't a path we want to go down, add-ons should just duplicate dependencies they need. Instead, it might be better to look at how the different parts of an add-on (appModules, brailleDisplayDrivers, synthDrivers, globalPlugins and visionEnhancementProviders) can share code internally, but prevent leaking this to other add-ons. I'd recommend a dependency injection approach:

  • A lib package in the addon contain code to be shared within this add-on.
  • NVDA imports this and then injects it into each plugin type via a receiveLib() function with a default implementation.
  • I'm not sure what the most appropriate thing to inject would be. Can it be the module? If not, a static class might be more appropriate, or perhaps a callable with the result implementation defined (I.E. receiveLib(getLib: Callable[(), Any]))?

Interesting idea. I will research this further. A similar approach is used for bundling installTasks with an add-on.

@LeonarddeR

LeonarddeR commented Dec 2, 2022

Copy link
Copy Markdown
Collaborator Author

I discovered a pretty straight forward method to do this already.
An add-on can call addonHandler.getCodeAddon() and then loadModule on the add-on to get a module from its directory. I think that fully suffices the use case I'm proposing. It makes sense to document it tough

@LeonarddeR LeonarddeR closed this Dec 2, 2022
@LeonarddeR LeonarddeR mentioned this pull request Jan 3, 2023
Merged
6 tasks
seanbudd pushed a commit that referenced this pull request Jan 11, 2023
@LeonarddeR
Swap pkgutil.ImpImporter for importlib when loading custom modules fr…
a232641 
…om add-ons (#14481)

Replaces #14405

Summary of the issue:
Addons have the ability to import custom code from the add-ons directory. This is used to load install tasks, for example.
This mechanism was based on a deprecated solution in pkgutil and allowed importing modules using backslashes, resulting in malformed module identifiers in sys.modules.

Description of user facing changes
Importing submodules with loadModule should be done with a dot separated name (i.e. loadModule("lib.example") instead of loadModule("lib\\example")
loadModule now raises an exception when a module can't be found
Description of development approach
We no longer rely on pkgutil.ImpImporter.

Testing strategy:
Tested importing a lib module from an example addon as well as modules without init.py and submodules. Also tested modules with malformed content, resulting in tracebacks.

Known issues with pull request:
This would break cases where add-ons would use loadModule with a module name containing backslashes to import submodules. However, this scenario was broken anyway since it added malformed names to sys.modules. Note that this is strictly API breaking but I don't think noone was using it anyway.
As loadModule now raises exceptions, add-on authors need to account for that when using this API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@feerrenrut feerrenrut Awaiting requested review from feerrenrut

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

+1 more reviewer

@CyrilleB79 CyrilleB79 CyrilleB79 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@feerrenrut feerrenrut

@seanbudd seanbudd

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add package for addons to store plugin independent code

5 participants

@LeonarddeR @AppVeyorBot @feerrenrut @seanbudd @CyrilleB79