Allow NVDA build system to still function if NvDA repository is moved or copied to another location#12184
Conversation
…ivate.bat from the generated .venv, set the necessary environment variables manually, and make VIRTUAL_ENV be relative to this script, rather than a hard-coded path. This allows the NVDA repository to be moved or copied somewhere else on the same system and the environment used without having to be recreated.
…activate.bat so deactivate.bat may do the wrong thing. But more importantly, we already call endlocal which dumps all changes to the environment anyway, thus it is not needed.
| rem Ensure the environment is created and up to date | ||
| py -3.8-32 "%~dp0\ensureVenv.py" | ||
| if ERRORLEVEL 1 goto :EOF | ||
| call "%~dp0\..\.venv\scripts\activate.bat" |
There was a problem hiding this comment.
It would be good to have comments that delineate the following set of steps as having been adapted from the activate.bat script. This will help us to understand their origin and how to update them if they stop working (eg the requirements of the activate.bat script change).
| echo call %* | ||
| call %* | ||
| echo Deactivating NVDA Python virtual environment | ||
| call "%~dp0\..\.venv\scripts\deactivate.bat" |
There was a problem hiding this comment.
What does deactivate.bat do? shouldn't those environment variables be cleaned up in some way?
There was a problem hiding this comment.
deactivate.bat sets particular environment variables back to their old values.
However, as we called setlocal before ensureAndActivate.bat, all the environment variables will go back to their original values once either endlocal is called, the script ends, or the script is aborted with control+c.
There was a problem hiding this comment.
Oh ok. Might be worth a comment to point that out. Otherwise this all looks good.
|
I have tested this PR's branch in the following situations:
|
|
@feerrenrut I think I have added all the commenting you requested now. |
See test results for failed build of commit a8ae841ae1 |
…standard activate.bat.
d5a3ef2 to
9cbca82
Compare
feerrenrut
left a comment
There was a problem hiding this comment.
Looks good, thanks @michaelDCurran
Link to issue number:
Replaces #12171
Closes #12166
Summary of the issue:
When creating a Python virtual environment, Python's venv package hard-codes the absolute path where the environment was created, in its activation script. This therefore means that it is impossible to use a Python virtual environment if it has been copied or moved to a different location.
Description of how this pull request fixes the issue:
Rather than relying on the generated venv's standard activation script, our ensureAndActivate script now sets the needed environment variables (such as VIRTUAL_ENV, PATH etc) manually, and ensures the path to the venv is relative to our script.
this allows the NVDA repository to be renamed or moved, and the virtual environment can still successfully be activated.
However, if you want to run the build system or NvDA from source on another machine via a shared network drive, Python must be installed at the same location on both systems. there is no workaround for this.
Testing strategy:
Ran the NVDA build system commands such as scons, runnvda, runlint successfully.
Renamed the NVDA repository and performed the above test again.
Known issues with pull request:
None known.
Change log entry:
None needed.
Code Review Checklist:
This checklist is a reminder of things commonly forgotten in a new PR.
Authors, please do a self-review and confirm you have considered the following items.
Mark items you have considered by checking them.
You can do this when editing the Pull request description with an x:
[ ]becomes[x].You can also check the checkboxes after the PR is created.