Fix up of: remove pyd file in installation folder (#10070)

[lukaszgo1]

Link to issue number:

Discussion in #10070

Summary of the issue:

When updating NVDA from Python 2 based version to the Python 3 one old .pyd, pyc. .pyo and .dll files are still present in the program folder.
This is causing issues such as #9960 and #10063. There are also old dlls from Python, BRLApi and some braille display drivers.
#10070 and #9961 tried to fix this, however in both cases old compiled Python files are removed from userConfig in case of portable copies.

Description of how this pull request fixes the issue:

1. Files with the following extensions are removed from all folders inside NVDA directory, except systemConfig and userConfig:

• .pyc
• .pyo
• .pyd
• .dll
• .manifest

2. The fix for Error loading NVDA Slave on NVDA Restart #4235 introduced in 67a0908 is no longer needed, as all dlls are removed during update.

Testing performed:

Created launcher with these changes, and updated portable version of Threshold with it. ensured that old files from pyWin32 compiled .pyc and .pyo files and no longer used dll's are gone.
Also tested that files with these extensions aren't removed from systemConfig and userConfig.

Known issues with pull request:

1. At the moment I have excluded lib directories from removeOldProgramFiles, because they are cleaned as part of removeOldLibFiles. After reading Installer: try to clean up old library files at the end of installation with out affecting the rest of a successful install #7649 I believe that it should remain that way for the time being - I don't think we want installations problem at this stage.
2. I've decided not to remove anything from system and user config. The approach taken in remove pyd file in installation folder #10070 removed all compiled Python files except the .pyd files for the current Python release in use, however this wasn't taking into account portable versions. If someone has strong objections against it we can always reconsider it later, however I don't think we should make it harder for add-on authors to provide compatibility for both Python 2 and 3.
   It also makes it impossible to use one config for a portable Python 3 and installed Python 2 version, because some appModules and add-ons are provided only as .pyc and .pyo files. I understand that it isn't good development practice, but we shouldn't punish users for developers mistakes.

Change log entry:

None needed.

[LeonarddeR]

For systemConfig, I still consider it desirable to remove old, incompatible files wit the *.pyd extension.

[lukaszgo1]

The only situation in which having .pyd files there may be problematic is the case in which someone distributes an add-on compatible with both Python 2 and 3 and loads improper version of the file. This is very unlikely, as it would be noticeable during normal NVDA usage. Even if someone has old add-on compatible only with Python 2 it wouldn't be loaded due to lack of compatibility values in the manifest.

[LeonarddeR]

AH yes, I didn't tink about that.

[Brian1Gaff]

Could any of this be behind my issue with no auto updates of alpha snaps on a portable install? I did post a python Console dump on the dev list yesterday. Brian bglists@blueyonder.co.uk Sent via blueyonder. Please address personal E-mail to:- briang1@blueyonder.co.uk, putting 'Brian Gaff' in the display name field. Newsgroup monitored: alt.comp.blind-users

[JulienCochuyt]

Of course, I agree with you we should not touch .pyd files shipped by add-ons.

t also makes it impossible to use one config for a portable Python 3 and installed Python 2 version, because some appModules and add-ons are provided only as .pyc and .pyo files. I understand that it isn't good development practice, but we shouldn't punish users for developers mistakes.

I'm not sure sharing a configuration between Python 2 and 3 could ever be advised:

• The schema version has moved from 2 to 3 between release-2019.2 and master.
• If you use with Python 2 a configuration that was used previously by Python 3, all manually disabled add-ons are activated again. I guess it has something to do with the pickle data file format, but I did not check further.

Regarding .pyc/.pyo files shipped with add-ons, we can IMHO handle them automatically with still quite some safety:

• If an add-on provides only .pyc/.pyo files, it cannot be compatible with both Python 2 and 3, and it is its responsibility to state the proper supported NVDA version in its manifest.
• If, for any compiled .pyc/.pyo file, we find the corresponding source .py file, we should remove the .pyc/.pyo file to instruct the interpreter to recompile it.

What do you think? Does it make sense?

[LeonarddeR]

* The schema version has moved from 2 to 3 between release-2019.2 and master.

I don't think so, unless I've really missed something here.

* If you use with Python 2 a configuration that was used previously by Python 3, all manually disabled add-ons are activated again. I guess it has something to do with the pickle data file format, but I did not check further.

This is true, pickle in python 3 uses version 3 by default, instead of pickle version 2. There is also version 4. We could specify the version at pickling time though, so there is a way to avoid incompatibility. In fact, I had to do this for NVDA Vocalizer Expressive.

Regarding .pyc/.pyo files shipped with add-ons, we can IMHO handle them automatically with still quite some safety:

* If an add-on provides only .pyc/.pyo files, it cannot be compatible with both Python 2 and 3, and it is its responsibility to state the proper supported NVDA version in its manifest.

I've discussed this with @michaelDCurran and @jcsteh in the past. Providing only pyc and pyo in an add-on tends to violate the GPL, as it does not bundle the source of the add-on. Strictly spoken I believe that it is not a violation of the GPL if the author is willing to share the source, but it is considered suspicious if no *.py files are provided. I"m also pretty sure that addonHandler.getCodeAddon will fail in those add-ons. Therefore, I think that this case can or should be neglected.

* If, for any compiled .pyc/.pyo file, we find the corresponding source .py file, we should remove the .pyc/.pyo file to instruct the interpreter to recompile it.

When should we do this?

[JulienCochuyt]

• The schema version has moved from 2 to 3 between release-2019.2 and master.

I don't think so, unless I've really missed something here.

Cf. commit 216e476 on 2019-08-02 for PR #9771
This change of latestSchemaVersion from 2 to 3 is maybe a small impact though, I did not check.

We could specify the version at pickling time though, so there is a way to avoid incompatibility.

Good point. I guess we should do that.
This would ensure manual deactivation state is preserved when updating.

• If, for any compiled .pyc/.pyo file, we find the corresponding source .py file, we should remove the .pyc/.pyo file to instruct the interpreter to recompile it.

When should we do this?

Maybe when updating the configuration from schema version 2 to schema version 3?

This strategy, along with pickle format 2, would ensure 2019.3 can properly use a configuration from an earlier versions.
Nevertheless, I guess there is no safe way to allow 2019.2 to use back a configuration already migrated to 2019.3. This is probably a fair limitation though, as long as we properly warn users.

[LeonarddeR]

Cf. commit 216e476 on 2019-08-02 for PR #9771
This change of latestSchemaVersion from 2 to 3 is maybe a small impact though, I did not check.

Good catch, I missed this one.

We could specify the version at pickling time though, so there is a way to avoid incompatibility.

Good point. I guess we should do that.
This would ensure manual deactivation state is preserved when updating.

I assume you meant downgrading? Note that python doesn't allow you to specify the pickle version at load time, it will automatically detect the appropriate protocol version and loads version 2 pickled data just fine. However, when repickling, it will save as protocol 3 instead. I think this makes sense.

having said, I just tried to depickle python 2 data on Python 3 version of NVDA, and it failed. This probably has to do with line ending format incompatibility. I will look into this.

[XLTechie]

Leonard de Ruijter wrote:

I've discussed this with @michaelDCurran and @jcsteh in the past. Providing only pyc and pyo in an add-on tends to violate the GPL, as it does not bundle

With respect, you can't "tend to violate" a legal agreement--either you violate it, or you don't.

the source of the add-on. Strictly spoken I believe that it is not a violation of the GPL if the author is willing to share the source, but it is considered

The "distributor", not just the author, has the duty to share, or make available, the source to anyone who has the binary, if the distributor had the source in the first place. I suspect a U.S. court would probably interpret that to mean NV Access, under these circumstances, if the add-on is distributed via nvda-project.org. In that case, assuming the author did provide a source code request mechanism (an "offer to distribute"), then NV Access would fall under section 3(C) of GPL V2, and no violation will have occurred at all. Let us not forget, that if the author himself violates the GPL, which I'm not saying this is, then there is no enforcement authority. He won't sue himself for failing to distribute source in exactly the correct way. The license exists to protect (limitedly) the rights of the author, not to enable people to go after the author for doing things against his own license. suspicious if no *.py files are provided. Suspicious to whom? It is done all the time, with no suspicion, by Microsoft, Google, and countless other big names--just read the "About" section for any major company's mobile apps, to find plenty examples of binaries distributed without source, and an offer to provide the source. In many of those cases (iOS, for example) it would not even be feasible to distribute source with the components in question, because a well behaved user would have no means of accessing it. I agree of course that this is a bad idea for add-ons, and am not defending the practice. It doesn't even usually solve the problem that add-on devs think it does. But saying it violates, or tends to violate but not strictly, the GPL, seems like a harsh and apparently inaccurate characterization; and a misuse of the GPL to support the otherwise good argument not to do this.

[JulienCochuyt]

I assume you meant downgrading?

Sure, my bad, wrote too quickly.

Note that python doesn't allow you to specify the pickle version at load time, it will automatically detect the appropriate protocol version and loads version 2 pickled data just fine. However, when repickling, it will save as protocol 3 instead. I think this makes sense.

It does.

having said, I just tried to depickle python 2 data on Python 3 version of NVDA, and it failed. This probably has to do with line ending format incompatibility. I will look into this.

I do not reproduce.
I can correctly depickle 2019.2 add-on states using 2019.3

I also in the console quickly pickled with Python 3 using protocol 2 and cPickle in Python 2 correctly depickled the file.

Nevertheless, testing live branch accessolutions/nvda/i7105-pickleFormatVersion2 (based on master) currently fails.
The add-on state as produced by this branch is still not loaded by 2019.2, but I have not yet tracked down what exactly fails.

[LeonarddeR]

@XLTechie: Thanks for pointing this out. I'm clearly not as known to the GPL guidelines as you are.

With suspicious, I mean that usually, an NVDA add-on provides *.py files. If not, the distributor has to compile them and explicitly remove the *.py files before bundling the add-on. If the add-on has to be open source according to the GPL, why would an add-on distributor put effort in removing the source from the bundle? In my opinion, this would be bad practise within the NVDA community.

[JulienCochuyt]

The add-on state as produced by this branch is still not loaded by 2019.2, but I have not yet tracked down what exactly fails.

Found. The culprit is the fact that 2019.2 opens the file as text instead of binary mode.
Just tested with protocol 0, which is text based, and for this very case it works perfectly.

[LeonarddeR]

for Vocalizer Expressive, we had to do something like this for unpickling:

	with open(path, "rb") as f:
		try:
			data = pickle.load(f)
		except pickle.UnpicklingError:
			log.warning(f"Couldn't automatically unpickle {path!r}, trying manual method")
			raw = f.read().replace(b"\r\n", b"\n")
			data = pickle.loads(raw)
		return data

It looks like on python 2, the pickle module uses windows style line endings (crlf) where on Python 3, unix style line endings (lf) are expected.

[JulienCochuyt]

It looks like on python 2, the pickle module uses windows style line endings (crlf) where on Python 3, unix style line endings (lf) are expected.

The main matter is that our legacy Python 2 code opens pickle file with mode "r" and not "rb".
Anyway, protocol 0 works just fine: If written in mode "wb" by Python 3, it can be loaded by Python 2 in mode "r".
If you don't mind, we'll probably avoid extra noise in this ticket and continue as needed in a dedicated one though. I'll file a PR for this sole subject in the next minutes.

[lukaszgo1]

• If, for any compiled .pyc/.pyo file, we find the corresponding source .py file, we should remove the .pyc/.pyo file to instruct the interpreter to recompile it.

When should we do this?

The more important question in my opinion should be why should we want to do this?
I don't consider myself to be an expert in this matter but as far as I know .pyo files are no longer used since Python 3.5, so there is no risk of them being ever used by Python 3 version of NVDA. In case of .pyc files are there any cases in which they wouldn't be recompiled by the approrpriate version of the Python interpreter during NVDA load? I'm not opposed to the general idea of removing them when sources are available - in fact I've considered similar solution before writing this pr, I just don't see real need for doing this and complicating code without a god justification.

[JulienCochuyt]

In case of .pyc files are there any cases in which they wouldn't be recompiled by the approrpriate version of the Python interpreter during NVDA load?

This is a very good question. Thanks for pointing me at checking this further.
I faced a few Issues with .pyc files in other projects, but they actually arise when messing with timestamps, or when using different versions of Python 2.x or different "bitness" (32 vs 64), not when moving from 2 to 3 both 32 bits!
Python 2.x stores compiled bytecode in a .pyc file with the same name and directory as the source .py file.
Python 3, however, stores it in the __pycache__ subdirectory and prepend the .pyc extension with the name of the interpreter (here: cpython-37)
Thus, .pyc files from 2.7 and 3.7 (both 32 bits) can both exist without disturbing each other.

When the .py files are missing and only the .pyc files generated by Python 2 are provided, Python 3 fails to load them, but then the add-on is simply not compatible.

In short: All apologies for the disturbance. .pyc files are no problem in the present context.

[XLTechie]

Leonard de Ruijter wrote:

With suspicious, I mean that usually, an NVDA add-on provides *.py files. If not, the distributor has to compile them and explicitly remove the *.py files before bundling the add-on. If the add-on has to be open source according to the GPL, why would an add-on distributor put effort in removing the source from the bundle? In my opinion, this would be bad practise within the NVDA community.

Oh, I absolutely agree that it is bad practice. My understanding is that the ones who do this, generally do it to hide access keys, oauth tokens, and such. Very poor security. @josephsl, am I correct about that?

[michaelDCurran]

I am a bit lost as to the state of this pr. Are changes required. Is this the right approach? Can someone Summarize for me?

[LeonarddeR]

There is still a bit of discussion about whether or not to remove pyd files in general. Currently, systemConfig and userConfig aren't touched, but I'd still prefer incompatible pyd files to be dropped from at least the systemConfig, that is, pyd files without a python specific extension prefix.

[lukaszgo1]

@LeonarddeR wrote:

There is still a bit of discussion about whether or not to remove pyd files in general. Currently, systemConfig and userConfig aren't touched, but I'd still prefer incompatible pyd files to be dropped from at least the systemConfig, that is, pyd files without a python specific extension prefix.

I've explained above, and was under the impression that you've agreed that add-on bundling .pyd files and present in the system config would be problematic if and only if it would load improper version of the file. This would be noticeable during normal NVDA run, so this is very unlikely.

[LeonarddeR]

@LeonarddeR wrote:

There is still a bit of discussion about whether or not to remove pyd files in general. Currently, systemConfig and userConfig aren't touched, but I'd still prefer incompatible pyd files to be dropped from at least the systemConfig, that is, pyd files without a python specific extension prefix.

I've explained above, and was under the impression that you've agreed that add-on bundling .pyd files and present in the system config would be problematic if and only if it would load improper version of the file. This would be noticeable during normal NVDA run, so this is very unlikely.

I'm sorry for not expressing myself clearly there.

If I understand correctly how python works, current behavior is as follows (please correct me if I"m wrong):

1. When executing import testFile, Python would be able to import both testFile.cp37-win32.pyd and testFile.pyd
2. An add-on compatible with both python 2 and 3 would bundle testFile.cp37-win32.pyd for Python 3.7 and testFile.pyd for Python 2.7
3. Upgrading NVDA to Python 3.8 would result in trying to import testFile.cp38-win32.pyd, and as that doesn't exist, it falls back to testFile.pyd, which exists, but for an incompatible version of Python.

My point is that, how unlikely it may be, we should avoid this from happening.

[JulienCochuyt]

3. Upgrading NVDA to Python 3.8 would result in trying to import testFile.cp38-win32.pyd, and as that doesn't exist, it falls back to testFile.pyd, which exists, but for an incompatible version of Python.

My point is that, how unlikely it may be, we should avoid this from happening.

I assume migrating to Python 3.8 would lead to an update of the NVDA API version.
Isn't it the responsibility of the add-on author not to lie in its manifest about which is the latest supported NVDA API version?
Sure, we've all seen misbehaving add-on authors in that matter, but I guess we only have two alternatives to consider here:

• Either try to help an add-on with a lying manifest to have a greater chance to execute.
• Or allow well constructed add-ons to supply as many .pyd files versions as needed.

EDIT: By the way, I thought systemConfig\addons was not considered at all at runtime. Did I miss something? Or maybe are we closer than I thought to see #6822 implemented?

[lukaszgo1]

Also at least add-ons which I have seen aren't importing just test directly but doing something like this (code sample taken from @CyrilleB79 char info add-on):

if sys.version_info.major == 2: #Python2
	addonPath = os.path.dirname(__file__).decode("mbcs")
	sys.path.append(os.path.join(addonPath, "libPy2"))
else: #Python3
	addonPath = os.path.dirname(__file__)
	sys.path.append(os.path.join(addonPath, "libPy3"))
import unicodedata2 as unicodedata
del sys.path[-1]

[LeonarddeR]

Python 3.8 doesn't necessarily mean that we need an increase of the API version, as long as there aren't breaking changes.My personal opinion still is that we should avoid bundling pyd files with add-ons when possible.

[JulienCochuyt]

Python 3.8 doesn't necessarily mean that we need an increase of the API
version, as long as there aren't breaking changes.My personal opinion
still is that we should avoid bundling pyd files with add-ons when possible.

I'd argue "when possible" is the key here.
Not increasing the API for Python 3.8 would mean to strictly disallow add-ons from being compatible with Python 2.7, 3.7 and 3.8, if they provide .pyd files, even if they are perfectly well constructed, as long as your previous statement is verified:

Upgrading NVDA to Python 3.8 would result in trying to import testFile.cp38-win32.pyd, and as that doesn't exist, it falls back to testFile.pyd, which exists, but for an incompatible version of Python

I actually do not know if this statement is correct, but IMHO we'd need to check this out before deciding whether to increase or not the API version.

Anyway, what about systemConfig\addons? If, as I presume, no add-on is ever loaded from there, and if we stick on the track of allowing to share the same userConfig between 2019.2.1 and 2019.3 - which motivated acceptance of PR #10255 - I guess there is no point in further questioning the fate of add-ons for the present PR.

[lukaszgo1]

@JulienCochuyt wrote:

Anyway, what about systemConfig\addons? If, as I presume, no add-on is ever loaded from there, and if we stick on the track of allowing to share the same userConfig between 2019.2.1 and 2019.3 - which motivated acceptance of PR #10255 - I guess there is no point in further questioning the fate of add-ons for the present PR.

Your assumption is incorrect. Add-ons are loaded from systemConfig when on secure screens as long as their manifest states that they are compatible with currently used NVDA version.
Changing this is in my view out of the question, as we cannot possibly predict all use cases for add-ons on secure screens.

[JulienCochuyt]

@lukaszgo1 wrote:
Your assumption is incorrect. Add-ons are loaded from systemConfig when on secure screens as long as their manifest states that they are compatible with currently used NVDA version.

Thank you for correcting me on this.

Changing this is in my view out of the question, as we cannot possibly predict all use cases for add-ons on secure screens.

I agree.

[LeonarddeR]

Also at least add-ons which I have seen aren't importing just test directly but doing something like this (code sample taken from @CyrilleB79 char info add-on):

This add-on wouldn't be a risk, sure. I'm therefore not worried about the well constructed add-ons, more about the edge cases. Running an add-on like that on a user profile isn't still that much of a problem, I'm just worried about unexpected/undesirable things happening at secure screens, on which NVDA has full access to the system.

May be I'm just seeing lions and bears here, though.

[lukaszgo1]

A few more points which might help us decide what to do here:

1. .pyd files provided by NVDA itself don't have Python specific extension. Are we certain that all .pyd files included inside add-ons would have it? If we don't then we might break absolutely well constructed add-on by mistake.
2. We don't protect users from this problem when non secure copy is active. My understanding is that the worth that might happen when .pyd file for different Python version than one currently used is imported is crash of NVDA. For more experienced users Narrator is as easily runable on secure screens as everywhere else, and for those not as computer proficient crash of NVDA regardless of the place means probably call for help to someone more experienced.
3. At the moment the only way in which add-ons might end up in the systemConfig is to install them on normal configuration, and then copy them. Can you imagine a situation in which it would crash on secure screens but not during normal NVDA session? If the answer is no such badly constructed add-on wouldn't even have a chance to be copied to the secure configuration.

[feerrenrut]

A solution to this will need to be in 2019.3, I have added the milestone and added this to the python 3 project board for tracking purposes. I haven't read through this to get up to speed on it yet, but in case it hasn't been suggested, perhaps 2019.3 should require a clean install (all NVDA files removed prior to installation). Most add-ons will need to be reinstalled anyway.

[lukaszgo1]

@feerrenrut The only remaining think before this can be merged is decision about removing .pyd files from add-ons on secure configs. The issue lies with the fact that some add-ons might want to support both old Python 2 version of NVDA and the new Python 3 based one. My preference would be not to be overprotective and let add-on authors be as compatible as they want. If you agree this can be reviewed as is. Removing everything during 2019.3 update wouldn't solve this issue as all compiled .pyd files would be removed from configs even from portable copies which is not what we want.

[lukaszgo1]

@feerrenrut Is the problem more clear now, or would you like any additional explanations? I think this should be reviewed and merged as soon as possible because modifying installer shortly before the release might cause issues. It is also important to point out that we cannot leave installer in the current state - this breaks add-ons for portable configs.

[lukaszgo1]

It turns out that in some Python packages included .pyd files do not have Python specific extension - one example is PyWin32. Unless someone has a better way of checking for which version of Python given .pyd file was compiled we cannot delete them.

[lukaszgo1]

Someone on NVDA add-ons list complained that each update of portable NVDA Alpha removes all .pyd files from his add-on. It turns out that as I've explained in my last commend he is using Pywin32 and these files do not have Python specifiic suffix. Can this be reviewed and merged please?

[michaelDCurran]

This should only be done at the root of destPath. Currently this is being done in every subdirectory.

[lukaszgo1]

I am afraid I have no idea how to fix this.

[LeonarddeR]

It should be enough to check whether curDestDir is equal to destPath.

[michaelDCurran]

I think that ```curDestDir == destPath``` Should be enough. But this would need to be tested.

[lukaszgo1]

@michaelDCurran Done.
I've tested as follows:

1. Created portable copy of 2019.2.1,, placed some .pyd files in the UserConfig, systemConfig and in the directory called systemConfig in brailleDisplayDrivers.
2. Upgraded it with launcher created from this branch.
3. Ensured that .pyd files are still present in the userConfig and SystemConfig, and that they are removed from systemConfig in brailleDisplayDrivers.