Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Apr 11, 2023

Conversation

lukaszgo1
Copy link
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
@lukaszgo1
Copy link
Contributor Author

@seanbudd All done.

@AppVeyorBot
Copy link

See test results for failed build of commit 6c18157f37

@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