Swap pkgutil.ImpImporter for importlib when loading custom modules from add-ons

[LeonarddeR]

Link to issue number:

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:

1. 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.
2. As loadModule now raises exceptions, add-on authors need to account for that when using this API.

Change log entries:

For Developers

• Add-ons relying on addonHandler.getCodeAddon().loadModule to load custom add-on modules should now import submodules using dot instead of backslash as a separator. So if your addons folder contains a lib containing example.py, loadModule should be called with "lib.example" instead of "lib\example"
  API breaking changes:
• addonHandler.Addon.loadModule now raises an exception when a module can't be loaded or has errors, instead of silently returning None without giving information about the cause.

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.

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/y1sp4b5pemohif8b/artifacts/output/nvda_snapshot_pr14481-27381,ae6b79c4.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.0,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 27.2,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 23.0,
  FINISH_END 0.1

See test results for failed build of commit ae6b79c403

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/x4tm1304jpb8540w/artifacts/output/nvda_snapshot_pr14481-27382,2b2f3ab8.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.9,
  INSTALL_END 0.8,
  BUILD_START 0.0,
  BUILD_END 24.5,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 22.3,
  FINISH_END 0.1

See test results for failed build of commit 2b2f3ab89b

[AppVeyorBot]

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/arndt9mbljbyvp87/artifacts/output/nvda_snapshot_pr14481-27400,bd51e60a.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.9,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 27.3,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 22.9,
  FINISH_END 0.1

See test results for failed build of commit bd51e60a95

[LeonarddeR]

I just marked this ready for review. It replaces #14405. Both implementations have their stronger and weaker sides, but for an IDE, you need to trick the system anyway. Current code in a brailleDisplayDriver I have in concept now looks as follows:

if typing.TYPE_CHECKING:
	from ..lib import driver
	from ..lib import protocol
else:
	addon: addonHandler.Addon = addonHandler.getCodeAddon()
	driver = addon.loadModule("lib.driver")
	protocol = addon.loadModule("lib.protocol")

[LeonarddeR]

@seanbudd I decided to go the exception route. This ensures that loadModule behaves very similar to importlib.import_module i nterms of raising exceptions. Note that importlib.import_module is used under the hood anyway. While it is API breaking, I don't think it has major impact

[AppVeyorBot]

• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/y0bunn2t7gj9w5t4/artifacts/output/
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.8,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  FINISH_END 19.9

See test results for failed build of commit 4adfbfbfaa

[seanbudd]

considering this relies on an underscored part of python, can we expect stability here when updating python? It might be safer to use a custom message here.

[LeonarddeR]

Good point. I changed this into an f-string.

[seanbudd]

Hi @LeonarddeR , I'm going to include both change log items under "API breaking changes". This is because the first item requires add-on authors to change their usage of the code.