Skip to content

Fix-up of #13366: Replace dots with underscores before trying to check if the given App Module exists to work around bug in the Python import system#13814

Merged
seanbudd merged 2 commits into
nvaccess:betafrom
lukaszgo1:I13813
Jun 20, 2022
Merged

Conversation

@lukaszgo1

@lukaszgo1 lukaszgo1 commented Jun 19, 2022

Copy link
Copy Markdown
Contributor

Opened against beta since this fixes regression introduced during 2022.2 development cycle.

Link to issue number:

Fix-up of #13366
Fixes #13813

Summary of the issue:

As part of PR #13366 I've modified the code responsible for retrieving application name from process id so that it no longer tries to import an App Module for a hosting process if it does not exist. Unfortunately Python find_Module method seems to have a bug - it returns a module when the provided string contains dots and the file named exactly like the last segment of the provided name exists in the package. For example if you have module named qq in the package, and you would like to check if the module named 6.5.qq exists True is returned even though there is no such module. I haven't yet tested how this behaves with more recent versions of Python nor opened an issue against them.

Description of user facing changes

NVDA no longer fails to work in applications containing dots in their file names in cases where you have an App Module for an application named exactly like the last segment of the application name.

Description of development approach

Dots are normalized to underscores before checking if the given module exists. While we did this already before trying to import a standard App Module this has not been done before trying to get module for a hosting process.

Testing strategy:

  • Performed steps from Regression: Attempt to import wrong appModule #13813
  • Tested that an App Module for AZARDI (it binary file contains dot in the file name) is loaded successfully
  • Tested that the correct name is retrieved for a Java application from its hosting process

Known issues with pull request:

None known

Change log entries:

None needed - unreleased regression.

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

@lukaszgo1
Replace dots with underscores before trying to check if the given App…
9405985 
… Module exists to work around bug in the Python import system
@lukaszgo1 lukaszgo1 requested a review from a team as a code owner June 19, 2022 11:47
@lukaszgo1 lukaszgo1 requested review from seanbudd and removed request for a team June 19, 2022 11:47
@cary-rowen cary-rowen mentioned this pull request Jun 19, 2022
Closed
@cary-rowen

This comment was marked as resolved.

Sign in to view
@lukaszgo1

This comment was marked as off-topic.

Sign in to view
@cary-rowen

This comment was marked as outdated.

Sign in to view
@seanbudd seanbudd added this to the 2022.2 milestone Jun 20, 2022
@seanbudd

seanbudd commented Jun 20, 2022

Copy link
Copy Markdown
Member

@lukaszgo1

From reading Python importlib docs find_module is deprecated, and we should use find_spec instead. As the function is deprecated, I don't think reporting the bug to python is a good use of time.

Can you test using find_spec instead of find_module and see if this is still an issue?

I would test this but I am struggling to understand the Python bug described in the issue a summary. A minimal python sample would be helpful here.

@seanbudd

seanbudd commented Jun 20, 2022

Copy link
Copy Markdown
Member

@lukaszgo1 I'm looking into using find_spec with the STR in #13813

@seanbudd

seanbudd commented Jun 20, 2022

Copy link
Copy Markdown
Member

It appears that find_spec has the same bug

@seanbudd seanbudd merged commit 7e85396 into nvaccess:beta Jun 20, 2022
@seanbudd

seanbudd commented Jun 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

This doesn't appear to be a bug. Note the documentation from pkgutil.iter_importers, which is what we are using as the base for find_module:

Yield finders for the given module name

If fullname contains a '.', the finders will be for the package
containing fullname, otherwise they will be all registered top level
finders (i.e. those on both sys.meta_path and sys.path_hooks).

If the named module is in a package, that package is imported as a side
effect of invoking this function.

If no module name is specified, all top level finders are produced.

@lukaszgo1 lukaszgo1 deleted the I13813 branch June 20, 2022 07:02
@lukaszgo1 lukaszgo1 mentioned this pull request Jun 28, 2022
Merged
5 tasks
seanbudd pushed a commit that referenced this pull request Jun 30, 2022
@lukaszgo1
Use importlib.util.find_spec when checking if the given appModule exi…
527e54c 
…sts to workaround issues in find_module and for forward compatibility. (#13853)

None, discussion in PR #13814

Summary of the issue:
As mentioned in #13814 (comment) FileFinder.find_module is deprecated. find_spec is the recommended replacement method.
As noted in #13814 (comment), using either find_spec or find_module with pkgutil.iter_importers results in paths containing "." being incorrectly treated as python packages, causing #13813

Description of development approach
Description of user facing changes
No user facing change

Description of development approach
When checking if the given appModule exists, instead use importlib.util.find_spec, which does not have the same problematic behaviour as pkgutil.iter_importers.
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2022.2

Development

Successfully merging this pull request may close these issues.

3 participants

@lukaszgo1 @cary-rowen @seanbudd