Skip to content

Logging: Fix some edge cases when identifying if the logged messages comes from external code or not.#14812

Merged
seanbudd merged 6 commits into
nvaccess:masterfrom
lukaszgo1:isPathExternalToNVDA_more_robust
Apr 11, 2023
Merged

Logging: Fix some edge cases when identifying if the logged messages comes from external code or not.#14812
seanbudd merged 6 commits into
nvaccess:masterfrom
lukaszgo1:isPathExternalToNVDA_more_robust

Conversation

@lukaszgo1

Copy link
Copy Markdown
Contributor

Link to issue number:

Discussion in #14806

Summary of the issue:

When logging NVDA tries to determine if the logged message comes from its own code or from an add-on. In some cases it does so incorrectly.

Case 1: Directory prepended to sys.path by an add-on

Some add-ons have to modify sys.path to import additional libraries. NVDA's logHandler relied on the fact that the first entry in path represents the location of NVDA's source code. While add-on developers should really clean-up after themselves i.e. modify path only for as long as necessary to import additional libraries this is not something we can enforce. Initially observed by @CyrilleB79 for an add-on for ChatGPT.

Case 2: Messages logged from add-ons installed in the default config for source copies

By default configuration folder for source copies is placed next to NVDA sources, i.e. in the source folder. When determining if the path belongs to NVDA or not we used to check if the given path starts with NVDA's source folder which was true for the plugins in the user config even though the code was not a part of NVDA.

Description of user facing changes

This should affect only developers who inspect the log.

Description of development approach

When checking if the path is external or not we no longer rely on sys.path - rather the location of the logHandler is used to determine the true location of NVDA's source code. In addition if the path is beneath the current NVDA's config it is always marked as external.

Testing strategy:

  • With add-on for ChatGPT installed ensured that messages logged from NVDA core are not marked as external for both source and binary copies
  • When running from sources ensured that messages logged from plugins installed in the default config folder are marked as coming from external code.

Known issues with pull request:

None known

Change log entries:

For Developers

  • NVDA should more accurately determine if the logged message is coming from its own code

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.

@lukaszgo1 lukaszgo1 requested a review from a team as a code owner April 8, 2023 16:35
@lukaszgo1 lukaszgo1 requested a review from seanbudd April 8, 2023 16:35
Comment thread source/logHandler.py Outdated
Comment thread source/logHandler.py Outdated
@lukaszgo1

Copy link
Copy Markdown
Contributor Author

@seanbudd All done.

@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 6c18157f37

Comment thread source/logHandler.py Outdated
@seanbudd seanbudd merged commit 54fe32c into nvaccess:master Apr 11, 2023
@nvaccessAuto nvaccessAuto added this to the 2023.2 milestone Apr 11, 2023
@lukaszgo1 lukaszgo1 deleted the isPathExternalToNVDA_more_robust branch April 12, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants