recreate or abort if Python virtual environment has been moved from where it was originally located #12171

michaelDCurran · 2021-03-14T23:47:14Z

Link to issue number:

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. This is a limitation in venv, and there really is nothing that NvDA can do to work around this.
However, NvDA should at least offer to recreate the virtual environment at the new location if it has been moved or copied.
As currently, executing runnvda.bat or other scripts while in this state cause hard to understand errors such as NVDA thinking that there is no virtual environment at all.

Description of how this pull request fixes the issue:

ensureVenv.py detects if the Python virtual environment has been moved by temporarily activating it, and capturing the content of the VIRTUAL_ENV environment variable, and then comparing that against the physical venv path.
If they differ, the user is asked if the virtual environment should be recreated. Saying no causes the script to abort.

Testing strategy:

Executed runnvda.bat from a non-moved location, confirming that ensureVenv runs without errors and NVDA starts.
Renamed the NVDA repository directory, and performed the above step again. This time confirmed that the script asked if the virtual environment should be recreated. Saying no aborted. Running again and saying yes recreated the environment and then ran NVDA.

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.

…re they were created in their activation script, dedetect if the venv has been copied to another location and if so ask the user if the environment should be recreated at the new location or to abort.

michaelDCurran · 2021-03-14T23:49:30Z

@CyrilleB79 could you please test this branch and confirm this detects your situation and allows you to recreate the environment or abort?

…ted if it has been moved from its original place of creation.

lukaszgo1 · 2021-03-14T23:53:47Z

venvUtils/ensureVenv.py

+	Verifies that the Python virtual environment at c{VENV_PATH} was actually created at that location,
+	and not just copied or moved there.
+	"""
+	# Activate the Python virtual environement and capture the content of the VIRTUAL_ENV environment variable.


environement -> environment

lukaszgo1 · 2021-03-14T23:54:41Z

venvUtils/ensureVenv.py

@@ -101,6 +124,16 @@ def ensureVenvAndRequirements():
 	if not os.path.exists(venv_path):
 		print("Virtual environment does not exist.")
 		return createVenvAndPopulate()
+	if not verifyExistingVenvPath():
+		if askYesNoQuestion(
+			"A Python virtual environement cannot be activated from a different location "


lukaszgo1 · 2021-03-14T23:57:35Z

venvUtils/ensureVenv.py

+			"call", "echo", "%VIRTUAL_ENV%",
+		],
+		shell=True,
+	).decode('utf8').rstrip()


Windows console isn't Unicode by default. It'll probably work fine as long as there is no characters outside of the ASCI range in the path but to be safe this should be decoded using oem

AppVeyorBot · 2021-03-15T00:07:36Z

System test failure
Build (for testing PR)

See test results for failed build of commit 865ccce493

AppVeyorBot · 2021-03-15T00:44:58Z

System test failure
Build (for testing PR)

See test results for failed build of commit 24b37dc980

feerrenrut

Thanks @michaelDCurran, this approach looks good.

feerrenrut · 2021-03-15T09:35:40Z

venvUtils/ensureVenv.py

+	existingPath = subprocess.check_output(
+		[
+			os.path.join(venv_path, "scripts", "activate.bat"),
+			"&&",
+			"call", "echo", "%VIRTUAL_ENV%",
+		],
+		shell=True,
+	).decode('oem').rstrip()


Please could you add a comment here to show an example of the output for this command? If the output changes, this will make it easier to verify.

CyrilleB79 · 2021-03-15T11:48:10Z

Hi
I have tested this branch and confirm that the location change has been detected and I am asked if I want to re-generate the environment. Thus the error I get is clearer and it allows me to understand the issue; also the dev doc gives information for this. Thanks.

Unfortunately for me, it does not solve the situation that led me to open #12166.
Before #12075, it was possible to run NVDA from source on a computer that does not have Visual Studio installed: for this, I had to compile NVDA source code on PC1 (with Visual studio installed), copy the source code tree to PC2 (without Visual Studio installed) and run NVDA from source on PC2.
Now this does not seem to be possible anymore.
If this impossibility is unfortunately confirmed and that nothing can be done for this, please at least indicate this new limitation in the known issues of this PR since this only partly solves #12166.

lukaszgo1 · 2021-03-15T11:55:31Z

Unfortunately for me, it does not solve the situation that led me to open #12166.
Before #12075, it was possible to run NVDA from source on a computer that does not have Visual Studio installed: for this, I had to compile NVDA source code on PC1 (with Visual studio installed), copy the source code tree to PC2 (without Visual Studio installed) and run NVDA from source on PC2.
Now this does not seem to be possible anymore.
If this impossibility is unfortunately confirmed and that nothing can be done for this, please at least indicate this new limitation in the known issues of this PR since this only partly solves #12166.

Why it is impossible i.e what happens if you try?

michaelDCurran · 2021-03-15T23:44:13Z

We could consider either providing our own version of activate.bat (it does not look that complicated) which uses relative paths. Or, we could programmatically rewrite activate.bat after it is generated, changing the absolute paths to relative paths. I think I prefer the first option.

CyrilleB79 · 2021-03-16T09:35:23Z

In activate.bat, I have tried to change manually the path stored in VIRTUAL_ENV variable. This should work if the folder is just moved/renamed on the same computer.
This would already be something good to implement.

However, when trying to copy the source tree to another computer and execute runnvda.bat, Python complained that it did not find the pythonw.exe.
That was because Python was not installed at the same place on the second computer.

After having manually modified Python location in pyvenv.cfg, I have finally succeeded in executing runnvda.bat on the second computer.
If something can be done to find the Python 3.8-32 location and update the pyvenv.cfg file, this would be very nice.

If not, modifying the activate.bat file is already a good thing.

Thanks!

AppVeyorBot · 2021-03-16T10:58:51Z

System test failure
Build (for testing PR)

See test results for failed build of commit c67e4b2a8a

michaelDCurran · 2021-03-16T23:05:39Z

Closing in favor of #12184

ensureVenv.py: As Python virtual environements hard-code the path whe…

a500178

…re they were created in their activation script, dedetect if the venv has been copied to another location and if so ask the user if the environment should be recreated at the new location or to abort.

michaelDCurran requested a review from feerrenrut March 14, 2021 23:47

buildSystemNotes: mention that the virtual environment will be recrea…

01316fb

…ted if it has been moved from its original place of creation.

lukaszgo1 suggested changes Mar 14, 2021

View reviewed changes

Address review actions.

8e26286

feerrenrut suggested changes Mar 15, 2021

View reviewed changes

michaelDCurran mentioned this pull request Mar 16, 2021

Allow NVDA build system to still function if NvDA repository is moved or copied to another location #12184

Merged

7 tasks

michaelDCurran closed this Mar 16, 2021

recreate or abort if Python virtual environment has been moved from where it was originally located #12171

recreate or abort if Python virtual environment has been moved from where it was originally located #12171

michaelDCurran commented Mar 14, 2021 •

edited

michaelDCurran commented Mar 14, 2021

lukaszgo1 Mar 14, 2021

lukaszgo1 Mar 14, 2021

lukaszgo1 Mar 14, 2021

AppVeyorBot commented Mar 15, 2021

AppVeyorBot commented Mar 15, 2021

feerrenrut left a comment

feerrenrut Mar 15, 2021

CyrilleB79 commented Mar 15, 2021

lukaszgo1 commented Mar 15, 2021

michaelDCurran commented Mar 15, 2021 via email

CyrilleB79 commented Mar 16, 2021

AppVeyorBot commented Mar 16, 2021

michaelDCurran commented Mar 16, 2021

recreate or abort if Python virtual environment has been moved from where it was originally located #12171

recreate or abort if Python virtual environment has been moved from where it was originally located #12171

Conversation

michaelDCurran commented Mar 14, 2021 • edited

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entry:

Code Review Checklist:

michaelDCurran commented Mar 14, 2021

lukaszgo1 Mar 14, 2021

Choose a reason for hiding this comment

lukaszgo1 Mar 14, 2021

Choose a reason for hiding this comment

lukaszgo1 Mar 14, 2021

Choose a reason for hiding this comment

AppVeyorBot commented Mar 15, 2021

AppVeyorBot commented Mar 15, 2021

feerrenrut left a comment

Choose a reason for hiding this comment

feerrenrut Mar 15, 2021

Choose a reason for hiding this comment

CyrilleB79 commented Mar 15, 2021

lukaszgo1 commented Mar 15, 2021

michaelDCurran commented Mar 15, 2021 via email

CyrilleB79 commented Mar 16, 2021

AppVeyorBot commented Mar 16, 2021

michaelDCurran commented Mar 16, 2021

michaelDCurran commented Mar 14, 2021 •

edited