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

Allow NVDA to still function if a component changes the current directory #11650

Merged
merged 26 commits into from Sep 29, 2020

Conversation

michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Sep 21, 2020

Link to issue number:

Fixes #6491

Summary of the issue:

When testing the Ivona SAPI5 demo voices for another issue from https://www.harposoftware.com/ I noticed that once loading one of these voices:

  • NVDA could no longer save its configuration
  • Could no longer switch back to eSpeak or OneCore
  • If restarted with this voice, it could no longer speak at all, due to NVDA being unable to load character descriptions / symbol data.

All of this is caused by the fact that this SAPI5 engine very inappropriately changes the current directory to its own directory when the voice is loaded, and never changes it back.
Many parts of NVDA assume that the current directory is set to NVDA's application directory, and therefore all relative paths (and there are many of them) depend on the current directory.

As much as this engine is behaving very badly, we should at very least make sure that NVDA does not become unusable. I.e. config should still be able to be saved, symbols (and therefore speech) should still work, and switching back to another synthesizer should also work.

Description of how this pull request fixes the issue:

  • When NVDA first executes, as well as changing the current directory to the application dir, also save this application dir off to globalVars.appDir so that other parts of NVDA can use it later on.
  • ensure that all paths given as commandline arguments are made absolute as soon as possible, before the current directory has a chance to change to something we did not expect.
  • Replace the majority of relative paths with absolute paths made from globalVars.appDir, rather than just calling os.path.abspath. This includes the config dir, eSpeak paths, nvdaHelper dll paths, gui doc file paths, and more.

It is possible that some paths have been missed, but with these changes and running SAPI5 Ivona it is certainly now possible to:

  • save configuration
  • Switch to eSpeak or Onecore
  • speak with correct symbols after restarting.

Testing performed:

See the three points above.

Known issues with pull request:

None

Change log entry:

  • When using the SAPI5 Ivona voices from harposoftware.com, NvDA is now able to save configuration, switch synthesizers, and no longer will stay silent after restarting.

… made absolute as soon as possible to protect against the current directory changing later on.

Also store NVDA's app dir in globalVars.
@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

@lukaszgo1
Copy link
Contributor

There are indeed some places where NVDA expects current directory to be its install dir which you've missed:

  • in logHandler _getDefaultLogFilePath writes log to the current dir if running from sources.
  • TABLES_DIR in brailletables
  • Path to the html template in ui browseableMessage
  • Most places where we call nvwave.playWaveFile

Please also grep for places in which we call os.getcwd

@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

@AppVeyorBot
Copy link

@AppVeyorBot

This comment has been minimized.

@lukaszgo1
Copy link
Contributor

Two more cases of relative path's are SLAVE_FILENAME in config and path to the lilli.dll in brailleDisplayDrivers. Also this PR fixes issue #6491

@lukaszgo1
Copy link
Contributor

Sorry one more - path to builtin.dic in speechDictHandler is also relative.

source/addonHandler/__init__.py Show resolved Hide resolved
from logHandler import log
from ctypes import windll
from ctypes import *
Copy link
Contributor

Choose a reason for hiding this comment

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

windll is the only part of ctypes used in this file - this line can be removed.

h = ctypes.windll.kernel32.LoadLibraryExW(
os.path.join(globalVars.appDir, "lib", versionInfo.version, "nvdaHelperRemote.dll"),
0,
0x8
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this a named constant in winKernel and use it both here and in NVDAHelper.

@AppVeyorBot

This comment has been minimized.

@AppVeyorBot

This comment has been minimized.

…in NVDAHelper and nvda_slave when loading NvDAHelperRemote.
@AppVeyorBot

This comment has been minimized.

@codeofdusk
Copy link
Contributor

Do you plan to merge this fairly quickly? (i.e. does it make sense to change #11639 so it uses globalVars.appDir, therefore blocking that PR on this one)?

@michaelDCurran
Copy link
Member Author

@codeofdusk This pr may take a little time to be carefully reviewed I think.

@codeofdusk
Copy link
Contributor

OK, I'll leave #11639 as-is (to avoid unnecessary delay) but we should make a note to adjust the nvda_dmp invocation to use globalVars.appDir once this goes in.

feerrenrut
feerrenrut previously approved these changes Sep 29, 2020
Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

looks good to me @michaelDCurran

@josephsl
Copy link
Collaborator

Hi,

I'm afraid this PR introduced startup regression - the resulting alpha build isn't even starting at all, or if it did, instlal screen or UAC (in case of an update) isn't showing.

Thanks.

@michaelDCurran
Copy link
Member Author

michaelDCurran commented Sep 30, 2020 via email

@michaelDCurran
Copy link
Member Author

I have temporarily reset master to the commit before this merge while we work out why this causes an incompatibility with NVDARemote and thus stops NVDA from starting.

@michaelDCurran
Copy link
Member Author

See NVDARemote/NVDARemote#225

@michaelDCurran
Copy link
Member Author

NVDA is stuck in a while loop.
The stack for the main thread is:

Python stack for thread 13648 (MainThread):
  File "source/nvda.pyw", line 236, in <module>
  File "C:\Users\mick\programming\git\nvda\source\core.py", line 483, in main
    globalPluginHandler.initialize()
  File "C:\Users\mick\programming\git\nvda\source\globalPluginHandler.py", line 32, in initialize
    for plugin in listPlugins():
  File "C:\Users\mick\programming\git\nvda\source\globalPluginHandler.py", line 23, in listPlugins
    plugin = importlib.import_module("globalPlugins.%s" % name, package="globalPlugins").GlobalPlugin
  File "C:\Program Files (x86)\Python37-32\lib\importlib\__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "C:\Users\mick\programming\git\nvda\source\userConfig\addons\nvdaRemote\globalPlugins\remoteClient\__init__.py", line 25, in <module>
    addonHandler.initTranslation()
  File "C:\Users\mick\programming\git\nvda\source\addonHandler\__init__.py", line 533, in initTranslation
    addon = getCodeAddon(frameDist=2)
  File "C:\Users\mick\programming\git\nvda\source\addonHandler\__init__.py", line 525, in getCodeAddon
    while curdir not in _getDefaultAddonPaths():
  File "C:\Users\mick\programming\git\nvda\source\addonHandler\__init__.py", line 176, in _getDefaultAddonPaths
    return addon_paths

michaelDCurran added a commit that referenced this pull request Sep 30, 2020
…g them to stop an infinite while loop introduced in #11650
@michaelDCurran
Copy link
Member Author

in addonHandler.getCodeAdon: we walk back up to the root-most directory of an addon, but we need to normalize the paths with os.path.normpath before comparing them. Previously os.path.abspath did this for us.

@michaelDCurran
Copy link
Member Author

Leaving master reset back to the commit before this (I.e. this pr has been reverted). I shall open a new pr containing all the current changes plus the fix.

michaelDCurran added a commit that referenced this pull request Sep 30, 2020
…tory (#11707)

* nvda.pyw: ensure that all paths coming from commandline arguments are made absolute as soon as possible  to protect against the current directory changing later on.
Also store NVDA's app dir in globalVars.

* Use the NVDA app dir rather than the current directory for relative paths.

* Fix unit tests.

* Remove all usage of os.getcwd and replace it with globalVars.appDir

* Replace all remaining os.path.join("* calls with os.path.join(globalVars.appDir calls.

* nvda.pyw: provide an absolute path to gettext.translate

* nvda_slave: set globalVars.appDir, and provide an absolute path to gettext.translate

* getDefaultLogFilePath no longer uses the current directory.

* brailleTables: TABLES_DIR is no longer relative to the current directory.

* ui.browsableMessage no longer uses a relative path to get to the html file.

* Change all playWavefile calls to be non-relative

* Fix linting issues

* another relative wave file path

* Fix linting issues

* speechDictHandler: the path to builtin.dic is no longer relative.

* config: slave_fileName is no longer relative

* Lilli braille driver: path to dll is no longer relative.

* Fix linting issues

* nvda_slave: don't load nvdaRemote with a relative path.

* Remove all usage of os.path.abspath, but add a couple of assertions in places where we can't be completely sure the path is absolute.

* Fix translation comments

* Add the ALTERED_LIBRARY_SEARCH_PATH constant to winKernel and use it in NVDAHelper and nvda_slave when loading NvDAHelperRemote.

* Lili  braille dirver: remove unneeded import.

* Update what's new

* addonHandler.getCodeAddon: make sure to normalize paths when comparing them to stop an infinite while loop  introduced in #11650
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.

NVDA shouldn't rely on the CWD
7 participants